-
Notifications
You must be signed in to change notification settings - Fork 64
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
Feature: mutations #176
Feature: mutations #176
Conversation
This is related to #147 , right? |
Right |
It can be used for all kinds of cases.
The first three are indeed also possible through a factory. The latter is the provider functionality I requested in #147. |
{ | ||
public function mutate(object $object): object | ||
{ | ||
$object->setFoo('mutated'); |
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.
There is a problem I think. If the setters are relaying on other objects which have dependency on di then it will not work as expected right ? The mutation expects the setters are made possible via normal intantiation or string currently.
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.
Well, the mutation class itself can be constructed with the di like any other class, so it is possible to inject its depencies (other classes, config vars or even the container itself).
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.
A second option would be to pass the container as a second argument to the mutate
method.
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.
Yes, I think it may be good to have container object.
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.
@harikt I had a look at it and my suggestion to pass the container as second argument is not possible after all. At least not without a large refactor because the container is not available in InjectionFactory
and Factory
. So in order to pass the container to the mutate
method we should somehow make the container available in those two classes (inject it).
I don't know if that is worth the effort since the mutation class can already be constructed as any other class. I added another example in the docs and included some more tests. The following works without a problem. I think this is in line with the rest of the library.
$di->params['Vendor\Mutation']['argX'] = $di->lazyGet('service');
$di->params['Vendor\Mutation']['argY'] = $di;
$di->mutations['Vendor\Object'][] = $di->lazyNew('Vendor\Mutation');
Hey @frederikbosch , I think you want to merge 4.x branch to yours. Else the same commits are showing in the diff which makes things hard for review. |
I will rebase and update the PR. |
Done the rebase. |
docs/mutations.md
Outdated
|
||
class ExampleMutation implements MutationInterface | ||
{ | ||
public function mutate(object $object): object |
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 noticed the MutationInterface
is having __invoke
and not mutate
. Also I think there need a better example. Basically in the examples you mentioned about arguments can be passed. But in the class they are missing which makes things a bit hard to digest. I am still trying to figure out the mutation and how they are working. I will look with a fresh eye later.
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.
Aight, I will add a better example.
@harikt I added some more docs and changed |
throw Exception::mutationDoesNotImplementInterface($mutation); | ||
} | ||
|
||
$object = $mutation($object); |
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.
Do you think we need to make sure the returned class is same for the object passed to mutation?
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.
Good thinking. I don't know for sure. I think that we should not do any checks. What would we gain from doing that?
I am good with the changes. @frederikbosch I hope you ok with waiting a bit for anyone else who love to review code. |
Sure, who would you like to review the code? Maybe we can add them as reviewers in this PR. |
@harikt Is there anything I can do to get this merged? |
No. Let us merge it. |
Adds a new feature. Allows class to be mutated after initialization. It can be seen as an abstracter method to modify the object after initialization compared to setters.