[#36692] - Use a sensible default if none is found
- Fixed in Code Base
- 28 Feb 2024
- Medium
- Build: 4.0-dev
- # 36692
- Diff
- roland-d:feature/uninstall-module-without-site-breaks
User tests: Successful: Unsuccessful:
Summary of Changes
When you have a module installed but the module manifest does not have the client attribute set, uninstalling this module crashes:
The same thing happens when you install the module but when you refresh the page it tells you the module installation was fine.
So the change here is to set a sensible default of site as that is where most modules are installed.
This was tested on PHP 8.1
Testing Instructions
- Go to System -> Global Configuration
- Click on the Server tab
- Set Error reporting to Maximum
- Click on Save & Close
- Go to System -> Install -> Extensions
- Install the attached module
mod_test.zip - See that you get the error as shown above but the module is installed
- Uninstall the module
- See that you get the error as shown above but the module is uninstalled
- Apply the patch
- Install the module again
- Notice that there is no error now
- Uninstall the module
- Notice that there is no error now
Actual result BEFORE applying this Pull Request
The module does install but the user faces a fatal error
Expected result AFTER applying this Pull Request
The module is installed but no fatal error is shown
Documentation Changes Required
None
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
@ChristineWk Thank you for testing. I was actually doing my tests on PHP 8.1 but even though the issue should be reproducible on 7.4. For it to show on PHP 7.4 I have changed the instructions a bit, can you see if you can get an error now?
Tested again. No error ... Checks:
Module for testing purposes / 1.0.0 / Jan15 / Developer
PHP 7.4.25 / DB 5.7 / PHP Built on Linux / cgi-fci
So you are assuming site rather than administrator or cli here... would it not be better to just not run the doLoadLanguage if no client_id was found?
So you are assuming site rather than administrator or cli here...
Yes, because modules cannot be loaded in the cli and as I stated in my opening post is that most modules are installed for the front-end of the site instead of the administrator.
I do not think this is so different from the default set in the TemplateAdapter.php except that there the default is set to administrator.
@ChristineWk I am going to test it again to see if I cannot get the error but I really do not understand why you see no error.
IMHO, such a module (or any extension) without a client in the manifest should just be prevented from being installed and should throw an error.
@infograf768 That actually crossed my mind as well but the module is already installed at this point, so I figured why not :) I am fine to change this PR to also check if the client is set. Regardless I think a default is not a bad fallback.
In fact, I tried to installed that module and result depends on php version.
Php 8.0.8 I get an internal server error.
php 7.4.21: the module installs with a default clientId of 0 (site) and can be uninstalled without any error.
Therefore, what we need is preventing installing and meaningfull error I guess to adapt to php 8+
@infograf768 The Internal Server Error is actually the 0 Undefined constant "JPATH_" You can see that in the Network tab and click on the repsonse tab. That will show you the HTML page with the error in it.
So I checked again on PHP 7.4 and now the installer runs fine as @ChristineWk and you found out. This is because an undefined constant is only fatal since PHP 8.
The default client ID of 0 is not a change from this PR, that is already in Joomla. The adaptation to PHP 8+ is to also set a default client to site so you get an existing constant.
If possible why not change it so that modules cannot be installed without a defined client AND use a default when uninstalling a module that doesnt have a defined client
I wonder if not allowing a module to install when the client is missing is a B/C break at this point.
It would be friendly allowing the installation and write an information "Module is installed for frontend. "
| Labels |
Added:
?
|
||
This pull request has automatically rebased to 4.2-dev.
This pull requests has been automatically converted to the PSR-12 coding standard.
| Labels |
Added:
PBF
?
?
Removed: ? |
||
This pull request has been automatically rebased to 4.3-dev.
One issue is that we allow extensions with the client tag to be 0 while it should be site, for instance. We should stop allowing extensions to be installed with an improper value for 'client'. It is now well documented (change for 5.0 ?).
AND use the fix from Roland @roland-d for b/c with extensions that have already be installed.
So, I am in favor of this PR, still necessary for b/c even if we start enforcing the client values.
I have tested this item ✅ successfully on e45e805
I'm on PHP 8.1 installing the test module is not working. I get the same error as @infograf768. Adding the patch will allow the module to install. I count this as a successful test.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
I have tested this item ✅ successfully on e45e805
I'm on PHP 8.1 installing the test module is not working. I get the same error as @infograf768. Adding the patch will allow the module to install. I count this as a successful test.
This pull request has been automatically rebased to 4.4-dev.
I have tested this item ✅ successfully on e45e805
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
| Labels |
Added:
bug
?
PR-4.4-dev
Removed: ? ? |
||
I have tested this item ✅ successfully on 8f9f2ce
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
I have tested this item ✅ successfully on 8f9f2ce
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
| Labels |
Added:
Small
Removed: ? |
||
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
Successfully tested this in Joomla 5.1 alpha 3 with php 8.3.3. When installing I get a warning: Unable to detect manifest file. Nothing is installed.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
I have tested this item ✅ successfully on 0e5c276
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.
| Labels |
Added:
RTC
|
||
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-02-28 17:58:25 |
| Closed_By | ⇒ | MacJoom |
Thank you!
@roland-d
Can't confirm point 3 and point 5. (No error)
Maybe it belongs to PHP version? I'm using PHP 7.4 / Joomla Version 4.0.6-dev
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36692.