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

isNumber() bugfix #262

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

develart-projects
Copy link
Collaborator

Using isNumber() in current implementation is causing float to be used as array key and that's prohibited by newer PHP versions.
Here is the bugfix for the problem. Already tested localy on production servers.

@develart-projects
Copy link
Collaborator Author

..anyone here? :)

@@ -524,7 +528,10 @@ public static function isNumber($input, array $options = [])

$regexs = Zend_Locale_Format::_getRegexForType('decimalnumber', $options);
$regexs = array_merge($regexs, Zend_Locale_Format::_getRegexForType('scientificnumber', $options));
if (!empty($input) && ($input[0] == $symbols['decimal'])) {

$firstChar = substr($input,0,1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this better than $input[0]?

@glensc
Copy link
Collaborator

glensc commented Oct 3, 2022

provide simple reproducible test case. i.e php snippet. I believe this is locale aware problem, so include your locale setup details.

@develart-projects
Copy link
Collaborator Author

develart-projects commented Oct 3, 2022

provide simple reproducible test case. i.e php snippet. I believe this is locale aware problem, so include your locale setup details.

Any localization using ',' instead of '.' as decimal separator triggers the warning, when used as filter in form field definition. (As far as I remember, because I applied this fix like year+ ago. Now just patching the upstream as well. )

@glensc
Copy link
Collaborator

glensc commented Oct 4, 2022

i've found a fix for localization aware float to use sprintf %F formatter

@glensc
Copy link
Collaborator

glensc commented Oct 4, 2022

you say "triggers the warning", but you have not provided what the warning is.

and still, if you want this to be merged, provide a code snippet to reproduce it. other maintainers may be more forgiving and merge without any tests, but not me.

@develart-projects
Copy link
Collaborator Author

you say "triggers the warning", but you have not provided what the warning is.

and still, if you want this to be merged, provide a code snippet to reproduce it. other maintainers may be more forgiving and merge without any tests, but not me.

Msg was something about using float as array key is not allowed. I don't remember where exactly I found the problem, so can't really reproduce immediately. Need to run into that again.

Trying to get UTs working right now to verify the change. Still some work to be done there, commented in another thread (UT9 migration).

@develart-projects
Copy link
Collaborator Author

I managed to find place of my code where "old" code is triggering the warning:
Warning: Trying to access array offset on value of type float in /usr/share/NetBeansProjects/library/development/Zend/Locale/Format.php on line 527

/usr/share/NetBeansProjects/library/development/Zend/Locale/Format.php:527:float 400

Now I'm trying to reproduce this problem using some real-life sample code to move this PR forward.

@develart-projects
Copy link
Collaborator Author

develart-projects commented Jul 24, 2023

OK, here is how to reproduce the problem / how to trigger the warning:

$input = 0.005; // valid numeric entry
$result = Zend_Locale_Format::isNumber($input);

As you can see, array access to the float does not really work. My patch is fixing the problem.
Now, please re-check the PR and merge, or let me know if you like to get that fixed another way. I would like to move forward here and mainline my current modified copy.

Thanks in advance.

@develart-projects
Copy link
Collaborator Author

I have slightly updated the PR to make UTs happy. Now FloatTest is passing as well, so pls consider to merge, thanks.

@develart-projects
Copy link
Collaborator Author

Anyone here or already dead?

@develart-projects develart-projects merged commit 4afdf79 into Shardj:master Aug 9, 2023
4 checks passed
@develart-projects develart-projects deleted the isNumber-bugfix branch August 9, 2023 08:02
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.

2 participants