[#16613] - New class JExtensionHelper providing a safe way to determine if an extension is core or not
- Closed
- 16 Jun 2017
- Medium
- Build: staging
- # 16613
- Diff
- richard67:get-core-extensions
- 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
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #16579 .
Summary of Changes
New class JExtensionHelper for helping with core extensions detection.
Thanks to @andrepereiradasilva who did the first implementation. I extended that a bit.
As soon as this is finished and merged, I will make another PR if necessary to replaces existing tests in SQL statements for extension_id > 999 or similar by using 'NOT (' . JExtensionHelper::getWhereCondition() . ')' as a query condition or using the JExtensionHelper::getCoreExtensionsIds() or JExtensionHelper::getCoreExtensionsIdsList() functions if that would be more appropriate.
And I will make similar PRs to the repositories of the authors of PRs #16474 and #16494 if those PRs are still open then, otherwise they will have been caught with the previous step.
Testing Instructions
- Code reviews by experienced manitainers are welcome
- 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::getWhereConditionCore():"; ?><p>
<p><?php $test1=JExtensionHelper::getWhereConditionCore(); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::getWhereConditionNonCore():"; ?><p>
<p><?php $test1=JExtensionHelper::getWhereConditionNonCore(); var_dump($test1); ?><p>
<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::getCoreExtensionsIds():"; ?><p>
<p><?php $test1=JExtensionHelper::getCoreExtensionsIds(); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::getNonCoreExtensionsIds():"; ?><p>
<p><?php $test1=JExtensionHelper::getNonCoreExtensionsIds(); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::getCoreExtensionsIdsList():"; ?><p>
<p><?php $test1=JExtensionHelper::getCoreExtensionsIdsList(); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::getNonCoreExtensionsIdsList():"; ?><p>
<p><?php $test1=JExtensionHelper::getNonCoreExtensionsIdsList(); 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>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::checkIfCoreExtensionId(1):"; ?><p>
<p><?php $test1=JExtensionHelper::checkIfCoreExtensionId(1); var_dump($test1); ?><p>
<p style="font-weight: bold;"><?php echo "JExtensionHelper::checkIfCoreExtensionId(10000):"; ?><p>
<p><?php $test1=JExtensionHelper::checkIfCoreExtensionId(10000); var_dump($test1); ?><p>
Expected result
The result should look similar to follows (long stuff replaced by "..."):
JExtensionHelper::getWhereConditionCore():
string(13256) "`type` = 'component' AND `element` = 'com_admin' AND `client_id` = 1 OR `type` = 'component' AND `element` = 'com_ajax' AND `client_id` = 1 OR ... OR 1 = 2"
JExtensionHelper::getWhereConditionNonCore():
string(13900) "(`type` <> 'component' OR `element` <> 'com_admin' OR `client_id` <> 1) AND (`type` <> 'component' OR `element` <> 'com_ajax' OR `client_id` <> 1) AND ... AND 1 = 1"
JExtensionHelper::getCoreExtensions():
array(161) { [0]=> array(4) { [0]=> string(9) "component" [1]=> string(10) "com_mailto" [2]=> string(0) "" [3]=> int(0) } [1]=> array(4) { [0]=> string(9) "component" [1]=> string(11) "com_wrapper" [2]=> string(0) "" [3]=> int(0) } ... }
JExtensionHelper::getCoreExtensionsIds():
array(161) { [0]=> string(1) "1" [1]=> string(1) "2" ... }
JExtensionHelper::getNonCoreExtensionsIds():
array(15) { [0]=> string(5) "10000" [1]=> string(5) "10001" ... }
JExtensionHelper::getCoreExtensionsIdsList():
string(608) "1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,22,23,24,25,27,28,29,30,31,32,33,34,102,103,104, ... ,700,802,10041,10042,10043"
JExtensionHelper::getNonCoreExtensionsIdsList():
string(89) "10000,10001,10002,10004,10005,10006,10044,10045,10055,10060,10062,10079,10086,10109,10110"
JExtensionHelper::checkIfCoreExtension('component', 'com_mailto', '', 0):
bool(true)
JExtensionHelper::checkIfCoreExtension('component', 'com_blabla', '', 0):
bool(false)
JExtensionHelper::checkIfCoreExtensionId(1):
bool(true)
JExtensionHelper::checkIfCoreExtensionId(10000):
bool(false)
Verify that the comma-separated list of extension_ids which has been produced by JExtensionHelper::getCoreExtensionsIdsList() contains only the IDs of core extensions, especially if there are IDs greater than or equal to 10000. Also those should belong to core extensions, the list should not contain any IDs of non-core extensions. For the others (smaller than 10000) it is enough to spot check some of them, checking all it too much.
Verify that the comma-separated list of extension_ids which has been produced by JExtensionHelper::getNonCoreExtensionsIdsList() contains only the IDs of **non-**core extensions.
Check that no PHP errors are shown or logged in your log file (if you use some).
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 |
| Labels |
Added:
?
|
||
@richard67 can you please add doc block comments for the properties too? Basicly Usage, type (using @var ) and since tag
@zero-24 Done. Thanks for the hint, also thanks to @Quy for the review before.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
@richard67 i just don't agree in putting core extensions stuff in the constructor. I mean that class can be used in the future for more method that are not core extensions related. It's called jextensionhelper, not jcoreextensionshelper ;)
@andrepereiradasilva Which constructor? I changed it back to a normal callable function a few commits before.
Oh forget it. Commenting on old version. Ignore.
@Quy Done the ID to id thing, also at a few more places.
@andrepereiradasilva Renamed things which are related to core extensions only so it is clear and more open for future extensions by handling non-core extensions, also extended doc block comments.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
@andrepereiradasilva By the way, I checked against joomla.sql of current staging and the array was complete, nothing was missing or wrong. Thanks for your work for this.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
Should be less than:
Check that if the comma-separated list of extension_ids which has been produced by JExtensionHelper::getCoreExtensionsIdsList() contains values greater than or equal to 10000, then these extensions belong to the core.
@Quy Testing instructions alrerady had been updated.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
@Quy No, i mean those which are in the list and greater or equal than 1000. For those it has to be checked if they are core, otherwise they should not be in the list, and for those smaller than 10000 it is not really necessary to check because they are very likely core, but of course someone can check this, too.
Do you mean this ? then these extensions do not belong to the core.
@Quy No. I updated testing instructions to make it clear.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
Installations which have gone through updates which have addded new core extensions during their history will have core extensions with IDs greater than or equal to 10000.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
I don't see the problem you are trying to solve. But to determine if an extension belongs to core I would add a new property (column) or even introduce a new state for an extension. For me it is hard to understand that we must point devs to that file when they want to add a new extension to the core.
- It is rare that we add or remove an extension
- It is no harder than adding a removed file to script.php
@laoneo As you can see in the PR, we already maintain such a list so the manifest of core can be updated during a CMS update. Just that this list currently isn't available for other use cases. This PR creates an helper method which can be used when needed.
A new column in the database was discussed, but to me doesn't make sense since this is static information which is always the same for a given Joomla version. Imho it doesn't make sense to store such information into the database.
@Quy Sorry but I thought it is useful to add some new functions for non-core extensions. Could you test and review again? That would be fine.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.
i really like the concept but
please not in this way....
queries are very ugly to me
Which else way? The query is the same as it was in script.php before this PR. It is nothing new, only has been moved into the new class.
ok
i was stuck on pr title
New class JExtensionHelper
@alikon Do you think some index could speed up the queries?
Maybe
ALTER TABLE j3ux0_extensions ADD KEY idx_type_element_folder_client_id (type, element, folder, client_id);
or
ALTER TABLE j3ux0_extensions ADD KEY idx_type_element_client_id (type, element, client_id);
or indexes on the individual columns?
i don't think the main problem here is about perfomance, rather, is much more on the data model design side,
in my opinion adding a boolean field 'core_extenions' could be a good choice....
Are really all this functions needed? Why not just providing a function which returns the extensions. Additionally we can think about a flag, probably something like public static function getExtensions($onlyCore = false)? Keep in mind this class should also get some unit tests at the end.
@laoneo What do you mean with "which returns the extensions"? One which returns the static array like getCoreExtensions is in my PR right now? In this case a "$onlyCore = false" would not make sense, since we do not have and never wanna have a static array including the non-core extensions. Or one which reads stuff from DB? Please make more clear what you mean, and maybe paste here a doc bloc of some function you have in mind so we can understand better what you mean.
The more I think about it, the less I think that if data is static or not is not a criteria whether it should e in database or not, like @Bakual commented a bit above. The columns type, element, folder and client_id we use now in script.php and possibly in future with this PR to identify the core extensions are static, too. It makes sense to store stuff in database if it helps the application with having more simple code and/or be much more performant.
A column "is_core" in the extensions table having an index on it whould simplify code much and speed up queries.
To be able to rely on the value in that column, it is important that the schema update settting the values for this columns on update of old installations is run on the update. In case this is not granted, we could still let script.php go through the extensions table like it does now for updating the manifest cache, using the static array with the type, element, folder and client_id values.
If we give the new column a default value of 0 (false) we could even not mention it in docs and examples for non-core extension developers, but maybe we should mention it and tell that if they set this to 1 in their extensions, stuff might not work, so they have to set it to 0.
I think about making a new PR with the database column solution as an alternative to this one here.
To be able to rely on the value in that column, it is important that the schema update settting the values for this columns on update of old installations is run on the update.
Friendly reminder: The reason we discuss this PR in the first place is because we can't rely on the updates being run properly. We can rely on the structure, but not on the data. That's why core extensions could have IDs higher than 10000 even if they shouldn't.
Honestly, this PR evolved for some reason beyond the initial idea. Personally I would just store that array with the needed data. Imho, there is no reason for a method doing the SQL query to fetch the IDs. Can't we keep the database stuff out of this class? The class certainly looks very complex now for such a trivial task.
The cases where that list of core extensions is used are very limited. Do we need the IDs in all those cases?
Well the idea behind the additional functions beside getCoreExtensions was to avoid duplicate code.
But limiting the new helper to the static aray only and keeping all database stuff out has also its charme.
@laoneo Did you mean same as @Bakual , to limit the new helper to the static array thing only?
I think we should at least keep the checkIfCoreExtension function, too, not only the getCoreExtensions. What do you think about this?
Yeah, that one looks nice and helpful.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-06-16 11:41:58 |
| Closed_By | ⇒ | richard67 |
@Quy I have implemented your review comments. As soon as AppVeyor has finished with success, all is ready I hope.
Questions:
If so, it would have to be extended by the information that the array in the new helper class has to be updated, too, if adding a new extension.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16613.