bug PR-4.4-dev Pending

User tests: Successful: Unsuccessful:

avatar sba
sba
5 Jan 2024

Summary of Changes

cli command config:set should save integer values in configuration.php as integer, not string
this change affects all integer values independent of the transmitted cli param. it is the same behavior as with boolean

Testing Instructions

Run /cli/joomla.php config:set force_ssl=2 to update configuration.php

Actual result BEFORE applying this Pull Request

public $force_ssl = '2'; in configuration.php

Expected result AFTER applying this Pull Request

public $force_ssl = 2; in configuration.php

Link to documentations

Please select:

  • No documentation changes for docs.joomla.org needed
  • No documentation changes for manual.joomla.org needed
avatar sba sba - open - 5 Jan 2024
avatar sba sba - change - 5 Jan 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 5 Jan 2024
Category Libraries
avatar sba sba - change - 6 Jan 2024
Labels Added: bug PR-4.4-dev
avatar richard67
richard67 - comment - 6 Jan 2024

@sba Thanks for this pull request. I had suggested to fix the issue that the CLI saves the integers as strings, and your PR fixes that.

But now as I see the existing code for the booleans and yours for the integers I think that it is not the right thing to do.

E.g. someone could use a database server with host name "12345678". Your code would change that to an integer.

That's not your mistake. The existing code for the booleans was also not really the right thing. Imagine you use a host name "true" or "false" in a local network. Silly example, but a valid host name.

Both examples are very unlikely, as normally a host which is not localhost and not in the local LAN will have also a domain part in the host name, so the examples from above will not happen.

But I think you get what I mean: It can go wrong when trying to assume a data type from the data content.

The right way to determine the type of the particular configuration values to be written would be to check the type of these values in the existing configuration. But that would be a bigger change.

So after having thought about that, I'm not sure if we should use this PR or not. I will discuss that with other maintainers and report back the result.

avatar richard67 richard67 - change - 6 Jan 2024
The description was changed
avatar richard67 richard67 - edited - 6 Jan 2024
avatar richard67 richard67 - change - 6 Jan 2024
The description was changed
avatar richard67 richard67 - edited - 6 Jan 2024
avatar sba
sba - comment - 6 Jan 2024

@richard67 You are absolutly right, I was thinking about that too...

The behaviour with true or false is even worse than with the integer conversion. The integer is probably always correctly converted into a string if a string is expected.

I had a look about the correct way yesterday, I believe the types are saved in ´administrator/components/com_config/forms/application.xml´. But as you said, to convert it against this definition it's a bigger change.

I came across this error and the bugfix because I had migrated over 100 Joomla websites from version 3 to 4 and now the setting force_ssl=2 no longer works for all of them. Because the updater keeps all integers saved as strings in configuration.php.

With the cli-command it would have been easy to mass-update the configuration, but it didn't work either...

Btw:
´/cli/joomla.php config:set´ has othter bugs, if a key/value pair does not exist (e.g. after the Joomla update) it throws warnings. Reworking the whole thing would be the best...

avatar sba
sba - comment - 6 Jan 2024

Perhaps a good interim solution would be for the cli-command to only update the transmitted value.

Then at least all "true" or "false" strings would no longer be converted to Boolean when saving.

avatar sba sba - change - 8 Jan 2024
The description was changed
avatar sba sba - edited - 8 Jan 2024
avatar richard67 richard67 - change - 10 Jan 2024
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2024-01-10 19:35:43
Closed_By richard67
avatar richard67
richard67 - comment - 10 Jan 2024

We have discussed this PR in the CMS Maintenance team and decided to reject it for the reasons stated in comments above.

@sba Sorry that I have motivated you to make this PR. But as we saw in the discussion above, it is not really the right fix, even if existing code for true and false does it already in that way. Of course if you can make a pull request to fix the real issue, we would be happy, or if you have ideas for that just let us know. Thanks for this PR because it has helped to identify the real issue.

avatar richard67 richard67 - close - 10 Jan 2024
avatar richard67
richard67 - comment - 10 Jan 2024

P.S.: I've created an issue so this is not forgotten, see #42640 .

Add a Comment

Login with GitHub to post a comment