Skip to content

Add locked field to extensions table, prevent uninstalling core extensions, restructure protected extension list#17219

Closed
mbabker wants to merge 55 commits intojoomla:3.10-devfrom
mbabker:pr-13037
Closed

Add locked field to extensions table, prevent uninstalling core extensions, restructure protected extension list#17219
mbabker wants to merge 55 commits intojoomla:3.10-devfrom
mbabker:pr-13037

Conversation

@mbabker
Copy link
Contributor

@mbabker mbabker commented Jul 24, 2017

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:

  • protected indicates an extension which cannot be disabled or uninstalled

New:

  • protected indicates an extension which cannot be disabled
  • locked indicates 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.

@Quy
Copy link
Contributor

Quy commented Apr 23, 2018

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');

@mbabker
Copy link
Contributor Author

mbabker commented Apr 23, 2018

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).

@Quy
Copy link
Contributor

Quy commented Apr 23, 2018

I have tested this item ✅ successfully on 9ccc14e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.

@ReLater
Copy link
Contributor

ReLater commented Apr 23, 2018

I have tested this item ✅ successfully on 9ccc14e

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.

@ReLater
Copy link
Contributor

ReLater commented Apr 23, 2018

I have tested this item ✅ successfully on 9ccc14e

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.

@Quy
Copy link
Contributor

Quy commented Apr 23, 2018

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17219.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 23, 2018
@mbabker
Copy link
Contributor Author

mbabker commented Jun 2, 2018

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.

@laoneo
Copy link
Member

laoneo commented Jun 2, 2018

For me it makes sense. Lets get it in sync and then merge.

@infograf768
Copy link
Member

I like this too.

@ReLater
Copy link
Contributor

ReLater commented Jun 28, 2018

Very welcome feature

@csthomas
Copy link
Contributor

csthomas commented Jul 3, 2018

For me you should fix the conflict and merge it ASAP.

@mbabker
Copy link
Contributor Author

mbabker commented Jul 4, 2018

Rebased.

@mbabker
Copy link
Contributor Author

mbabker commented Sep 9, 2018

I give up.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 9, 2018
@ReLater
Copy link
Contributor

ReLater commented Sep 10, 2018

Bad news! Great thank you for your past efforts concerning this feature!

@HLeithner
Copy link
Member

@mbabker whats the problem with this PR I would love it, I use custom SQL queries file after installing joomla to disable core extensions and plugins and delete modules that are not needed.

So why didn't get it merged?

@mbabker
Copy link
Contributor Author

mbabker commented Sep 10, 2018

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.

@HLeithner
Copy link
Member

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.

@N6REJ
Copy link
Contributor

N6REJ commented May 13, 2019

@mbabker please make this happen. I understand your frustration.

@HLeithner
Copy link
Member

@mbabker any chance that you rebase it to j4 and would really like to see this in j4...

@mbabker
Copy link
Contributor Author

mbabker commented May 13, 2019

If someone else wants to put in the effort then go for it, but I'm done trying.

@richard67
Copy link
Member

Woever wanted this feature to happen: Please test PR #28462 . It is a redo of this one here by @Quy for 4.0-dev.

@richard67
Copy link
Member

Testing especially for update will be simplified because we have new donload links for packages at the bottom of that PR #28462 in the test result details on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.