? ? Failure

User tests: Successful: Unsuccessful:

avatar andrepereiradasilva
andrepereiradasilva
27 Nov 2016

Summary of Changes

This makes com_plugins respect the protected status of plugins, ie, when the plugin extension is protected it will not be able to be enabled/disabled.

Testing Instructions

  1. Code review
  2. Apply patch
  3. Check in extensions manage which plugins are protected.
  4. Go to plugins and check those plugins are now protected (cannot be enabled or disabled) in the list view and also on edit plugin form.
  5. Try to disable/enable protected plugins. You should NOT be able to do it.
  6. Try to disable/enable unprotected plugins. You should be able to do it.

Documentation Changes Required

None.

B/C considerations

This could have some B/C considerations, since current protected plugins will not be able to be enabled/disabled.
Also to not have plugins etternaly disabled this will enable all protected plugins on update. This will make user disabled protected plugins enabled, which, dependening on which plugins, can have consequences.
Mantainers please check if this is acceptable or not, and in that case only for 4.0.

@mbabker i think this was what you're talking about, please check if ok.

avatar andrepereiradasilva andrepereiradasilva - open - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Nov 2016
Category Administration com_plugins Language & Strings Templates (admin)
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar mbabker
mbabker - comment - 27 Nov 2016

This is going to require an update statement because you will otherwise lock plugins which were disabled in that state. And I will tell you I do disable plugins my sites do not use (including the cache and language filter plugins, which are protected).

That brings up issue 2. If we're going to lock out these plugins from being disabled, we need to make sure they actually have a reason to be in a protected state. I can't think of a valid reason to force the language filter plugin to be enabled on a single language site as an example. Sadly, addressing this will either have to be accepted before this PR or as part of this PR.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

This is going to require an update statement because you will otherwise lock plugins which were disabled in that state. And I will tell you I do disable plugins my sites do not use (including the cache and language filter plugins, which are protected).

you're right

That brings up issue 2. If we're going to lock out these plugins from being disabled, we need to make sure they actually have a reason to be in a protected state. I can't think of a valid reason to force the language filter plugin to be enabled on a single language site as an example. Sadly, addressing this will either have to be accepted before this PR or as part of this PR.

That's already done in my other PR #13037
Check the last part of the update sql - after -- Now protect from disabling essential core extensions.

avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 27 Nov 2016
Category Administration com_plugins Language & Strings Templates (admin) SQL Administration com_admin Postgresql MS SQL com_plugins Language & Strings Templates (admin)
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

update sql done.

avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar brianteeman
brianteeman - comment - 27 Nov 2016

Maybe i am thick or i am missing something. But

  1. What is the problem this is solving
  2. Why the change to force an extension to be enabled if it is protected.
avatar mbabker
mbabker - comment - 27 Nov 2016

What is the problem this is solving

The extension manager does not let you change the published state of any protected extension, the plugin manager does. That inconsistency is fixed here.

Why the change to force an extension to be enabled if it is protected.

It seems to be our design right now that a protected extension is supposed to be enabled and cannot be uninstalled. We have another PR splitting this definition leaving the protected column to only "lock" the published state. The update statement is required to re-enable any extension which is flagged as protected which the user might have disabled.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

