Use a sensible default if none is found

[roland-d]

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

1. Go to System -> Global Configuration
2. Click on the Server tab
3. Set Error reporting to Maximum
4. Click on Save & Close
5. Go to System -> Install -> Extensions
6. Install the attached module mod_test.zip
7. See that you get the error as shown above but the module is installed
8. Uninstall the module
9. See that you get the error as shown above but the module is uninstalled
10. Apply the patch
11. Install the module again
12. Notice that there is no error now
13. Uninstall the module
14. 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

[ChristineWk]

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

[roland-d]

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

[ChristineWk]

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

[PhilETaylor]

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?

[roland-d]

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.

[roland-d]

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

[infograf768]

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.

[roland-d]

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

[infograf768]

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+

[roland-d]

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

[brianteeman]

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

[roland-d]

I wonder if not allowing a module to install when the client is missing is a B/C break at this point.

[chmst]

It would be friendly allowing the installation and write an information "Module is installed for frontend. "

[roland-d]

@chmst That already happens based on the method tag in a module.

[HLeithner]

This pull request has automatically rebased to 4.2-dev.

[joomla-bot]

This pull requests has been automatically converted to the PSR-12 coding standard.

[HLeithner]

This pull request has been automatically rebased to 4.3-dev.

[obuisard]

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.

[RickR2H]

I have tested this item :white_check_mark: successfully on e45e80518fb849f6b00275e5c57beb52ac82c1f6

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

[pjasmits]

I have tested this item ✅ successfully on https://github.com/joomla/joomla-cms/commit/e45e80518fb849f6b00275e5c57beb52ac82c1f6

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.

[HLeithner]

This pull request has been automatically rebased to 4.4-dev.

[heelc29]

I have tested this item :white_check_mark: successfully on e45e80518fb849f6b00275e5c57beb52ac82c1f6

---

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

[reDimDev]

I have tested this item :white_check_mark: successfully on 8f9f2ceb19dae34950a629cf34fb04bb66829e16

---

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

[tomsrocket]

I have tested this item :white_check_mark: successfully on 8f9f2ceb19dae34950a629cf34fb04bb66829e16

---

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

[richard67]

RTC

---

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

[stimpsonjcat]

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

[stimpsonjcat]

I have tested this item :white_check_mark: successfully on 0e5c2763225496bcd0d489fcabc5e317fdc6127d

---

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

[MacJoom]

Thank you!