[#17219] - Add locked field to extensions table, prevent uninstalling core extensions, restructure protected extension list
- Closed
- 9 Sep 2018
- Medium
- Build: 3.8-dev
- # 17219
- Diff
- mbabker:pr-13037
- Pending hound Hound is busy reviewing changes... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #13037
Summary of Changes
This is PR #13037 updated to the current 3.9 branch. See the original PR for added context and past conversation.
The long and short is this adds a new field, locked, to the extensions table, and splits the current definition of the protected field. This also updates the protected extension list to make the list only include admin components, core libraries, and the files extension that Joomla tracks itself with (this means all modules and plugins are now unprotected and can be enabled/disabled consistently).
Current:
protectedindicates an extension which cannot be disabled or uninstalled
New:
protectedindicates an extension which cannot be disabledlockedindicates an extension which cannot be uninstalled
Why This Distinction Matters
Uninstalling core extensions can be problematic, and honestly it's not very effective given our current packaging and upgrading solutions. Even if you do uninstall the extensions, they end up back on your site's filesystem during the update process because we don't make the package extraction step aware of the database and inherently the installed extensions. Also, uninstalling components takes their tables with them, and if an update includes a SQL delta for one of those tables, it causes the update to fail over. So we should take an extra step to protect users from doing things that could be dangerous for their sites. Next, an uninstallable extension should not mean that the extension must be enabled. The only extensions which should be protected are those which if disabled would critically bring down the site, every other aspect of the extension listing should be controllable by the site administrator.
Testing Instructions
With the patch applied and relevant SQL updates applied, the user should be unable to uninstall any core extension (on a correct install this is any extension with an ID less than 10000). The user should not be able to disable extensions which are critical to the management of the site (i.e. the extension manager, plugin manager, or update component, any of the libraries, or Joomla files extension) or are coupled to the core application stack (i.e. com_categories or com_content), all other extensions should be able to be disabled through the Extension Manager interface.
Documentation Changes Required
We should probably add a page explaining the various extension states and what these columns exist for if one doesn't already exist.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | SQL Administration com_admin Postgresql MS SQL com_installer Language & Strings Templates (admin) Installation Libraries |
| Labels |
Added:
?
?
|
||
| Labels |
Added:
?
Removed: ? |
||
Synced up.
Made some comments inline about the sql - obviously also need to be changed in sql and postgres - and i didnt check the install sql file but i guess it is the same
Other than comments inline all looks good to me
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
| Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
| Labels |
Added:
?
|
||
| Status | Ready to Commit | ⇒ | Pending |
| Labels | |||
I have tested this item
missed @Quy comment on first test sorry ;)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
When uninstalling many locked extensions, the same message is listed for each selected locked extension. Should the message be displayed once?
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
Locked extensions cannot be uninstalled.
We run one message per extension. We do not have logic anywhere in the system to do "if message already queued don't queue it again" type stuff.
How about disabling the checkbox if the extension is protected and locked?
How about disabling the checkbox if the extension is protected and locked?
I looked, can't do right now. JHtmlGrid::id() does not support adding a disabled attribute to the generated markup and adding it would be beyond scope of this PR.
| Labels |
Removed:
?
|
||
I don't understand yet how the update sql 3.8.0-2017-07-24.sql works. After doing the Database > Fix with staging, it appears to not be executed.
The database fix routine only runs schema changing statements (i.e. ALTER TABLE). It cannot run data statements (i.e. UPDATE).
Then how to run the data statements?
They would be run during a normal update routine.
The database fixer is a tool that can only validate the schema is in the corrected state. The fixer cannot (nor should anyone try to change this) validate that data statements have been executed.
| Labels |
Added:
?
|
||
Rebased to 3.9.
zero-24
- | Labels |
Removed:
?
|
||
Installed 3.9 dev.
Applied patch.
Fixed database.
In Extensions > Manage, under Locked column, all values are No.
Because the database fixer will not run data SQL statements (INSERT/UPDATE). It will only run schema SQL statements (i.e. ALTER TABLE). You either have to test this as a fresh install or use a release package built with this patch integrated and run the update through the update component.
Please advise how to test to trigger the following:
\JLog::add(\JText::sprintf('JLIB_INSTALLER_ERROR_COMP_UNINSTALL_LOCKED', $this->extension->name), \JLog::WARNING, 'jerror');
If I'm following the code diff right, you'd have to remove this check to allow the library checks to run (because that check is in essence doing the same thing the library API is checking, just at a point before the uninstall method in the library is triggered, so it saves a few CPU cycles otherwise it's just a redundant thing).
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
I have tested this item
With update from 3.8.8-dev, Upload & Update (com_joomlaupdate), package pr-13037.zip.
Tested ordering, filtering, uninstalling of locked items.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
I have tested this item
With a new installation of package pr-13037.zip.
Several tests following the Testing Instructions. Plus filtering, ordering.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.
If there is actually interest in merging this I will deconflict this PR one last time. Otherwise, I'm abandoning it because it is too complex to keep trying to rebase, get tested and reviewed, then have it sit for weeks on end.
For me it makes sense. Lets get it in sync and then merge.
I like this too.
Very welcome feature
For me you should fix the conflict and merge it ASAP.
| Labels |
Added:
?
?
Removed: ? |
||
Rebased.
I give up.
| Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-09-09 17:03:15 |
| Closed_By | ⇒ | mbabker |
Bad news! Great thank you for your past efforts concerning this feature!
It's not a end user facing feature that adds all sorts of bells and whistles to the UI (basically pure code level changes get next to no testing and therefore are difficult to impossible to get merged).
There seems to be a lack of interest from project leadership (which is the same reason I almost abandoned the privacy tools for 3.9 before that finally getting the attention needed to merge it).
TBF, resolving the merge conflicts for this change is a massive pain in the ass, and it would probably be damn near impossible to actually merge this to the 4.0 branch because of the number of conflicts it will create in the SQL files.
All in all, I'm not interested in "owning" issues and pull requests anymore that I know are going to improve Joomla but aren't going to actually get the attention they deserve unless I put the time into them. And my time is already overbooked, with some project teams wanting me to re-prioritize things now. If someone else wants to try and see this through, more power to you (the diff is still readily accessible through the GitHub interface and always will be); same goes for every RFC, feature request, or architectural concern I've raised and decided to abandon. But, I don't have the energy to keep fighting for changes that I perceive are moreso me fighting against the system than introducing welcome change.
Ok thx for the answer. I also checked some of the other PR you dropped and its a bit sad seeing them closed. But I understand that the main task atm is to get a working J4 beta.
If someone else wants to put in the effort then go for it, but I'm done trying.
wanted to test this but there a lot of conflicts