Improved check on property name#726
Conversation
Specfically avoid the property name "0" crashes the parser with a 'LogicException: This code should not be reachable'
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #726 +/- ##
=========================================
Coverage 98.76% 98.76%
- Complexity 1875 1876 +1
=========================================
Files 71 71
Lines 5345 5345
=========================================
Hits 5279 5279
Misses 66 66 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| continue; | ||
| } | ||
| if (isset($match['name']) && $match['name']) { | ||
| if (isset($match['name']) && 0 < strlen($match['name'])) { |
There was a problem hiding this comment.
strlen() cannot be smaller then 0
There was a problem hiding this comment.
Hi @staabm that part of the code is meant to check that the length of the property name is actually greater than zero. I think it is correct as is, please let me know if you still disagree.
(the codestyle requirements from php-cs-fixer requires the constant to be written first, that might look a bit confusing at first glance)
There was a problem hiding this comment.
This is checking that whatever is in 'name' is not (equivalent to) the empty string.
That is better than the previous test that checked if the value was "true" (the boolean true, or some non-zero number, or some string that does not "look like" zero.
Note: if somehow the 'name' is set to boolean false then its strlen(FALSE) will be the integer 0. That will throw the LogicException below, which is "a good thing".
So IMO this change is "a good thing".
|
Backported to the The code format changes were not valid for ancient PHP 7.1 and 7.2, so I didn't end up backporting those. (the 2nd commit in this PR) |
4 participants
Fixes that property name "0" crashes parser with 'LogicException: This code should not be reachable'