-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
ApiController Extender #2451
ApiController Extender #2451
Conversation
32e215a
to
0ee522f
Compare
Btw, the methods
|
42aa939
to
38f7111
Compare
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.
Looking great! A few minor comments
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.
Alright there's an issue I found while updating the bundled extensions to use this extender.
I passed an invokable class to prepareDataForSerialization
(to replace the event listener) and the problem is that the class cannot receive the $data
array by reference (which throws a php warning).
That's because when it comes to invokable classes, they're not directly called, it's the callback that returns the invokable which is called. (ContainerUtil::wrapCallback).
I think we can work around this if we receive the function arguments by reference when wrapping the invokable class object.
I'll make a separate pull request for that.
Dumb question, but aren't all arrays received by reference on account of them being mutable objects? I thought PHPs pass by reference was only needed for primitives and strings |
Arrays are not considered objects, so just like primitives they are always passed by value and you have to explicitly specify that you would like to receive them by reference to modify them, otherwise you're just modifying a copy. |
PHP why must you be like this 😬
Alright, good catch! Could we add some tests that will ensure that this will work once we fix this in |
One of the tests naturally fails, until #2485 is merged. |
6c14110
to
c884006
Compare
Part B of #1624
Part of #1891
Changes proposed in this pull request:
WillGetData
Event.WillSerializeData
Event.Reviewers should focus on:
WillGetData
to theAbstractSerializeController
, the signature for those methods are somewhat incomplete, some could use proper type hinting while other could explicitly support mixed parameter types ofstring|array
do we want to do that ?WillGetData
event for adding/removing includes ...etc ?Confirmed
composer test
).