-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Serializer] optims and cleanup #26141
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
Conversation
return $this->appendCData($node, $val); | ||
} elseif (is_string($val)) { | ||
} elseif ($isString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure using a variable is faster at all (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the call is cheap and it's better to duplicate the call to is_string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep: the call is not an call anymore, it's an opcode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be written like this maybe:
} elseif (\is_string($val)) {
if ($this->needsCdataWrapping($val)) {
return $this->appendCData($node, $val);
} else {
return $this->appendText($node, $val);
}
} elseif {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wouldn't bring anything, the check is almost free
@@ -108,13 +108,13 @@ public function denormalize($data, $class, $format = null, array $context = arra | |||
$attribute = $this->nameConverter->denormalize($attribute); | |||
} | |||
|
|||
$allowed = false === $allowedAttributes || in_array($attribute, $allowedAttributes); | |||
$ignored = in_array($attribute, $this->ignoredAttributes); | |||
$allowed = false === $allowedAttributes || \in_array($attribute, $allowedAttributes, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could setting the strict
argument to true
create some BC break in some obscure edge case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's remove all "true" added to in_array checks (the 3rd arg is not needed to benefit from the optim)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, but both are strings so it cannot have a BC break.
Status: needs review |
$allowed = false === $allowedAttributes || in_array($paramName, $allowedAttributes); | ||
$ignored = in_array($paramName, $this->ignoredAttributes); | ||
$allowed = false === $allowedAttributes || \in_array($paramName, $allowedAttributes, true); | ||
$ignored = \in_array($paramName, $this->ignoredAttributes, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/, true//
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Thank you @dunglas. |
This PR was squashed before being merged into the 2.7 branch (closes #26141). Discussion ---------- [Serializer] optims and cleanup | Q | A | ------------- | --- | Branch? | 2.7 <!-- see below --> | Bug fix? | no | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | n/a <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | n/a Tiny optimizations and small code cleanup. The opcode triggering is especially useful in the hot path (normalizers and encoders) because it's a recursive process. Commits ------- 8ee8387 [Serializer] optims and cleanup
Tiny optimizations and small code cleanup. The opcode triggering is especially useful in the hot path (normalizers and encoders) because it's a recursive process.