User tests: Successful: Unsuccessful:
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
Run /cli/joomla.php config:set force_ssl=2
to update configuration.php
public $force_ssl = '2';
in configuration.php
public $force_ssl = 2;
in configuration.php
Please select:
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
bug
PR-4.4-dev
|
@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...
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-01-10 19:35:43 |
Closed_By | ⇒ | richard67 |
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.
@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.