-
Notifications
You must be signed in to change notification settings - Fork 2
add extension api #137
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
base: 2.0.x
Are you sure you want to change the base?
add extension api #137
Conversation
|
Hello 👋 here is the most recent benchmark result:
This comment gets update everytime a new commit comes in! |
|
|
||
| interface GuesserProvider | ||
| { | ||
| /** @return iterable<Guesser|array{0: Guesser, 1?: int}> */ |
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.
| /** @return iterable<Guesser|array{0: Guesser, 1?: int}> */ | |
| /** @return iterable<array{0: Guesser, 1?: int}> */ |
I think reducing it to this is a better api, otherwise we have 2 version's to omit the priority as the also allow here the priority to be missing. wdyt?
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 do not think either "type" is harmful, and I like the DX of the first one. Besides, it is practically the same principle as in Symfony.
|
|
||
| interface MetadataEnricherProvider | ||
| { | ||
| /** @return iterable<MetadataEnricher|array{0: MetadataEnricher, 1?: int}> */ |
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.
same here
| use Patchlevel\Hydrator\Middleware\Middleware; | ||
| use Patchlevel\Hydrator\MiddlewareProvider; | ||
|
|
||
| final class CryptographyExtension implements MiddlewareProvider, MetadataEnricherProvider |
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.
other idea which i have in mind is: instead of this provider use attributes to define middleware/metadataenricher?
#[Middleware(priority: 64)]
final readonly class CryptographyMiddleware implements Middleware {}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, I do not want to work with attributes here. I think attributes are good if you want to enrich your own code (from your domain) with information from external sources. But not as an extension point for the library. I would always prefer interfaces for that.
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 dont want to remove the interface (I missed it in the small code example) it's more about the provider / configuration of the priority
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 extension is there to register the middleware, etc. I find it unnecessary to read the attributes from it. In my opinion, that adds no value and only complicates things.
I am also considering whether interfaces like MiddlewareProvider are overkill, and whether it would be better to simply provide an abstract class Extension that predefines methods with empty arrays. That would also make things simpler.
No description provided.