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

BUGFIX: Ensure that only numerical property values are converted to integer #3535

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

grebaldi
Copy link
Contributor

fixes: #3158, #3157

The problem

The UI's NodePropertyConversionService over-generalizes when converting incoming values to integer. It just casts any incoming string to int via (int) $someValue.

Now, PHP throws no error if the value we're casting to int actually cannot be meaningfully converted. So, pretty much anything it cannot make sense of ends up being 0:

php > echo (int) "foo";
0

The solution

I adjusted the method convertInteger of NodePropertyConversionService to only convert strings to integers that pass PHP's is_numerical check.

In case the string doesn't pass, convertInteger now defaults to null.

Because of this, I needed to adjust the TextFieldEditor component, so that it now handles the occurrence of null-values correctly.

As a result, if I enter a non-numerical string into a text field for a property of type integer, the property will receive null as a value. The same thing happens, if I enter an empty string.

@grebaldi grebaldi linked an issue Jun 22, 2023 that may be closed by this pull request
@github-actions github-actions bot added Bug Label to mark the change as bugfix 7.3 labels Jun 22, 2023
@grebaldi grebaldi linked an issue Jun 22, 2023 that may be closed by this pull request
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thanks you ;)
Looks good for know. In the future (Neos 9) id like to have strict validation via ajax directly in the editor. (And we could make our NodePropertyConversion thing more strict by throwing exceptions ...)

@mhsdesign
Copy link
Member

Tested it, and after fixing also the TextAreaEditor to handle null as value it works as expected:

before

Bildschirmaufnahme.2023-07-05.um.08.15.44.mov

after

Bildschirmaufnahme.2023-07-05.um.08.26.15.mov

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

I think the more annoying issue (that ought to be fixed for LTS imho) is #3157 (unsetting an optional integer value is impossible).

Very good point, take my vote :)

@mhsdesign mhsdesign merged commit 1d39901 into neos:7.3 Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.3 Bug Label to mark the change as bugfix next-patch
Projects
None yet
3 participants