Move core extensions array from script.php to a new helper class to provide a safe way to check if core extension #16724

richard67 · 2017-06-16T11:38:54Z

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.

Move 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, and add 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.

richard67 · 2017-06-16T11:47:03Z

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

richard67 · 2017-06-16T12:08:32Z

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

Bakual · 2017-06-16T12:16:55Z

Any idea where I would have to add unit tests for it?

I'm the wrong guy to ask regarding unit tests.

richard67 · 2017-06-16T12:19:58Z

I'm the wrong guy to ask regarding unit tests.

And who would be the right guy?

mbabker · 2017-06-16T12:26:03Z

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.

richard67 · 2017-06-16T12:30:15Z

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

mbabker · 2017-06-16T12:34:39Z

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)

richard67 · 2017-06-16T12:36:30Z

I will add the unit tests as recommended by @mbabker to this PR soon. Stay tuned.

mbabker · 2017-06-16T12:37:34Z

libraries/cms/extension/helper.php

+	 *
+	 * @since   __DEPLOY_VERSION__
+	 */
+	public static function checkIfCoreExtension($type, $element, $folder, $client_id)


$folder and $client_id can be optional parameters.

public static function checkIfCoreExtension($type, $element, $folder = '', $client_id = 0)

Although I would also possibly suggest changing the parameter order so that it's $type, $element, $client_id, $folder since when using this function you're more likely to need to specify a client ID versus a folder (which only applies to plugins).

Makes sense

mbabker · 2017-06-16T12:38:14Z

libraries/cms/extension/helper.php

+	 */
+	public static function checkIfCoreExtension($type, $element, $folder, $client_id)
+	{
+		if (in_array(array($type, $element, $folder, $client_id), self::$coreExtensions))


This can be simplified to return in_array(array($type, $element, $folder, $client_id), self::$coreExtensions); since in_array() is going to give the right boolean value.

Sure, just as you say it I see how silly I was ;-)

Change order of parameters and add default values for function checkIfCoreExtension().

richard67 · 2017-06-16T12:48:32Z

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

Quy · 2017-06-16T14:20:02Z

I have tested this item ✅ successfully on 525d6e0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.}

richard67 · 2017-06-16T14:20:57Z

@Quy Thanks for testing. Do you have suggestions for unit tests?

Quy · 2017-06-16T14:23:54Z

@richard67 Sorry, I am the wrong guy to ask too 😞

rdeutz · 2017-06-16T14:35:17Z

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.

mbabker · 2017-06-16T14:41:14Z

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.

rdeutz · 2017-06-16T14:46:55Z

The difference is: as static class you don't have the option.

richard67 · 2017-06-16T15:34:40Z

@Quy No need to test again, I only added new unit tests so your previous test is still valid. It just needs a review of the unit tests in addition to be ok again.

richard67 · 2017-06-16T15:37:51Z

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

Correct subpackage in doc block of the unit tests.

alikon · 2017-06-16T16:39:20Z

I have tested this item ✅ successfully on 871f0d8

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.}

laoneo · 2017-06-16T17:21:55Z

Guess like that we have a good helper class also for the future.

ghost · 2017-06-16T18:56:03Z

@Quy can you please retest?

Bakual · 2017-06-19T13:08:39Z

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

mbabker · 2017-06-19T13:09:06Z

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.

richard67 · 2017-06-19T16:35:38Z

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

richard67 · 2017-06-19T16:49:40Z

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

@rdeutz

Remove unit test as recommended by @rdeutz

richard67 · 2017-06-19T17:00:06Z

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

rdeutz · 2017-06-19T17:56:27Z

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. :-)

richard67 · 2017-06-19T18:01:00Z

@rdeutz Sure, I can close it. But I will not care anymore if one of these other 2 PRs #16474 and #16494 or both will use hardcoded ID numbers 999 or 9999.
@Bakual @mbabker Do you suggest to close it, too?

alikon · 2017-06-19T18:01:21Z

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

richard67 · 2017-06-19T18:04:03Z

I thought that, too, but now I am not sure anymore.

mbabker · 2017-06-19T18:04:14Z

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?

alikon · 2017-06-19T18:06:09Z

future usage 😄

richard67 · 2017-06-19T18:06:51Z

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

richard67 · 2017-06-19T18:08:52Z

@mbabker See here:

https://github.com/joomla/joomla-cms/pull/16474/files#diff-3bd7bed5d45e0d7de65bd3ce4c3d1bf2R106

https://github.com/joomla/joomla-cms/pull/16494/files#diff-35b54662480daa18b046886a634f00d6R1016

