Skip to content

Conversation

@vinespie
Copy link
Contributor

@vinespie vinespie commented Oct 19, 2016

Summary of Changes

Exclude components in discover state from list.

Method "getComponents" in class JComponentHelper return all components, even those who are in the state to discover. Must add state in queries.

Test instructions

  1. In joomla components dir, create a directory named "com_test" for example,
  2. In this dir, create one file named "router.php",
  3. Edit this file and paste this code :
    <?php defined('_JEXEC') or die; die('Router call'); ?>
  4. In this dir, create one file named "test.xml",
  5. Edit this file and paste this :
    <?xml version="1.0" encoding="utf-8"?> <extension type="component" version="3.1" method="upgrade"> <name>com_test</name> <files folder="site"> <filename>router.php</filename> </files> </extension>
  6. In backend, go to "Extensions => Manage => Discover"
  7. Click "Discover" button, you must see "com_test" in list,
  8. Go to fronted, you must see message "Router call",
  9. Apply patch,
  10. Go to fronted, no message, router not loaded

Related pull request

#8986 Parse preprocess rules from component routers : router of all components are loaded, even those that are not installed.

@mbabker
Copy link
Contributor

mbabker commented Oct 19, 2016

The pull request you've linked (and commented on) is completely unrelated to this proposed change. Please describe why you think this change is necessary.

@andrepereiradasilva
Copy link
Contributor

yes, and add test instructions too (the test instructions header is there when you create a pull request for a reason ...)

@vinespie
Copy link
Contributor Author

vinespie commented Oct 19, 2016

@mbabker @andrepereiradasilva description update with test instructions

@brianteeman
Copy link
Contributor

Can you ever have an extension in the database that does not have a status of 0
From what I can guess you are trying to exclude extensions that are not installed - so therefore they are not in the database at all and they wont get returned by the existing query either

(I could be completely missing the point and you description has been lost in translation)

@mbabker
Copy link
Contributor

mbabker commented Oct 19, 2016

When you discover extensions, all found extensions are loaded into the database with state = -1. They remain in that state until they are fully installed or the records purged.

@vinespie
Copy link
Contributor Author

Yes @mbabker , extension with state = -1 (discover) are considered as installed.

@brianteeman
Copy link
Contributor

As I said the description and test instructions were incomplete and misleading. There is NO mention of doing a discover at all. Only to create the files

@vinespie
Copy link
Contributor Author

@brianteeman description update

@mahagr
Copy link

mahagr commented Oct 24, 2016

Alright, it looks like this change only affects on discovered components. It really should also prevent disabled components (enabled != 1) from messing up the site.

See the linked issue from above.

Prevent disabled extensions "enabled = 0"
@vinespie
Copy link
Contributor Author

Yes @mahagr , adding request condition for prevent disabled extensions.

@mbabker
Copy link
Contributor

mbabker commented Oct 24, 2016

I'd only add it to the load method. Adding the enabled condition on the isInstalled method makes that method report invalid results (it's possible to have an extension installed and disabled).

@vinespie
Copy link
Contributor Author

ok @mbabker

@mahagr
Copy link

mahagr commented Oct 24, 2016

I agree that it should be only on load() method.

BTW, this pull request will change how component helper works and may have some visible side effects on peoples sites. This is because of both getComponent() and getComponents() calls will not anymore return component that exists in the database, but isn't either installed or enabled.

After running some tests with J!3.6.3, it looks like disabling extension makes it to stop to work from frontend and hidden in backend, though in backed you can still access the extension by typing the option manually (a bug?). On the other hand discovered component seems to be fully working as long as it has been enabled (another bug?).

I need to test the same with the patch applied; I'll find some time for that.

@mahagr
Copy link

mahagr commented Oct 25, 2016

I just tested all the combinations with the patch (disabled, discovered | enabled, discovered, disabled, installed | enabled, installed) and it looks like that it doesn't change much in the admin -- except that discovered/enabled component is shown in the admin menu, but you cannot access it. You cannot access disabled component either.

This is an improvement over discovered or disabled component being fully accessible in admin, but it would be nice to have another patch hiding the admin menu items in that case (I've seen sites with discovered but enabled components before -- not sure how you end up getting them, maybe failed install?)

Copy link

@mahagr mahagr left a comment

Choose a reason for hiding this comment

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

Fix looks good to me.

@christianboulbi
Copy link

I have tested this item ✅ successfully on 4d3775f

test von Christianboulbi


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

@perrez
Copy link

perrez commented Aug 22, 2017

I have tested this item ✅ successfully on 4d3775f


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 22, 2017
@ghost
Copy link

ghost commented Aug 22, 2017

RTC after two successful tests.

@mbabker
Copy link
Contributor

mbabker commented Aug 23, 2017

Merge conflict needs to be addressed.

@ghost
Copy link

ghost commented Oct 30, 2017

@vinespie can you please resolve conflicting Files?

@mbabker
Copy link
Contributor

mbabker commented Jul 7, 2018

Replaced by #21010 with merge conflicts resolved.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants