-
-
Notifications
You must be signed in to change notification settings - Fork 295
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
Fix ProxyGenerator magic __set with void return type #921
Fix ProxyGenerator magic __set with void return type #921
Conversation
Please create a test that covers the bug behavior. |
@@ -24,7 +24,7 @@ class MagicSetClassWithScalarTypeAndRenamedParameters | |||
* | |||
* @throws BadMethodCallException | |||
*/ | |||
public function __set($n, $val) |
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 class is part of a different test and it should keep __set($n, $val)
to cover cases where no types are used. Your case is already covered with MagicSetClassWithScalarTypeAndVoidReturnType
.
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'll undo that change, but the __set()
there should definitely have a scalar type in it's signature, otherwise the name of the test class is misleading. As I understood it, the MagicSetClass
class does not have any type in its signature, while the MagicSetClassWithScalarType
class does have a type in its signature, hence the name. So the MagicSetClassWithScalarTypeAndRenamedParameters
should either be renamed to MagicSetClassWithRenamedParameters
or have the type added, to make this consistent. But that could also be proposed in yet another PR, as it does not impact the changes introduced with this one.
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 don't know about the intention of the naming, so let's keep those names for now, while keeping the original state of this __set()
. Your new test will cover the type.
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.
Agreed. I'm going to revert this change now and hand in the PR for a re-review :)
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'd prefer a second Doctrine reviewer who's taking a look at this. From my pov this looks good.
Thanks @GameplayJDK! |
See #917 for details.