User tests: Successful: Unsuccessful:
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.
First do a code review.
Two test needed: update, install
Update
Install
This needs to be tested in all 3 db systems.
None.
Status | New | ⇒ | Pending |
Category | ⇒ | SQL Administration com_admin Postgresql MS SQL com_installer Language & Strings Installation |
Title |
|
Title |
|
Title |
|
Labels |
Removed:
?
?
|
Labels |
Added:
?
?
|
Labels |
Removed:
?
?
|
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 |
Labels |
Removed:
?
?
|
@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.
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.
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.
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.
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
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.
@brianteeman on it's way: #13036
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 ...
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.
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?).
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.
I know it's on its way. Finally. That's why I raised it
the menu stuff (which is welcomed) is another issue, somewhat related to this, but this is a different issue.
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.
So what is the main issue then. I don't see it
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.
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
@brianteeman read my last comment
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.
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.
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.
ok
Title |
|
||||||
Labels |
Removed:
?
?
|
Title |
|
updated to locked and modified PR title and description
Labels |
Added:
?
?
|
still need to allow unistalling protected
, but not locked
extensions to be uninstalled.
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:
But i'm not enterily sure if this is 100% correct
Are those the site or admin modules?
errr... both. i didn't made a distintion, should i have?
BTW why is templates locked to codemirror? can't it work without codemirror?
@andrepereiradasilva it's the only editor with native formatting for php, css, js etc...
but if you disable the codemirror plugin will work right? just with no formatting, am i right?
yup, or at least it should (haven't try to be honest)
yes, it seems it fallback to use editor none
so not essencial.
ok so i reduced the number of protected extensions:
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 |
Title |
|
||||||
Labels |
Removed:
?
?
|
Title |
|
ok removed RFC status, ready for review/tests.
Labels |
Added:
?
?
|
already corrected that
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.
so the number of extensions that is need to be protected is reduced again
I have doubt reggarding if those plugins need always to be enabled. authentication: joomla and user joomla in particular.
fixed conflicts and now the number of extensions that is need to be protected is reduced again
All plugins and modules are now unprotected.
All core extensions.
Please test.
If this is something that's going to get seen through then it needs a new owner.
Status | Pending | ⇒ | Information Required |
If this PR get no Response, it will be closed at 23th July 2017.
Status | Information Required | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-07-23 08:29:01 |
Closed_By | ⇒ | franz-wohlkoenig |
Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/13037
closed as stated above.
@mbabker @richard67 @ggppdk @brianteeman @alikon @infograf768 would appreciate your opinions here.