? ? Failure

User tests: Successful: Unsuccessful:

avatar andrepereiradasilva
andrepereiradasilva
27 Nov 2016

Summary of Changes

This adds a new column to extensions table named locked.

This column is a flag 1 or 0 to check if extension can be uninstalled.

This PR also marks all core extensions as uninstallable (ie locked), but most are unprotected now so they can be disabled.

Testing Instructions

First do a code review.

Two test needed: update, install

  • Update

    • Apply patch
    • Run the update query manually
    • Check in extensions manage if protected extensions are protected and locked extensions are marked as so
    • Check you cannot unninstall a locked extension but you can disable it if it's not protected.
  • Install

    • Apply patch
    • Delete configuration.php and install joomla as usual
    • Check in extensions manage if protected extensions are protected and locked extensions are marked as so
    • Check you cannot unninstall a locked extension but you can disable it if it's not protected.

This needs to be tested in all 3 db systems.

Documentation Changes Required

None.

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 SQL Administration com_admin Postgresql MS SQL com_installer Language & Strings Installation
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add core field to extension (for core extensions)
[RFC] Add core field to extensions table (for core extensions)
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add core field to extension (for core extensions)
[RFC] Add core field to extensions table (for core extensions)
avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

@mbabker @richard67 @ggppdk @brianteeman @alikon @infograf768 would appreciate your opinions here.

avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add core field to extension (for core extensions)
[RFC] Add core field to extensions table (for core extensions)
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Labels Added: ? ?
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

also @alikon can you please check postgresql and sqlsrv sql staments to make sure they are ok?

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

@andrepereiradasilva It looks very good from my opinion, assuming it works as described and from a short look on the code changes. Unfortunately I have no time to test.

avatar mbabker
mbabker - comment - 27 Nov 2016

Personally, I'm not a fan of introducing a data point to differentiate between a "core" extension and third party. I honestly feel like if we need this level of distinction then we're messing something else up somewhere.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

IMHO a user should not be able to uninstall core extensions, if you can uninstall it, it should not be a core extensions, but a core supported extension.

So with this PR we can know which are the core exxtensions so we avoid them from being uninstalled... i honestly, as it is, don't see another way to check witch extensions are core extensions.

avatar mbabker
mbabker - comment - 27 Nov 2016

If an extension should not be uninstallable, it should be protected. A design flaw in our system also means it cannot be disabled. Adding an additional column to track this state IMO is unnecessary. We should not need to know nor care about what the core extensions are in the database.

if you can uninstall it, it should not be a core extensions, but a core supported extension

Considering PLT basically gave up on decoupling, at this point we should really just put weblinks back into core and we don't need to worry about making this distinction then. We can just protect everything then fix the design flaws that make protected extensions locked in an active state. Which is if I'm not mistaken entirely UI driven; the extension manager forcibly displays an inactive lock icon over protected extensions but if you go to the plugin manager you can freely enable/disable protected plugins (look at CodeMirror). So to me that indicates that the concept of a protected extension is already inconsistent in our interface and that just because an extension is protected does not mean it cannot be safely disabled.

avatar mbabker
mbabker - comment - 27 Nov 2016

The other thing. There should be a difference between protected as in install/uninstall and protected as in state. A "core" column does not either any of those. In theory, anyone should be able to use any column in any of our database tables for their extensions. So if someone wants to create a package extension, install all of the package's extensions, set their state to protected, and change that state when uninstalling the package (there's a very crude workaround for the "user is able to install a package then separately uninstall its extensions" issue 😉) there should be nothing stopping them.

avatar brianteeman
brianteeman - comment - 27 Nov 2016

If the objective is to be able to remove unused core extensions from the
admin menu then the problem is the hard coded nature of the current admin
menu and this can be easily resolved, as I have said for years, with a
proper menu manager for the admin. Something that thankfully we are very
close to.

avatar dgt41
dgt41 - comment - 27 Nov 2016

@brianteeman on it's way: #13036

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

there are also other issues, for instance, as it is now, we can remove a lot of core extensions and them if we somehow update their tables (INSERT/UPDATE/DELETE/ALTER/etc) with sql updates the joomla update will fail with sql errors ...

avatar mbabker
mbabker - comment - 27 Nov 2016

Still doesn't justify a "core" column that is only usable by the core extensions. If it's in the database, everyone can use it.

IMO we need to redefine the existing data points and behaviors, in part because of the inconsistencies I pointed out above.

  • What does "protected" mean?
    • If it means "cannot disable and cannot uninstall" do we need to split this definition into two columns?
    • If it means "cannot uninstall", do we need a second column to represent a protected state (cannot disable)?
    • If it means "cannot disable", do we need a second column to represent protected install status (cannot uninstall)?
  • If a plugin is protected, should it be able to have its state changed in the plugin manager?