mbabker · 2017-06-19T18:14:03Z

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

richard67 · 2017-06-19T18:16:53Z

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.

richard67 · 2017-06-24T13:01:32Z

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

richard67 · 2017-06-25T08:56:25Z

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

alikon · 2017-06-26T05:53:23Z

weird this pr is RTC but without 2 successfull tests

alikon · 2017-06-26T05:53:36Z

I have tested this item ✅ successfully on 14674ab

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.}

Quy · 2017-06-26T13:11:20Z

I have tested this item ✅ successfully on 14674ab

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16724.}

rdeutz · 2017-07-05T16:04:12Z

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.

richard67 · 2017-07-05T17:21:16Z

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

joomla-cms-bot added the PR-staging label Jun 16, 2017

richard67 mentioned this pull request Jun 16, 2017

New class JExtensionHelper providing a safe way to determine if an extension is core or not #16613

Closed

mbabker reviewed Jun 16, 2017

View reviewed changes

Change order of parameters, add default values

070adc2

Change order of parameters and add default values for function checkIfCoreExtension().

Simplify checkIfCoreExtension

525d6e0

Add unit tests

8ee265f

joomla-cms-bot added the Unit/System Tests label Jun 16, 2017

Correct subpackage in doc block

871f0d8

Correct subpackage in doc block of the unit tests.

Remove unit test

14674ab

Remove unit test as recommended by @rdeutz

joomla-cms-bot removed the Unit/System Tests label Jun 19, 2017

rdeutz added this to the Joomla 3.7.4 milestone Jun 21, 2017

richard67 mentioned this pull request Jul 1, 2017

Extensions: Manage - Warning for not using Update #16474

Closed

rdeutz merged commit c9b9419 into joomla:staging Jul 5, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 5, 2017

richard67 deleted the get-core-extensions-2 branch July 5, 2017 17:17

zero-24 mentioned this pull request Jul 5, 2017

[Update] Changes in the scrip.php file && JExtensionHelper #16985

Closed

Move core extensions array from script.php to a new helper class to provide a safe way to check if core extension #16724

Move core extensions array from script.php to a new helper class to provide a safe way to check if core extension #16724

Conversation

richard67 commented Jun 16, 2017 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

richard67 commented Jun 16, 2017

richard67 commented Jun 16, 2017

Bakual commented Jun 16, 2017

richard67 commented Jun 16, 2017

mbabker commented Jun 16, 2017

richard67 commented Jun 16, 2017

mbabker commented Jun 16, 2017

richard67 commented Jun 16, 2017

mbabker Jun 16, 2017

Choose a reason for hiding this comment

richard67 Jun 16, 2017

Choose a reason for hiding this comment

richard67 Jun 16, 2017

Choose a reason for hiding this comment

mbabker Jun 16, 2017

Choose a reason for hiding this comment

richard67 Jun 16, 2017

Choose a reason for hiding this comment

richard67 Jun 16, 2017

Choose a reason for hiding this comment

richard67 commented Jun 16, 2017

Quy commented Jun 16, 2017

richard67 commented Jun 16, 2017

Quy commented Jun 16, 2017

rdeutz commented Jun 16, 2017

mbabker commented Jun 16, 2017

rdeutz commented Jun 16, 2017

richard67 commented Jun 16, 2017

richard67 commented Jun 16, 2017

alikon commented Jun 16, 2017

laoneo commented Jun 16, 2017

ghost commented Jun 16, 2017

Bakual commented Jun 19, 2017

mbabker commented Jun 19, 2017

richard67 commented Jun 19, 2017

richard67 commented Jun 19, 2017

richard67 commented Jun 19, 2017

rdeutz commented Jun 19, 2017

richard67 commented Jun 19, 2017

alikon commented Jun 19, 2017

richard67 commented Jun 19, 2017

mbabker commented Jun 19, 2017

alikon commented Jun 19, 2017

richard67 commented Jun 19, 2017

richard67 commented Jun 19, 2017

mbabker commented Jun 19, 2017

richard67 commented Jun 19, 2017 • edited

richard67 commented Jun 24, 2017 • edited

richard67 commented Jun 25, 2017

alikon commented Jun 26, 2017

alikon commented Jun 26, 2017

Quy commented Jun 26, 2017

rdeutz commented Jul 5, 2017

richard67 commented Jul 5, 2017

richard67 commented Jun 16, 2017 •

edited by joomla-cms-bot

richard67 commented Jun 19, 2017 •

edited

richard67 commented Jun 24, 2017 •

edited