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

Change API Platform priorities #2357

Closed
dunglas opened this issue Dec 1, 2018 · 16 comments
Closed

Change API Platform priorities #2357

dunglas opened this issue Dec 1, 2018 · 16 comments

Comments

@dunglas
Copy link
Member

dunglas commented Dec 1, 2018

With the rise of autowiring, user provided normalizers must be called before API Platform ones. It's currently not the case, and you have to set a custom priority, it hurts DX.

Current priorities:

  • ApiPlatform\Core\Hydra\Serializer\ConstraintViolationListNormalizer
  • ApiPlatform\Core\Hydra\Serializer\DocumentationNormalizer
  • ApiPlatform\Core\Hydra\Serializer\EntrypointNormalizer
  • ApiPlatform\Core\Hydra\Serializer\ErrorNormalizer
  • ApiPlatform\Core\Swagger\Serializer\ApiGatewayNormalizer
  • ApiPlatform\Core\Swagger\Serializer\ApiGatewayNormalizer
  • ApiPlatform\Core\Hydra\Serializer\CollectionFiltersNormalizer
  • ApiPlatform\Core\Problem\Serializer\ConstraintViolationListNormalizer
  • ApiPlatform\Core\JsonLd\Serializer\ItemNormalizer
  • ApiPlatform\Core\Problem\Serializer\ErrorNormalizer
  • ApiPlatform\Core\GraphQl\Serializer\ItemNormalizer
  • App\Serializer\ReactionCollectionNormalizer <-- autowired normlaizer is here
  • ApiPlatform\Core\Serializer\ItemNormalizer
  • Symfony\Component\Serializer\Normalizer\JsonSerializableNormalizer
  • Symfony\Component\Serializer\Normalizer\DateTimeNormalizer
  • Symfony\Component\Serializer\Normalizer\ConstraintViolationListNormalizer
  • Symfony\Component\Serializer\Normalizer\DateIntervalNormalizer
  • Symfony\Component\Serializer\Normalizer\DataUriNormalizer
  • Symfony\Component\Serializer\Normalizer\ArrayDenormalizer
  • Symfony\Component\Serializer\Normalizer\ObjectNormalizer

Related: #2315

@Taluu
Copy link
Contributor

Taluu commented Dec 1, 2018

There's also #1815 that can be related, but it's not only normalizers.

@soyuka soyuka added the bug label Dec 7, 2018
@teohhanhui
Copy link
Contributor

Not a bug.

@teohhanhui
Copy link
Contributor

Changing priorities is a BC break, isn't it?

@dunglas
Copy link
Member Author

dunglas commented Dec 19, 2018

It is. For 3.0?

@wimme002
Copy link

wimme002 commented Jan 7, 2019

Upgrading Symfony to 4.2.2 changes the order of the dataproviders and can break an application.

providers in ChainCollectionDataProvider on Symfony 4.2.2

  • "ApiPlatform\Core\Bridge\Doctrine\Orm\CollectionDataProvider"
  • "App\DataProvider\MyProvider"

providers in ChainCollectionDataProvider on Symfony 4.2.1

  • "App\DataProvider\MyProvider"
  • "ApiPlatform\Core\Bridge\Doctrine\Orm\CollectionDataProvider"

This means that the only way to use a custom Provider is to manually define it in services.yaml with a priority > 0

@olivermack
Copy link

@wimme002 thanks for your note - I actually ran in exactly this issue ;).

@dunglas
Copy link
Member Author

dunglas commented Jan 8, 2019

This "non-bug" is affecting many users... Maybe should we do something before 3.0, even if it's a potential BC break, it will be more reliable and less confusing.

@antograssiot
Copy link
Contributor

I think it is worth it

@deguif
Copy link
Contributor

deguif commented Jan 10, 2019

This seems related to symfony/symfony#29836

@lyrixx
Copy link
Contributor

lyrixx commented Jan 10, 2019

With 4.2.2 or 3.4.21 it's a big mess. As @deguif said, there is a new issue with tagged service.
When I added a priority on the tag of APIP service, everything was fine.
But, if it's not a bug...

@apfelbox
Copy link

Couldn't you just give the API Platform normalizers a negative priority? That way they would end up at the end, as the default priority is 0?

@dunglas
Copy link
Member Author

dunglas commented Jan 10, 2019

It's not a "bug" because these priorities have been defined before the introduction of autoconfiguration in Symfony. However now that it's broadly used, it's very annoying.

The only issue with changing this, it's that's a BC break that will definitely affect existing users. Anyway, this bug in Symfony is maybe the opportunity to move forward?

@apfelbox negative priorities would indeed be the best solution! PRs welcome guys :)

@juhulee
Copy link

juhulee commented Jan 15, 2019

hey, having the same problem.. do you have any example how to change the priority?
I tried the default stuff in services.yaml..
` ############################################
# EVENT LISTENERS + SUBSCRIBERS
############################################

# we need to overwrite the blamable listener definition in order to adjust its priority
# the default priority is 0 and thus it does not add the updatedBy before creating a logEntry
stof_doctrine_extensions.event_listener.blame:
    class: "%stof_doctrine_extensions.event_listener.blame.class%"
    arguments:
        - "@stof_doctrine_extensions.listener.blameable"
        - "@security.token_storage"
        - "@security.authorization_checker"
    tags:
        - { name: kernel.event_subscriber, priority: -1 }

`
I'm not using a special listener or subscriber, just the default stuff. this code above has been commented out, I just removed the comments and changed the priority from 10 to -1 ..
no difference, the error still occures..
Thanks,
Julia

@juhulee
Copy link

juhulee commented Jan 15, 2019

hmm but I just saw, that the PR has been reverted and already merged to symfony 3.4
waiting for 4.2 ;)
symfony/symfony#29853

@apfelbox
Copy link

@juhulee I am not a user of the API platform myself, but if I understand it correctly your own listener needs to be added before the core listeners. So you need to use a priority larger than zero.

@soyuka
Copy link
Member

soyuka commented Apr 7, 2019

That's done in 2.4, changes have been logged in the changelog!

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

No branches or pull requests