Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #2371: revert changes in the Get() method #2593

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

bschmalhofer
Copy link
Contributor

Returning the default value is now triggered again by the truthiness of the stored value. Note that the string containing the digit 0 is true.

Returning the default value is now triggered again by the truthiness
of the stored value. Note that the string containing the digit 0 is true.
@bschmalhofer bschmalhofer merged commit 0d832e4 into rel-11_0 Oct 24, 2023
@bschmalhofer bschmalhofer deleted the issue-#2371-rollback_core_config branch October 24, 2023 15:45
@bschmalhofer
Copy link
Contributor Author

That the change has to be reversed is clear. However there are five new test failures in the test suite, including the original errorAdminSystemConfigurationExampleSessionID.t. I merged anyways and accept that there are some intermediary failures.

Here is the analysis part. It was already stated above that the problem can be highlighted by dumping a numeric Sysconfig setting one time with JSON and the other time Cpanel::JSON::XS.

otobo@ad98e2ba4b0f:~$ cp JSON_old_but_working.pm Kernel/System/JSON.pm 
otobo@ad98eis2ba4b0f:~$ bin/otobo.Console.pl Maint::Config::Dump SessionUseCookie
"0"
otobo@ad98e2ba4b0f:~$ cp JSON_new_but_broken.pm Kernel/System/JSON.pm 
otobo@ad98e2ba4b0f:~$ bin/otobo.Console.pl Maint::Config::Dump SessionUseCookie
0
otobo@ad98e2ba4b0f:~$ 

Further debugging showed that the versions of the JSON libraries do not matter.

The explanation why SessionUseCookie is considered one time as a string and the other time as number is not obvious. But the relevant hint is right in the documentation of Cpanel::JSON::XS: https://metacpan.org/pod/Cpanel::JSON::XS#simple-scalars .

Non-deterministic behavior is following: scalars that have last been used in a string context before encoding as JSON strings, and anything else as number value:

For SessionUseCookie this checks out. The number is created by running ZZZAAuto,pm and never meddled with, e.g. printed, before converted to JSON.

JSON::XS follows more or less the same strategy but apparently there are differences in details.

The take home message is that the decision why a (numeric) value is represented as JSON as a string might be deterministic, but not transparent. So the next best idea is to force transparency. This can be done by setting $json->type_all_string(1) for the values that are passed into the JavaScript World. This is a simple rule, that forces stringiness on the numbers. It seems to be in line with the current implementation:

otobo$ grep -r Core.Config.Get  var/httpd/htdocs/js/ | grep parseInt | wc -l
27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant