[#25910] - [4.0] Fix PHP error on Joomla Update and Manage Extension pages
- Closed
- 17 Aug 2019
- Medium
- Build: 4.0-dev
- # 25910
- Diff
- richard67:4.0-dev-fix-core-modules-list
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #25888 .
Summary of Changes
This PR adds the testing sample data plugin to the list of core extensions in the ExtensionHelper class.
This is necessary to avoid PHP notice as described in issue #25888 when going to the Joomla Update Component view.
The reason is that update site checks are made all update sites, i.e. also for 3rd party extensions, and also when going to the Joomla Update Component, and for a core extension this fails because it has no own update site.
The testing sample data plugin is not shipped with Joomla CMS packages, it is only present on a cloned GitHub repository. So it is necessary to double-check that the change in this PR does not do harm when that plugin is not present. See testing instructions below for checking this with a nightly build.
Furthermore, this PR adds code to update the manifest cache of the testing sample data plugin after it has been installed.
This fixes the PHP notices you get when going to "Manage - Extensions", see comment here: #25910 (comment). Thanks @brianteeman for discovering these.
Finally, this PR remove a duplicate entry for the "sessiongc" system plugin. This change does not have any effect except code is clean, because duplicate entries don't do harm for the same reason as for having extensions in the list which are not present.
Testing Instructions
-
Install 4.0-dev from a cloned GitHub repository where testing sample data is present.
-
After installation has finished with success, login to backend and go to /administrator/index.php?option=com_joomlaupdate while watching the php error log.
Result: Multiple times PHP notice as follows:
PHP Notice: Trying to get property 'version' of non-object in /home/richard/lamp/public_html/joomla-cms/administrator/components/com_joomlaupdate/Model/UpdateModel.php on line 1357
- Now go to "Manage - Extensions" while watching the php error log.
Result: 2 PHP notices as follows:
PHP Notice: Undefined property: stdClass::$version in /home/richard/lamp/public_html/joomla-cms-4.0-dev/administrator/components/com_installer/tmpl/manage/default.php on line 108
PHP Notice: Undefined property: stdClass::$version in /home/richard/lamp/public_html/joomla-cms-4.0-dev/administrator/components/com_installer/tmpl/manage/default.php on line 124
-
Apply this PR e.g with patchtester.
-
Remove configuration.php, delete all database tables and install again.
-
Go again to /administrator/index.php?option=com_joomlaupdate while watching the php error log.
Result: No PHP notice.
- Now go again to "Manage - Extensions" while watching the php error log.
Result: No PHP notice.
- Repeat steps 1 and 2, but this time using a nightly build unpacked into an empty folder. In opposite to a cloned GitHub repository, this does not ship with testing sample data.
Result: No PHP notice.
- Continue with steps 3 to 7, still using the nightly build.
Results: Same as before in steps 3 to 7, i.e. without patch notice, with patch no notice.
Expected result
No PHP notice on the Joomla Update Component view.
No PHP notice on the "Manage - Extensions" view.
Actual result
When using a cloned GitHub repository, multiple times PHP notice as follows when going to the the Joomla Update Component view :
Notice: Trying to get property 'version' of non-object in /home/richard/lamp/public_html/joomla-cms/administrator/components/com_joomlaupdate/Model/UpdateModel.php on line 1357
No PHP error or warning or notice for that view when using a nightly build.
In any case on the "Manage - Extensions" view 2 PHP notices as follows:
PHP Notice: Undefined property: stdClass::$version in /home/richard/lamp/public_html/joomla-cms-4.0-dev/administrator/components/com_installer/tmpl/manage/default.php on line 108
PHP Notice: Undefined property: stdClass::$version in /home/richard/lamp/public_html/joomla-cms-4.0-dev/administrator/components/com_installer/tmpl/manage/default.php on line 124
Additional information
I have investigated where the list of core extensions is used in which way.
Either it is used to build a where clause for a database query where there will not be returned any result when a listed core extension is not really present in database, like is when using a normal Joomla package, or it is used for checking if an existing extension is in the list, which does also not happen when the extension is not present.
So I think it does not do any harm to have the testing sample data plugin in the list of core extensions in case that plugin is not present.
Documentation Changes Required
No.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Title |
|
||||||
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25910.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25910.
IMO, we should not include non-existing extensions here.
@SharkyKZ Then you have only 2 options:
- Live forever with the PHP notice which is solved by this PR, or
- Ship the testing sample data like other sample data with the regular CMS containers, so it can be added to the core extensions list.
I would prefer solution 2.
I agree it is a kind of a hack what I do in my PR here, but solution 2, which in my opinion is the right one, requires a decision on a higher level than I am.
And so I made this PR to fix the notice as long as we don't do that.
Test if you have time, and you will see it does not do any harm.
This should be fixed during installation. Currently, the plugin is "installed" by manually inserting a row in #__extensions table:
At the very least we could run Joomla\CMS\Installer\Installer::refreshManifestCache() like we do for other extensions:
@SharkyKZ Beside that, there are other reasons why the plugin should be in the list of core extensions. Your change will not do that. For example it will not be treated like an external extension when checking if it has an update site (which it hasn't). This has nothing to do with manifest cache.
Sorry but I dont think this is correct and that you have solved a symptom but not the cause. Even after this PR I get
@brianteeman Well this notice I get with and without my PR, and the other one was fixed with this PR. So it seems both is needed, my change and what @SharkyKZ said, that it needs to update the manifest cache.
@SharkyKZ Sorry for bothering you again, just wanted to let you know that I will make your proposed change. Thanks for the hint how to do it. If that solves all the PHP notices, I will undo adding the plugin to the core extensions list, otherwise if your change solves only the notice discovered by Brian, I will leave the change in the ExtensionHelper as it is.
| Labels |
Added:
?
|
||
| Category | Libraries | ⇒ | Installation Libraries |
@brianteeman @SharkyKZ It needs both changes to get rid of all PHP notices mentioned above, 1st my change for adding it to the list of core extensions for the notice when going to Joomla Update Component, and 2nd Sharky's change for updating the manifest to get rid of the PHP notices when going to Extensions - Manage.
Sharky, think it over with the core extensions list: It contains all core extensions, regardless if installed or not. Ok, they can't be uninstalled, but see it so: The testing sample data is still a core plugin, whether installed or not, and all code which uses the list does not rely on it being installed, it uses the list only as a condition for checking if things which are installed are core or not core.
So now I think this PR is good, I only have to update description and testing instructions
Installation change alone works fine for me.
I am confused why all of this is necessary - seems very wrong to me and that you are solving the wrong problem
@brianteeman Could you be more specific?
Did you reinstall after applying patch?
@SharkyKZ I did reinstall after having applied the patch. I just notice I have to update my testing instructions for that. After reinstall I tested that all notices have gone. Then I edited ExtensionHelper.php and commented out the line I have added:
//array('plugin', 'testing', 'sampledata', 0),
Then I went back to the Home Dashboard and clicked the Joomla Update Component button to get to that component. And voilà: The PHP notice mentioned in issue #25888 was back.
| Title |
|
||||||
@SharkyKZ You are right. When I make the new installation with only the change in DatabaseModel.php applied, then the PHP notice on the Joomla Update Component has also gone. So I revert my changes in the ExtensionHelper.php and will make a new PR for removing that duplicate entry for the sessiongc system plugin. That one can be then tested by code review. I will update testing instructions above, they should become much shorter now.
| Category | Libraries Installation | ⇒ | Installation |
| Category | Installation | ⇒ | Installation Libraries |
@richard67 I can't replicate your issue. Can you check if manifest_cache is added to #__extensions table for this plugin after installation with patch? If it is, you should not be getting the notice.
@SharkyKZ For easier and better testing I think I should split this PR into 3:
- One PR to fix duplicate element
array('plugin', 'sessiongc', 'system', 0),in array with core extensions in fileExtensionHelper.php - Another PR for adding element
array('plugin', 'testing', 'sampledata', 0),to the same array and the same file, and - Another PR with the changes in file
DatabaseModel.phpfor adding update of manifest cache after installation of testing sample data plugin.
Number 1 could be tested by code review, and 3 could be tested with and without 2 applied to see all interdependencies, if there are some.
What do you think?
If it makes things clearer, sure.
PR to fix duplicate element array('plugin', 'sessiongc', 'system', 0), in array with core extensions in file ExtensionHelper.php is #25918 , thanks @SharkyKZ for testing. Can be easily tested by code review.
PR with the changes in file DatabaseModel.php for adding update of manifest cache after installation of testing sample data plugin is #25919 . @SharkyKZ thanks in advance for testing
PR for adding the testing sample data plugin to the list of core extensions will come in a while. When that is ready I will close this one here and update reference to PR in issue #25888 .
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-08-17 15:09:43 |
| Closed_By | ⇒ | richard67 |
@C-Lodder Please test.