Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add locked field to extensions table (and use it in core extensions) #13037

Closed
wants to merge 34 commits into from

Conversation

andrepereiradasilva
Copy link
Contributor

@andrepereiradasilva andrepereiradasilva commented Nov 27, 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.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Nov 27, 2016
@andrepereiradasilva andrepereiradasilva changed the title [RFC] Add core field to extension (for core extensions) [RFC] Add core field to extensions table (for core extensions) Nov 27, 2016
@andrepereiradasilva
Copy link
Contributor Author

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

@andrepereiradasilva
Copy link
Contributor Author

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

@richard67
Copy link
Member

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

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 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.

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Nov 27, 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.

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 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.

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 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.

@brianteeman
Copy link
Contributor

brianteeman commented Nov 27, 2016 via email

@dgrammatiko
Copy link
Contributor

@brianteeman on it's way: #13036

@@ -129,15 +129,13 @@ WHERE (`type` = 'component' AND `element` IN (
'com_ajax',
'com_postinstall'
))
OR (`type` = 'module' AND `element` IN ('mod_login', 'mod_menu', 'mod_quickicon', 'mod_toolbar'))
OR (`type` = 'module' AND `element` = 1 AND `element` IN ('mod_login', 'mod_menu', 'mod_quickicon', 'mod_toolbar'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong element is never 1 ;) i guess it is meant to be client_id?

@andrepereiradasilva
Copy link
Contributor Author

already corrected that

@zero-24 zero-24 dismissed their stale review November 28, 2016 00:11

allrady fixed

@infograf768
Copy link
Member

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

@mbabker
Copy link
Contributor

mbabker commented Nov 28, 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.

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Nov 28, 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.

@andrepereiradasilva
Copy link
Contributor Author

andrepereiradasilva commented Dec 8, 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.

@mbabker mbabker added this to Testing/Review in [3.8] General Apr 21, 2017
@mbabker mbabker added the Ready to take over This is an abandoned feature which can be taken over by another person to finish it. label May 27, 2017
@mbabker
Copy link
Contributor

mbabker commented May 27, 2017

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

@joomla-cms-bot joomla-cms-bot removed the Ready to take over This is an abandoned feature which can be taken over by another person to finish it. label May 27, 2017
@ghost
Copy link

ghost commented Jun 22, 2017

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

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @franz-wohlkoenig by The JTracker Application at issues.joomla.org/joomla-cms/13037

@ghost
Copy link

ghost commented Jul 23, 2017

closed as stated above.


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

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.

None yet

8 participants