Let's just start there, there is a lot more to figure out too (i.e. inter extension dependencies, we have PHP API that should be used for this but do we somehow introduce either schema definitions in the manifest or additional data columns to further define this relationship?).

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

What does "protected" mean?

If it means "cannot disable", do we need a second column to represent protected install status (cannot uninstall)?

This is exactly what core does in this PR. core = cannot uninstall and protected = cannot disable

If a plugin is protected, should it be able to have its state changed in the plugin manager?

i agree this should be solved also.

avatar brianteeman
brianteeman - comment - 27 Nov 2016

I know it's on its way. Finally. That's why I raised it

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

the menu stuff (which is welcomed) is another issue, somewhat related to this, but this is a different issue.

avatar mbabker
mbabker - comment - 27 Nov 2016

This is exactly what core does. core = cannot uninstall

I don't agree with this. "core" to who, Joomla? Or in the case of a large extension package like Kunena, is it part of their core infrastructure and something that shouldn't be uninstalled separately? The column should not be named "core". And if the column is representing "cannot uninstall", then existing behaviors regarding the existing protected column need to be validated to only block changing state and consistently across all managers; protected on its own should not block uninstall if this is the direction you're going.

Remember, this is in the database. This can be set by any developer. So we should not add columns to the database schema that are "special cases only usable by the core Joomla distribution". Like it or not, this means if a developer wants to set an extension to a protected (state) or uninstallable condition, it's allowable. And like I pointed out earlier, there is a valid case for devs to want to do this.

avatar brianteeman
brianteeman - comment - 27 Nov 2016

So what is the main issue then. I don't see it

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

so @mbabker you most problem is the naming? core to other thing? do you have a suggestion? locked? blocked? other?

avatar mbabker
mbabker - comment - 27 Nov 2016