@brianteeman for this matter, there are to behaviours in extensions:

  • cannot be enable/disabled (let's call this protected)
  • cannot be uninstalled (let's call this locked)

As it is now the two are somewhat fused in protected flag, but this leads to some problems, because we can't make an extension that can't be uninstalled but can be enabled/disabled (ex: com_newsfeeds). We currently can either protected it and then cannot be enabled/disabled nor unisntalled, or, you not protected it and then can be disabled and/or unistalled.

So i made the other PR #13037 to split this protected in two different states: protected (cannot be enable/disabled) and locked (cannot be uninstalled).
This locked will also allow, for instance, for a 3RD to use this to not allow users to uninstall, for instance their framework.

Reggarding the issue this PR intends to solve.

Plugins, a special case, use the extensions table for their enabled/disabled status, which means when you disable a plugin in com_plugins you are in fact disabling the plugin extension.
Now if the extension is protected (for instance joomla extension plugin) you CANNOT disable it in the extensions manage ... but you CAN disable it in com_plugins ... which does not makes sense.

i'm not inventing issues here 😉

avatar brianteeman
brianteeman - comment - 27 Nov 2016

I understand the ui is confusing and not ideal but I just don't see the
issue that splitting this all up will address. Must be just me

avatar mbabker
mbabker - comment - 27 Nov 2016

PR #13037 will allow us to mark an extension as something you cannot uninstall for whatever reason (core extensions which if you uninstall screw up the update process because of the missing database tables) but still allow the extension to be enabled/disabled on the site (so we want to keep a user from uninstalling com_contact but still let them turn it off; right now if it were protected you wouldn't be able to turn it off).

This PR makes the behavior that exists in the extension manager carry forward into the plugin manager regarding protected extensions and the inability to enable/disable them.

avatar mahagr
mahagr - comment - 28 Nov 2016

I'm having a small issue with this change: in the past protected for plugins meant "locked" and not "protected". So basically plugins that used to be disabled may suddenly become enabled and changing the behaviour of production site without a notice to the admin.

This is true not only with packages locking the plugins from uninstall, but I'm also maintaining a site which disables Joomla core plugin User - Joomla which is basically replaced by custom version which customizes much of the plugin.

I'm not against this change, but I'm just saying that changes like this need to be somehow informed to the admins and developers. I'm also worried that this change accidentally goes into some patch version where people are not expecting changes in how their sites behave.

avatar brianteeman
brianteeman - comment - 28 Nov 2016

@mahagr if that change in behaviour is correct (not tested it myself) then this is not backwards compatible

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

the potential (depending on which locked plugins are disabled) B/C issue is what i said in B/C considerations section of this PR.

This could have some B/C considerations, since current protected plugins will not be able to be enabled/disabled.
Also to not have plugins etternaly disabled this will enable all protected plugins on update. This will make user disabled protected plugins enabled, which, dependening on which plugins, can have consequences.
Mantainers please check if this is acceptable or not, and in that case only for 4.0.

But i also want to say, this PR should never be merged without the other one being merged (#13037), and the other one limits the number of protected extensions to very few plugins.

avatar mbabker
mbabker - comment - 28 Nov 2016

It's a more complex issue than just whether or not some plugins are going to get re-enabled. If you go by the way the extension manager's UI is presented, those plugins should have never been able to be disabled to begin with.

The extension manager's UI doesn't let you disable anything that is protected. The plugin manager's does. So for the sake of consistency, this is a correct PR. The other option is it should be flipped and the extension manager stop locking the ability to enable/disable extensions that are protected (at which point adding the locked column as done in the other PR becomes unnecessary and we just change that PR to protect every "core" extension from being uninstalled). Either way, the main problem addressed with this PR is that you have two screens which offer the same general functionality behaving in two different ways and this breaks expectations.

#13037 is a prerequisite to this being merged. As pointed out, it changes the list of defined protected extensions. It also splits the definition of protected into two fields. That new field honestly doesn't mean anything in the context of the plugin manager, but the plugin manager should most assuredly respect the protected column as it is currently defined.

B/C or not, this combination of PRs does eventually need to be merged into core. Otherwise what's really the point of defining attributes when our own core API doesn't respect them?

avatar infograf768
infograf768 - comment - 28 Nov 2016

Tested #13037 + this one.
Looks like working here, i.e. when I disable a plugin in either of the interface (Manage or Plugins Manager), the other interface reflects the change.

avatar infograf768
infograf768 - comment - 29 Nov 2016

can't we normalise this? Here is what we get for modules:
screen shot 2016-11-29 at 07 42 19

avatar andrepereiradasilva
andrepereiradasilva - comment - 29 Nov 2016

You get that when a module extension is disabledd but you are using that module. Is not related to the module extension being protected or not. Also having a protected module extension does not means the modules that use that module extension cannot be unpublished in the same way that com_content extension being protected doesn't mean all article cannot be unpublished.

This is only the case for plugins and this is What this pr does.

avatar Hackwar
Hackwar - comment - 29 Nov 2016

I understand that the behavior between com_installer and com_plugins is inconsistent, but I strongly object to not being able to disable a plugin. I wouldn't know a plugin that really always have to be on or rather why that should even be a plugin at all then. If it had to be enabled all the time, it would have to be part of the application code and not a plugin extension.

avatar andrepereiradasilva
andrepereiradasilva - comment - 8 Dec 2016

@Hackwar since this shouldn't me merged without #13037 being merged i think that is nto an issue.

As it is now, after #13037 there is no module or plugin protected.
#13037 (comment)

avatar izharaazmi
izharaazmi - comment - 10 Dec 2016

I agree to @Hackwar here. If that is a plug-in it must offer a way to plug-out and save energy 😄

In addition to that, I'd like to point out that I don't like the idea of having two places where we can enable/disable a plugin. For a module we need it because when we publish a module, we create an instance of the extension. But for a plugin the extension itself is the instance, like singletons (may be). I'd like to get rid of state column from the plugins altogether and rely solely on extension manager's set state for that purpose.

avatar mbabker
mbabker - comment - 27 May 2017

If this is something that's going to get seen through then it needs a new owner.

avatar franz-wohlkoenig franz-wohlkoenig - change - 27 May 2017
Status Pending Information Required
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Jun 2017

If this PR get no Response, it will be closed at 23th July 2017.

avatar franz-wohlkoenig franz-wohlkoenig - change - 23 Jul 2017
Status Information Required Closed
Closed_Date 0000-00-00 00:00:00 2017-07-23 08:31:12
Closed_By franz-wohlkoenig
avatar joomla-cms-bot joomla-cms-bot - close - 23 Jul 2017
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Jul 2017

closed as stated above.


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

avatar joomla-cms-bot
joomla-cms-bot - comment - 23 Jul 2017

Add a Comment

Login with GitHub to post a comment