-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add middleware feature #47
base: master
Are you sure you want to change the base?
Conversation
e2f20d5
to
a2cb66d
Compare
18ed9e4
to
8ba45d9
Compare
I have rebased this branch on current master, could you review it ? |
I also added another commit that makes custom mapper more consistant with mapping configuration and configured middlewares. |
e85de1b
to
ae2fd2d
Compare
// You can either extend the CustomMapper, or just implement the MapperInterface | ||
// directly. | ||
class EmployeeMapper extends CustomMapper | ||
class EmployeeMapper implements DestinationMapperInterface |
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.
Note that CustomMapper is deprecated, and map method of the AbstractMapper class won't be invoked anymore (object is constructed through the automapper configuration, and then passed to mapToObject method)
ae2fd2d
to
a11d490
Compare
Just fixed an issue that was running default mapper and property middlewares two times. |
* An instance of class $to. | ||
* @throws UnregisteredMappingException | ||
*/ | ||
public function map($source, string $targetClass/**, array $context = [] */); |
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.
Since this is a completely new interface, we can actually make the $context
part of the interface
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.
Oh wait, the existing interface extends this one, disregard!
@@ -202,11 +227,64 @@ public function registerMapping( | |||
return $mapping; | |||
} | |||
|
|||
|
|||
public function registerMiddlewares(Middleware ...$middlewares): AutoMapperConfigInterface |
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 way you can only register all middlewares at once. It might be interesting to add an addPropertyMiddleware
and addMapperMiddleware
method. Or maybe we can take a look at Guzzle's way of doing things and make use of a separate stack class.
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.
Ok, i'll change this ! Sorry for the delay, i'm busy on one project right now, but I use this current fork for it and it really solves many use cases :).
I'll try to update the pull request tomorrow.
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.
No problem at all; I took a long time to reply myself :)
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.
@mark-gerarts I'm reading this pull request again, and it's not required to register all middlewares at once, because registerMiddlewares is chainable, and invoking registerMiddleswares won't remove already registered middlewares.
So you can add middlewares one by one, by calling registerMiddlewares as many times as you want.
Do you plan to merge this PR and release 2.0 ?
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 want to include this in release 2.0, which I aim to release this summer. As for the adding of middlewares: registerMiddlewares does indeed allow for chaining, but maybe It's useful to add some utility methods such as add. I'll look into this into detail once I start merging the PR.
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.
Hi @mark-gerarts, any ETA for this release ?
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've set up a board for the release. The scope is rather limited, but my time is as well at the moment. To be honest, I have not really been able to work on the release the past months.
I should be able to finally work a bit on the release again the following weeks (:crossed_fingers:). The question is if it will be enough to finish all open tasks. I'll keep you posted once I've worked myself in again and have a better overview!
@mark-gerarts I'll be happy to help. I think i'll have some spare time during the following weeks, so I would be happy to help you maintaining this project and performing the release if you wish too. |
@Toilal Thanks for the offer, it's very welcome! Let me know if you need help getting started. I should always be able to find some time myself to review and test changes. |
dd42f8b
to
6e4e9a0
Compare
Hi, any good news from this PR? |
This is a rewrite of #46 will what I think is a real middleware pattern now.