Changing the name would definitely help. It should be clear what its purpose is (if you're blocking uninstall, maybe locked works best here). As long as it is not a column whose single purpose is to distinguish a core extension versus a third party extension, I'm OK with going forward with things.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

of course this is a different concept i initially tought, because you will not be able to use this to check the core extensions this way - but that was not my most important objective

but it would solve the most essencial issue to me, not be able to uninstall core extensions to prevent errors but still be able to disable them. so i would be happy with it too

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

@brianteeman read my last comment

avatar mbabker
mbabker - comment - 27 Nov 2016

I still don't see why Joomla needs code within it (beyond what we already have in our update processing because none of core gets updated as an extension would) to distinguish a "core" extension installed with the distributed package versus an extension installed from anywhere else. It's one of those things I'm adamant about; if we are coding special conditions for the core extensions I feel like we messed something else up somewhere (and we already have those conditions unfortunately).

I support the notion of an attribute which blocks uninstall, we obviously need it. As long as we clearly define the attribute and distinguish it from the existing protected value, IMO we're on the right track.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

ok so i will change all core to locked and messages also.

them we need to had also a way to process this new field on extension install, but that will be another PR.

avatar mbabker
mbabker - comment - 27 Nov 2016

them we need to had also a way to process this new field on extension install, but that will be another PR.

Leave it alone. If someone wants to change this they can use the extension install script and the hooks it offers, I don't think we need an attribute in the manifest for this.

Use case - I want to prevent users who install my package extension from uninstalling its individual extensions. I can set this in the database with an update query and it "locks" the behavior in the UI correctly and when uninstalling the package I can reverse it with another update query. If we were to set this as an attribute in the manifest and make the in code checks based on that, something that was installed with this attribute set would never be uninstallable without putting a different version of the manifest into the filesystem.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

ok

avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add core field to extensions table (for core extensions)
[RFC] Add locked field to extensions table (and use it in core extensions)
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add core field to extensions table (for core extensions)
[RFC] Add locked field to extensions table (and use it in core extensions)
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

updated to locked and modified PR title and description

avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Labels Added: ? ?
avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

still need to allow unistalling protected, but not locked extensions to be uninstalled.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

All core extensions are already locked to not be uninstalled in this PR.

But, for this to go ahead we need to be sure which core extensions will be protected, ie, cannot be enabled/disabled. In other words, extensions which without them enabled, joomla will not work properly.
Currently i have this:

  • component: com_mailto, com_admin, com_cache, com_categories, com_checkin, com_cpanel, com_installer, com_languages, com_login, com_media, com_menus, com_modules, com_plugins, com_templates, com_content, com_config, com_users, com_joomlaupdate, com_ajax, com_postinstall
  • module: mod_login, mod_menu, mod_quickicon, mod_toolbar
  • plugin
    • system: logout
    • content: joomla
    • user: joomla
    • editors: codemirror, none
    • authentication: joomla
    • installer: packageinstaller, folderinstaller, urlinstaller
    • extension: joomla
  • library: phputf8, joomla, idna_convert, fof, phpass
  • language: en-GB
  • file: joomla
  • package: pkg_en-GB

But i'm not enterily sure if this is 100% correct

avatar brianteeman
brianteeman - comment - 27 Nov 2016

Are those the site or admin modules?

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

errr... both. i didn't made a distintion, should i have?

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

BTW why is templates locked to codemirror? can't it work without codemirror?

avatar dgt41
dgt41 - comment - 27 Nov 2016

@andrepereiradasilva it's the only editor with native formatting for php, css, js etc...

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

but if you disable the codemirror plugin will work right? just with no formatting, am i right?

avatar dgt41
dgt41 - comment - 27 Nov 2016

yup, or at least it should (haven't try to be honest)

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

yes, it seems it fallback to use editor none so not essencial.

avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

ok so i reduced the number of protected extensions:

  • component: com_mailto, com_admin, com_cache, com_categories, com_checkin, com_cpanel, com_installer, com_languages, com_login, com_media, com_menus, com_modules, com_plugins, com_templates, com_content, com_config, com_users, com_joomlaupdate, com_ajax, com_postinstall (same)
  • module: mod_login, mod_menu, mod_quickicon, mod_toolbar (same, but now only for admin)
  • plugin (removed several)
    • user: joomla
    • editors: none
    • authentication: joomla
    • installer: packageinstaller
    • extension: joomla
  • library: phputf8, joomla, idna_convert, fof, phpass (same)
  • language: en-GB (same)
  • file: joomla (same)
  • package: pkg_en-GB (same)
avatar joomla-cms-bot joomla-cms-bot - change - 27 Nov 2016
Category SQL Administration com_admin Postgresql MS SQL com_installer Language & Strings Installation Templates (admin) SQL Administration com_admin Postgresql MS SQL com_installer Language & Strings Templates (admin) Installation Libraries
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add locked field to extensions table (and use it in core extensions)
Add locked field to extensions table (and use it in core extensions)
Labels Removed: ? ?
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Title
[RFC] Add locked field to extensions table (and use it in core extensions)
Add locked field to extensions table (and use it in core extensions)
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
The description was changed
avatar andrepereiradasilva andrepereiradasilva - edited - 27 Nov 2016
avatar andrepereiradasilva
andrepereiradasilva - comment - 27 Nov 2016

ok removed RFC status, ready for review/tests.

6e92338 27 Nov 2016 avatar andrepereiradasilva ups
avatar andrepereiradasilva andrepereiradasilva - change - 27 Nov 2016
Labels Added: ? ?
avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

already corrected that

avatar infograf768
infograf768 - comment - 28 Nov 2016

My tests show here with a new install containing this PR + #13040 that one cannot uninstall anything core. Is it what is desired?

avatar mbabker
mbabker - comment - 28 Nov 2016

Yes. It blocks a user from uninstalling the core extensions (which end up back on the filesystem anyway during upgrades) in part to prevent possible SQL errors during the upgrade when a component has been removed.

avatar andrepereiradasilva
andrepereiradasilva - comment - 28 Nov 2016

so the number of extensions that is need to be protected is reduced again

  • component: com_mailto, com_admin, com_cache, com_categories, com_checkin, com_cpanel, com_installer, com_languages, com_login, com_media, com_menus, com_modules, com_plugins, com_templates, com_content, com_config, com_users, com_joomlaupdate, com_ajax, com_postinstall (same)
  • module: mod_login, mod_menu, mod_title, mod_toolbar (replace mod_quickicon by mod_title)
  • plugin (removed packageinstaller)
    • user: joomla
    • editors: none
    • authentication: joomla
    • extension: joomla
  • library: phputf8, joomla, idna_convert, fof, phpass (same)
  • language: en-GB (same)
  • file: joomla (same)
  • package: pkg_en-GB (same)

I have doubt reggarding if those plugins need always to be enabled. authentication: joomla and user joomla in particular.

avatar andrepereiradasilva
andrepereiradasilva - comment - 8 Dec 2016

fixed conflicts and now the number of extensions that is need to be protected is reduced again
All plugins and modules are now unprotected.

Locked Extensions (cannot be uninstalled by the user)

All core extensions.

Protected Extensions (cannot be enabled/disabled by the user)

  • component: com_mailto, com_admin, com_cache, com_categories, com_checkin, com_cpanel, com_installer, com_languages, com_login, com_media, com_menus, com_modules, com_plugins, com_templates, com_content, com_config, com_users, com_joomlaupdate, com_ajax, com_postinstall
  • library: phputf8, joomla, idna_convert, fof, phpass
  • language: en-GB (site), en-GB (admin)
  • file: joomla
  • package: pkg_en-GB

Please test.

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:29:01
Closed_By franz-wohlkoenig
avatar joomla-cms-bot
joomla-cms-bot - comment - 23 Jul 2017
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/13037.

Add a Comment

Login with GitHub to post a comment