[2.3] Static Code Analysis for Components #14800
Conversation
- not optimal regular expressions usage
- strlen miss-use
- not optimal conditionals in Process and Filesystem
- unsafe uniquid usage
| @@ -209,8 +209,8 @@ public function getDefaultValue() | |||
| */ | |||
| public function addChild(NodeInterface $node) | |||
| { | |||
| $name = $node->getName(); | |||
| if (!strlen($name)) { | |||
| $name = (string) $node->getName(); | |||
There was a problem hiding this comment.
The cast is not necessary. getName() returns a string.
There was a problem hiding this comment.
Tried to add validation into base node if string provided as name, UTs crashed.
Original code allows 0 and false, so I just put this visible - inspire of PhpDoc it's possible to provide non-string values as name, so perhaps it's a new bug discovered.
There was a problem hiding this comment.
Where do you see that one can provide non-string values as names?
There was a problem hiding this comment.
\Symfony\Component\Config\Definition\BaseNode::__construct doesn't validate a type.
And validation will break some of UTs, I didn't look into details as very constraint in time.
upd: 2 other setters don't check type as well
\Symfony\Component\Config\Definition\ArrayNode::setName
\Symfony\Component\Config\Definition\VariableNode::setName
There was a problem hiding this comment.
@xabbuh : I can just revert this change and create a bug-report, what do you think?
There was a problem hiding this comment.
Well, we don't do any checking for scalar types in Symfony. But the docblock clearly states that you have to pass string values. Thus, yes, please revert that change. But I don't think it is necessary to open an issue for that.
There was a problem hiding this comment.
Ok, shall I also revert then changes in Symfony/Component/Security/Acl/Domain/DoctrineAclCache.php, as I added a scalar type check?
There was a problem hiding this comment.
@xabbuh : done, in nutshell strlen's are forcing string context, covering issues with types, very elegant but not obvious hack.
…ring scalar type issues
| @@ -589,7 +589,7 @@ protected function getAbsoluteUri($uri) | |||
| $uri = $path.$uri; | |||
| } | |||
|
|
|||
| return preg_replace('#^(.*?//[^/]+)\/.*$#', '$1', $currentUri).$uri; | |||
| return preg_replace('#^([^/]*//[^/]+)\/.*$#', '$1', $currentUri).$uri; | |||
There was a problem hiding this comment.
Those are not equivalent. You cannot replace .*? or .+? like you are doing. All those changes should be reverted.
…egular expressions
| @@ -202,7 +202,7 @@ public static function phpize($value) | |||
| return '0x' === $value[0].$value[1] ? hexdec($value) : (float) $value; | |||
| case preg_match('/^0x[0-9a-f]++$/i', $value): | |||
| return hexdec($value); | |||
| case preg_match('/^(-|\+)?[0-9]+(\.[0-9]+)?$/', $value): | |||
| case preg_match('/^(-|\+)?\d+(\.\d+)?$/', $value): | |||
There was a problem hiding this comment.
what is the point/advantage of changing that?
There was a problem hiding this comment.
[0-9] not supporting Unicode digits
// matches Unicode digits, e.g. Arabic
preg_match('/\d+/u', '٠١٢٣٤٥٦٧٨٩', $m);
var_dump($m);
// will not match Unicode digits
preg_match('/\[0-9]+/u', '٠١٢٣٤٥٦٧٨٩', $m);
var_dump($m);
There was a problem hiding this comment.
whihc means your change is changing the feature, and so should be submitted separately as a feature addition (and not on 2.3)
There was a problem hiding this comment.
Not really,
I didn't introduce /u modifier, so [0-9] -> \d not changing behavior,
but can be safely switched to use unicode later (if the feature requested just /u modifier needs to be added).
There was a problem hiding this comment.
'/\d+/u' makes it support unicode numerics? Cool :)
There was a problem hiding this comment.
Yes, the same for \w, \W.
…o original version
|
Reverted \d, \w to not confuse people. |
This PR was squashed before being merged into the 2.3 branch (closes #14802). Discussion ---------- [HttpKernel] fix broken multiline <esi:remove> |Q |A | |--- |---| |Bug Fix? |yes| |New Feature? |n | |BC Breaks? |n | |Deprecations?|n | |Tests Pass? |yes| |Fixed Tickets| | |License |MIT| |Doc PR | | Originally found in #14800 (diff) `<esi:remove>` blocks with multiline contents were not removed. `<esi:comment>` blocks with multiline contents were not removed. Note. According to http://www.w3.org/TR/esi-lang `comment is an empty element, and must not have an end tag.` so the support for multi line comments are not actually supported in the standard. Commits ------- 06f97bf [HttpKernel] fix broken multiline <esi:remove>
This PR was squashed before being merged into the 2.3 branch (closes #14802). Discussion ---------- [HttpKernel] fix broken multiline <esi:remove> |Q |A | |--- |---| |Bug Fix? |yes| |New Feature? |n | |BC Breaks? |n | |Deprecations?|n | |Tests Pass? |yes| |Fixed Tickets| | |License |MIT| |Doc PR | | Originally found in symfony/symfony#14800 (diff) `<esi:remove>` blocks with multiline contents were not removed. `<esi:comment>` blocks with multiline contents were not removed. Note. According to http://www.w3.org/TR/esi-lang `comment is an empty element, and must not have an end tag.` so the support for multi line comments are not actually supported in the standard. Commits ------- 06f97bf [HttpKernel] fix broken multiline <esi:remove>
Static Code Analysis with Php Inspections (EA Extended):
- not optimal regular expressions usage
- strlen miss-use
- not optimal conditional statements in Process and Filesystem
- unsafe uniquid usage
- missing throws annotations