-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Migrate Internal namespace to PHP8 #9852
Conversation
end($baseElement[$relationAlias]); | ||
|
||
$this->_identifierMap[$path][$id[$parent]][$id[$dqlAlias]] = key($baseElement[$relationAlias]); | ||
$this->_identifierMap[$path][$id[$parent]][$id[$dqlAlias]] = array_key_last($baseElement[$relationAlias]); |
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 causes a new psalm error, that we baselined, because we don't think it's legitimate. But please triple check.
2c90661
to
951e0db
Compare
@@ -80,11 +75,10 @@ abstract class AbstractHydrator | |||
/** | |||
* Initializes a new instance of a class derived from <tt>AbstractHydrator</tt>. | |||
*/ | |||
public function __construct(EntityManagerInterface $em) | |||
public function __construct(protected EntityManagerInterface $_em) |
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.
Let's not pollute the public API with those unfortunate underscores.
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.
Wasn't the public API already polluted by this (there was a protected property with that name). Unless we are taking named properties into account, the public API is not more polluted than it already was. But maybe we should stop polluted the public API with those, as an additional BC-break?
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.
The underscore can be removed in a separate PR to keep this PR small.
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.
My point is that the underscore is being added to the constructor parameter with this PR. We should avoid that. If that means, we cannot use CPP yet for this parameter, so be it.
Thanks @MarcBrillault ! |
Refs #9772