#126: Fixes issues reading/writing typed properties which are uninitialised #370
Conversation
tests/GeneratedHydratorTest/Functional/HydratorFunctionalTest.php
Outdated
Show resolved
Hide resolved
Ocramius
added
bug
dependencies
|
Just CS fixes left 👍 |
| $bodyParts[] = $indent . "\$values['" . $propertyName . "'] = \$object->" . $propertyName . ';'; | ||
|
|
||
| if ($requiresGuard) { | ||
| $bodyParts[] = ' }'; |
There was a problem hiding this comment.
not actually sure what the CS rules wanted me to fix here. Surely it doesn't want to invert the condition to put a continue here and then make the assignment outside of the if?
There was a problem hiding this comment.
Yeah, I think it wants a continue; or such - conditionals inside a foreach are (rightfully) frowned upon by the current CS rules
This is what's left:
Error: Equals sign not aligned correctly; expected 1 space but found 2 spaces
Error: Use early exit to reduce code nesting.
There was a problem hiding this comment.
That would be objectively worse code; I've extracted it into a method instead
| $bodyParts[] = $indent . "\$values['" . $propertyName . "'] = \$object->" . $propertyName . ';'; | ||
|
|
||
| if ($requiresGuard) { | ||
| $bodyParts[] = ' }'; |
There was a problem hiding this comment.
Yeah, I think it wants a continue; or such - conditionals inside a foreach are (rightfully) frowned upon by the current CS rules
This is what's left:
Error: Equals sign not aligned correctly; expected 1 space but found 2 spaces
Error: Use early exit to reduce code nesting.
|
@Ocramius I've rebased this from the latest branch; hopefully that'll resolve the outstanding test coverage issue it was having can you OK the pipeline run? |
Sounds like the newly introduced code has mutations to be handled via new tests 🤔 Also, a |
|
Not entirely sure how to resolve that tbh; the new code does have test coverage. So it's a bit confusing that it's failing on that metric. |
|
Looking at test failures, I think it's not verifying the generated code output. |
|
It isn't verifying the generated code output; it's verifying the functioning of the generated code. Most if not all the escaped mutants are basically coding standards failures. eg it's putting whitespace at the end of a line instead of the start by flipping the order of a concat. Could add a functional test which runs php cs against the generated code to ensure it matches the standards which would catch the mutants; but I'm not convinced of the value of doing that 🤔 |
|
It's either testing the code until there's nothing left to test, or simplifying the code until there's no other way to wiggle it. It's very much true that things like alignment could be removed from the code generator, for example. |
|
@Ocramius I've improved the fix here thanks to a suggestion from my apprentice. Basically instead of detecting if a property has a type and changing the generated code to add an additional isset, it replaces the !== null check with isset in all cases. This is faster for typed properties and incurs a slight overhead on non-typed properties where the !== null is slightly faster. I think this is a reasonable trade off as I'd expect most people to be using typed properties on their data objects that they'll use this on. To solve the infection PHP issues I replaced the string concatenation in the new/changed code with sprintf infection can't mutate the sprintf version of the code without breaking the tests. Unlike the concat version where it could move whitespace around without a noticeable effect. |
…ialised and not passed in the data array
|
Will this change be merged soon ? I'm once again bitten by the unitialized property issue. |
|
Was on my plate / in background for far too long: thanks @carnage! |
#126 documents an issue with the generated hydrator failing with an error when trying to hydrate an object containing typed properties when a typed property is not present in the data array passed to the hydrator, contains no default value and cannot be null.
The fix is to check that the property isset before attempting to do the null check (hydration) or read from the property (extraction). Extraction behaviour for an unset property is not to include the property in the extracted payload; this is to ensure that the behaviour of chained $hydrator->extract() and $hydrator->hydrate() calls continues to work as expected since keeping the property in the array but leaving it null would cause the hydrate call to attempt to set the property to null, this is undesirable as the property type definition doesn't include null and would cause an error.
Have targetted against 4.1.x as this is a bug introduced by PHP 7.4 and this is the most recent version which supports PHP 7.4
This code could be considered a BC break as an operation which previously resulted in an error will now operate correctly, however there are very few scenarios that I can think of where this would cause someone an issue, since using hydration as a means of data validation would be a pretty suspect use case.
Fixes #126