[#16724] - Move core extensions array from script.php to a new helper class to provide a safe way to check if core extension
- Fixed in Code Base
- 5 Jul 2017
- Medium
- Build: staging
- # 16724
- Diff
- richard67:get-core-extensions-2
- Success hound No violations found. Woof! Details
- Success continuous-integration/drone/pr the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #16579 .
Replaces Pull Request #16613 .
Previous discussions see #16579 and #16613 .
Summary of Changes
This PR moves the static list of core extensions used in script.php for updating the manifest caches to a new helper class for making it possible to be used by other functions, too.
Compared to the array as it was in script.php, the array in the new class is alpha-ordered within the particular sections for better maintainability.
In addition, the new class provides a function for check if an extension is core or not.
This provides a safe way to detect core extensions, while checking if extension ID is greater than a certain value is not a safe way. With a later PR then any of those unsafe checks can be replaced by using methods of the new helper class.
Testing Instructions
- Code review
- Test the new class as follows:
- Have error reporting set to maximum in global configuration, and if possible let php log errors into a file by setting log_errors to On and error_log to the full path to a log file in your php.ini.
- Add the following code to your frontend template's index,php, e.g. in case of the protostar template just after
<div class="body" id="top">:
<p style="font-weight: bold;"><?php echo "JExtensionHelper::getCoreExtensions():"; ?><p>
<p><?php $test1=JExtensionHelper::getCoreExtensions(); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::checkIfCoreExtension('component', 'com_mailto', 0, ''):"; ?><p>
<p><?php $test1=JExtensionHelper::checkIfCoreExtension('component', 'com_mailto', 0, ''); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::checkIfCoreExtension('component', 'com_blabla', 0, ''):"; ?><p>
<p><?php $test1=JExtensionHelper::checkIfCoreExtension('component', 'com_blabla', 0, ''); var_dump($test1); ?><p>
- Download latest nightly build of 3.7.3-dev, unpack it, add the new helper class and replace script.php by the one from this PR, pack the package again and update a pre-3.7.3-dev, e.g. a 3.7.2, to the patched nightly build by using the upload and install package method.
Expected result
The result of test no. 2 should look similar to follows (long stuff replaced by "..."):
JExtensionHelper::getCoreExtensions():
array(161) { [0]=> array(4) { [0]=> string(9) "component" [1]=> string(9) "com_admin" [2]=> string(0) "" [3]=> int(1) } [1]=> array(4) { [0]=> string(9) "component" [1]=> string(8) "com_ajax" [2]=> string(0) "" [3]=> int(1) } ... [160]=> array(4) { [0]=> string(8) "template" [1]=> string(9) "protostar" [2]=> string(0) "" [3]=> int(0) } }
JExtensionHelper::checkIfCoreExtension('component', 'com_mailto', 0, ''):
bool(true)
JExtensionHelper::checkIfCoreExtension('component', 'com_blabla', 0, ''):
bool(false)
Check that no PHP errors are shown or logged in your log file (if you use some).
The result of test no. 3 should be that the update succeeds without any errors and that there are no PHP errors reported from script.php in the log file (if used).
Actual result
There is no actual result because it is a new class.
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_admin Libraries |
Added real test no. 3 if script.php still works.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
Any idea where I would have to add unit tests for it?
I'm the wrong guy to ask regarding unit tests.
I'm the wrong guy to ask regarding unit tests.
And who would be the right guy?
With the unit tests, once you get to this directory the way things are organized should very closely resemble what is actually in the libraries directory. So this should be tested in a tests/unit/suites/libraries/cms/extension/JExtensionHelperTest.php file.
@mbabker And what all should be tested? The complete result of JExtensionHelper::getCoreExtensions(), too? This would mean to either maintain the same static array of core extensions also in the unit test, or to obtain that array somehow from the installation//joomla.sql file. Or would it be sufficient just to test JExtensionHelper::checkIfCoreExtension(...) with a few examples, like I do in test no. 2 of this PR? And should the unit tests be added with this PR or in a following PR?
Preferably, unit tests should always be included in a PR if adding or changing library level code. However, that's mostly not enforced and most library changes have no accompanying tests
getCoreExtensions() should validate it returns an array containing some known element
checkIfCoreExtension() should validate true/false returns given different scenarios (vaguely similar to your instructions)
| Labels |
Added:
?
|
||
Adapted test instructions for test no. 2 to latest commit.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
@richard67 Sorry, I am the wrong guy to ask too
The value of testing a static helper class is limited. Furthermore I have my doubts if implementing this as a static class is the way to go, it will make testing the classes who are using this helper harder. Will not help you @richard67 but it is a question the maintainer have to answer.
In terms of mocking dependencies, JExtensionHelper::getCoreExtensions() is no better than (new JExtensionHelper)->getCoreExtensions(); neither of them will let you mock the objects. And project wide we don't have the code discipline yet to enforce dependency injection (the 3.x codebase doesn't even make use of the DI container, at least 4.0 is moving that way). So even if we wanted to make it a class with non-static methods, it's not going to necessarily make things better in testing code that might use it.
The difference is: as static class you don't have the option.
| Category | Administration com_admin Libraries | ⇒ | Administration com_admin Libraries Unit Tests |
@mbabker Are the unit tests ok? Or is something missing? What I do not understand is if the tests are to be called somewhere or if all is complete now.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
| Labels |
Added:
?
|
||
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
Guess like that we have a good helper class also for the future.
@richard67 How do you test unit tests? I code review it. I had to pay extra attention because the array's elements are not in the same order as the function's parameters.
-
CI integration passes, that means the tests ran without errors.
-
Download the branch locally and run PHPUnit on your local system. Shouldn't fail if CI passed.
Can you make the plugins groups sorted alphabetically please - it will make it easier to find things
eg authentication, captcha, content, editors
Also it will be easier if the comments were on line and always with the type of extension
eg
Core plugins - system
Core modules - administrator
@brianteeman Beside what you mentioned in your comment above, do you think I should change the order of the elements in the array for 1 extension so it fits to the order of parameters of the checkIfCoreExtension function, i.e. from type, element, folder, client_id to type, element, client_id, folder?
@Quy mentined in his comment above that it would increase readability of the unit tests, and I would like to have a second opinion on that, because that would be really a big change being hard to review.
@Quy you want me to do that change of the array elements order?
@brianteeman Should the ordering of the other stuff be changed, too, so "Core module extensions - administrator" will come before "Core module extensions - site"?
@richard67 yes please for consistency
@brianteeman Well, if I order everything alpabetically, I also have to reorder the sections by types.
The result would be:
- Core component extensions
- Core file extensions
- Core language extensions - administrator
- Core language extensions - site
- Core library extensions
- Core module extensions - administrator
- Core module extensions - site
- Core package extensions
- Core plugin extensions - authentication
- Core plugin extensions - captcha
- Core plugin extensions - content
- Core plugin extensions - editors
- Core plugin extensions - editors xtd
- Core plugin extensions - extension
- Core plugin extensions - fields
- Core plugin extensions - finder
- Core plugin extensions - installer
- Core plugin extensions - quick icon
- Core plugin extensions - search
- Core plugin extensions - system
- Core plugin extensions - two factor authentication
- Core plugin extensions - user
- Core template extensions - administrator
- Core template extensions - site
You recommend to do it like that?
@brianteeman Done. Last commit includes your recommendations.
@Quy Do you recommend me to change the array elements order so it fits to the function call?
Regarding testing the unit tests, one way could be to make a new PR after this one is merged. The new PR will change some array element in the helper class so the unit test will fail, which should be shown by travis in the log file. But I do not ant to test that with this PR here because I do not want to mess it up with some bad commit, and so I would prefer to test that in a new one after this one is merged.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
@Quy Forget about my previous question about the array elements order. I think it is ok like it is now.
@Quy and @alikon Could you test again? Many many thanks in advance.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
needs this PR a second Test or can it be set RTC?
As far as I know every PR needs 2 tests, or not?
@richard67 correct. This PR have 2 Tests, but JTracker shows 1 successful. So between the 2 Tests were Changes which sometimes need a Test one more Time and sometimes it can set RTC. As i'm no Dev i ask.
It has 1 test only also on GitHub. The test results had been reset with my latest commit, and up to now only Quy has tested again.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
Thanks for testing @alikon .
@franz-wohlkoenig RTC?
| Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
@richard67 I appreciate that you wrote a unit test for the class, but I don't think it makes so much sense to test this. I fact we are testing here, if PHPs array command and in_array works, not really something we need to test. Please remove the unit test from this PR. Thanks.
@andrepereiradasilva you can do it, but then you have to compare the full array, what means that you have to copy the array from the class to the test.
But we have still the question, what makes it better? You change the helper because we have a new core extension, the test fails and than you add the new core extension to the test and the test is green. So you have to add it twice!
:) i mean just testing if the methods are working fine after whatever change is made. Not testing if the whole array is fine
Doesn't makes sense, we are then back to testing in_array
Agree with @rdeutz, this is not testing functionality / logic :-)
Btw. personally i would prefer another approach, like a column in #__extensions table for this functionality instead of an array. Because we just add another not obvious file, which needs to be maintained and updated on core changes.
personally i would prefer another approach, like a column in #__extensions table for this functionality instead of an array
If we could rely that the database updates always work properly and thus the data stored in the database is always accurate, then we could think about doing it with a column. But if that would be the case, we could as well just rely on the ID column (id below 10'000).
The whole point of this helper is that we can not trust that the data was updated properly.
I'm still not convinced we need a list of core extensions anywhere outside the update script, and we definitely don't need it as an API. But if we're going to have this somewhere outside the update script, I'd rather it be a PHP class that can't be manipulated versus a database column that extension developers can, and will, abuse.
@yvesh If you had read the previous discussions mentioned in the desciption of this PR, and the head line of this PR, then you would know that it is not as you say:
Because we just add another not obvious file
No, we move things from 1 file (script.php) into another so it can be used by other functions without having to create an instance of the whole script.php stuff, nothing else. This PR does not introduce anything new to be maintaned, it was all the time there in script.php!
@mbabker @Bakual @rdeutz I am also not convinced anymore if we need this hardcoded array.
I have made this PR because 2 other PRs by other people, #16474 and #16494 , attempted to use 999 or 9999 to check for extension ID being core, which is definitely not OK, see issue #16579 .
But meanwhile I saw that in administrator/components/com_installer/models/updatesites.php in function rebuild() a comment tells about to exclude core extensions (id < 10000), see here: https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/models/updatesites.php#L327.
But the function getJoomlaUpdateSitesIds() which was called to create the list of update site IDs to be excluded does not use any hardcoded extension ID number. Instead of this, it gets the update site ID by known core extensions, see here: https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/models/updatesites.php#L382.
Maybe both PRs #16474 and #16494 who deal with update sites, too, can help themselves in the same way, and then this PR here would not be needed (unless someone finds another place where an unsafe check if an extension is core or not is performed, but I did not find anything using 999 or 9999 or 10000.
But I am not deep enough in com_installer/models/updatesites.php and also not in those 2 new PRs to know now if it can be done in some other way or if this PR is useful still.
So @rdeutz if you want can you set this PR here to "needs review", and some experienced maintainer or PLT can check and decide. Maybe @wilsonge should be involved because he was mentor of one of these 2 new PRs I mentioned.
And I have no problems if this PR is closed. I have more problems if it is accepted and then later everybody complains about it.
All I wanted to do is to help the authors of these other 2 PRs not to go the wrong way.
Meanwhile I will remove the unit tests according to advice by @rdeutz .
| Labels |
Added:
?
|
||
| Category | Administration com_admin Libraries Unit Tests | ⇒ | Administration com_admin Libraries |
@alikon @Quy Your tests are still valid. The last commit only removed the unit tests as recommended by @rdeutz . So if you want you can set your test result again. But I am not sure anymore if this PR really is needed, see my previous comment.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
I would say close it. If we merge it we should mark it as depreciated for 4.0 anyway so if there are other ways, we can get rid of a static helper before we add it to the package. :-)
if i'm not wrong the main question is:
Do we need to checkIfCoreExtension() outside the update script ?
maybe no, but can be usefull and should not to be hard to mantain i guess ....
I thought that, too, but now I am not sure anymore.
Someone enlighten me, please. Outside of the update routine, what necessity is there for us to make a distinction between a core extension (one supplied by the main download, just so it's clear we're excluding something like weblinks which is decoupled) from a non-core extension?
What issue are we trying to address, other than philosophical?
future usage
@mbabker As I mentioned before 100 times, I tried to address the fact that there are 2 PRs in progress, #16474 and #16494 , which seem to need to make such a distinction because they use hardcoded 999 and 9999 to check for extension IDs. If we do nothing and let these 2 PRs get merged as they are, we will definitely have a bug (or 2).
*sigh*
So I guess we need something. Since we already have this data buried away somewhere, I guess it's at least a starting point to figure out how we do this.
well, maybe if i had more time i could find a way how these 2 PRs could use the update site of some known core extension and exclude it this way, similar as done in administrator/components/com_installer/models/updatesites.php here https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/models/updatesites.php#L327 and here https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/models/updatesites.php#L382 .
But I did not get deep enough into these 2 new PRs to see if that would be a way.
If it would, we would not need the core or not core check anywhere else up to now, as far as I could see.
| Milestone |
Added: |
||
@rdeutz @mbabker @Bakual @alikon @yvesh and all the others who were joining the discussion:
I had meanwhile time to check these 2 PRs in progress, #16474 by @pe7er and #16494 by @piotrmocko , where 999 or 9999 was used to check by extension ID if an extension is core or not.
For the 2nd PR #16494 I think it could be solved by joining the upate sites table or using it in a subquery here https://github.com/joomla/joomla-cms/pull/16494/files#diff-35b54662480daa18b046886a634f00d6R1013, excluding the update sites from known core extensions, similar as it is done here: https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_installer/models/updatesites.php#L382.
But the 1st PR #16474 would be the perfect use case for my PR here.
It would just need to replace the "$item->extension_id > 999" by a call to the "checkIfCoreExtension" function of my PR in the condition here: https://github.com/joomla/joomla-cms/pull/16474/files#diff-3bd7bed5d45e0d7de65bd3ce4c3d1bf2R106. Worst case would be that the $item variable does not include the necessary columns for that call yet, but that could be extended (I guess in the model).
Please @rdeutz Check and if you agree with me let me know so I will not close this PR here as you required recently. Otherwise If I get negative feedback here from you and the others I will close it.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
@alikon @Quy Can you set your test result to success again? The last commit which has reset the test results only has removed the unit tests, anything else has not changed.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
weird this pr is RTC but without 2 successfull tests
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.
Ok, I merge this one, but I will add a depreciated for 4.0. I think we should try to get rid of static helpers and with 4.0 we can do this here different.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-07-05 16:06:55 |
| Closed_By | ⇒ | rdeutz |
@rdeutz OK. What I could find out up to now is that for both PRs #16474 or #16494 we need this one here.
But of course nobody knows yet if those 2 PRs will be merged.
I will provide PRs to the repos of the 2 PRs so they will use the new functin from this PR here.
Just for curiousity: How will it work in 4.0 with identifying core extensions?
@Bakual @laoneo Any idea where I would have to add unit tests for it?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.