Skip to content
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

Proposal: Add priorities feature to handlers #645

Merged
merged 1 commit into from
May 8, 2018

Conversation

adiq
Copy link
Contributor

@adiq adiq commented Mar 24, 2018

As a response to #642, #641 and #466 I prepared a proposal of change/feature in this regard, which should improve DX and let developers avoid confusion or similar problems in future.

What these changes introduce:

  1. Possibility to give jms_serializer.handler and jms_serializer.subscribing_handler tags a priority attribute
  2. CustomHandlersPass is loading handlers respecting order based on priorities
  3. Standard handlers have a default priority set to 100 (ones from JMS\Serializer namespace)
  4. Custom defined handlers (outside of namespace above) have a default priority set to 0 (will override standard ones by default)

@adiq
Copy link
Contributor Author

adiq commented May 2, 2018

Any feedback dear maintainers? 😉

@goetas
Copy link
Collaborator

goetas commented May 2, 2018

Hi, this will be definitively added, in this days im busy with the 2.0 release on the serializer, give me some time but will be done!

Copy link
Collaborator

@goetas goetas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all thanks for the PR and apologies if the review took to long.

I have only one change to request as expressed in the comment.
If you remove the special case for the jms priority, a lot of this pr can be simplified, no need for custom autoloader entry, the fixture can stay in the normal test namespace and so on.

Great contribution, thanks!

foreach ($container->findTaggedServiceIds($tagName, true) as $serviceId => $attributes) {
$definition = $container->getDefinition($serviceId);

if (!isset($attributes[0]['priority'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to keep this simpler without special cases. if the priority is not set, just set it to 0. users can still control priorities by adding their services by using negative priorities

@goetas goetas self-assigned this May 7, 2018
@goetas goetas merged commit de716d6 into schmittjoh:master May 8, 2018
@goetas
Copy link
Collaborator

goetas commented May 8, 2018

I did the changes by my self in 7bf1ea3

Thanks for the contribution!

@adiq
Copy link
Contributor Author

adiq commented May 8, 2018

Thanks @goetas! I am really happy to see that landed :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants