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

Can not register custom handler for multiple types since #645 #656

Closed
discordier opened this issue May 23, 2018 · 5 comments · Fixed by #657 or #658
Closed

Can not register custom handler for multiple types since #645 #656

discordier opened this issue May 23, 2018 · 5 comments · Fixed by #657 or #658
Labels

Comments

@discordier
Copy link
Contributor

The changes in #645 breaks registering the same service with multiple tags.

I have in my app a configuration, where I register some relation type:

tags:
   - { name: jms_serializer.handler, type: Relation, direction: serialization, format: json, method: serialize}
   - { name: jms_serializer.handler, type: Relation, direction: deserialization, format: json, method: deserialize}
   - { name: jms_serializer.handler, type: Relation<?>, direction: serialization, format: json, method: serialize}
   - { name: jms_serializer.handler, type: Relation<?>, direction: deserialization, format: json, method: deserialize}

Of these, only the very first tag is examined anymore, as all get priority 0 and the new code only examines the first tag.

This is pretty bad now, however, I do not know how to fix this without breaking the priority code again.

How shall we proceed here?

My first idea was to sort the handlers by priority per each direction, type and format and leaving only the one with the highest priority for final handling.

This works pretty well, however the subscribers, which are handled afterwards, will overwrite the handlers anyway (even in current code), which seems to be a glitch as well.
We might want to allow build an overall list over all configured handlers and handlers from subscriber results (which inherit the priority of the subscriber) and apply priority processing per direction+type+format afterwards.
This should provide us defined results which can be properly overridden and will not sacrifice features from previous versions

@discordier
Copy link
Contributor Author

PR opened, please see #657.

@narcoticfresh
Copy link

@discordier there also seems to be an issue if a service has multiple tags - but only one of them is relevant for the serializer..

i wanted to create a PR, but then have seen yours so it would conflict - i'm not sure if yours will land or not and when ;-)

example service definition is as follows:

    graviton.document.serializer.handler.extref:
        class: "%graviton.document.serializer.handler.extref.class%"
        arguments:
            - "@graviton.document.service.extrefconverter"
        tags:
          -
            name: jms_serializer.handler
            type: Graviton\DocumentBundle\Entity\ExtReference
            format: json
          -
            name: kernel.event_listener
            event: graviton.json_schema.constraint.format
            method: validateExtRef

With this definition, current master will complain with Each tag named "jms_serializer.handler" of service "%s" must have at least two attributes: "type" and "format". - which refers to the second tag (kernel.event_listener).

There is no check that all tags from the definition actually are relevant to the serializer.

It seems to me that your PR #657 does not fix this?

@goetas if this is a different issue - on what base should this fix be made? will #657 land and be the base of the change? or - @discordier - could you add this to your PR?

@goetas
Copy link
Collaborator

goetas commented May 25, 2018

@narcoticfresh can you test #658 ? this is the candidate solution to be merged

@goetas
Copy link
Collaborator

goetas commented May 25, 2018

if this is a different issue - on what base should this fix be made?

yes it is a different issue, i expect that #658 should become the solution for the first issue

@narcoticfresh
Copy link

@goetas just switched my dependency to discordier-hotfix/issue-656 of #658 - seems to fix the Each tag named "jms_serializer.handler" of service "%s" must have at least two attributes: "type" and "format" issue!

great, thanks! 👍

hope will land soon as this blocks our update to the new release.. we'll wait, no problem..

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