Skip to content

Conversation

mrossard
Copy link
Contributor

@mrossard mrossard commented Jul 15, 2023

Q A
Branch? 3.1
Tickets Closes #5610

This is a quick draft for completing/revisiting the work initiated here : #5611 , which seems abandoned right now.

@mrossard mrossard marked this pull request as draft July 15, 2023 13:50
@mrossard mrossard force-pushed the normalizer-getSupportedTypes branch 2 times, most recently from 6315311 to 8c28bed Compare July 15, 2023 16:12
@john-dufrene-dev
Copy link

@mrossard

With this commit, all depreciations disappear ?

@mrossard
Copy link
Contributor Author

@mrossard

With this commit, all depreciations disappear ?

it seems ok on my app, haven't tested everything yet.

@mrossard mrossard marked this pull request as ready for review July 17, 2023 13:18
@mrossard
Copy link
Contributor Author

Actually quite a lot of this is covered by existing tests, so i'll mark this as ready for review!

@mrossard mrossard force-pushed the normalizer-getSupportedTypes branch 3 times, most recently from 930ed97 to 16e7f32 Compare July 17, 2023 13:56
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

All hasCacheableSupportsMethod() must also be deprecated (as in Symfony).

@mrossard
Copy link
Contributor Author

Thank you for working on this!

All hasCacheableSupportsMethod() must also be deprecated (as in Symfony).

Shouldn't they be kept until APIP stops supporting older versions of SF? Using them instead of just copying their return values might be a mistake on my part though - i thought it could serve as a reminder that they are still useful for older SF versions.

@dunglas
Copy link
Member

dunglas commented Jul 20, 2023

Shouldn't they be kept until APIP stops supporting older versions of SF?

It's what we'll do, but we should trigger a deprecation anyway to be able to remove these methods when we'll drop support for old Symfony versions, as they are also part of our public API. Old versions of Symfony will still work with newer versions of API Platform, but a deprecation will be triggered (as excepted, because these methods really are deprecated :D).

Also, to prevent PHPStan errors, you can use this trick: https://github.com/symfony/serializer/blob/f783a7ce6a048393f14f30e1094619f7729b1eb0/Debug/TraceableNormalizer.php#L44

@mrossard
Copy link
Contributor Author

Shouldn't they be kept until APIP stops supporting older versions of SF?

It's what we'll do, but we should trigger a deprecation anyway to be able to remove these methods when we'll drop support for old Symfony versions, as they are also part of our public API. Old versions of Symfony will still work with newer versions of API Platform, but a deprecation will be triggered (as excepted, because these methods really are deprecated :D).

ok, i'll add deprecations for this then (and replace the calls I added right away while I'm at it). Would version 3.2 be the correct target for these?

Also, to prevent PHPStan errors, you can use this trick: https://github.com/symfony/serializer/blob/f783a7ce6a048393f14f30e1094619f7729b1eb0/Debug/TraceableNormalizer.php#L44

👍

@dunglas
Copy link
Member

dunglas commented Jul 20, 2023

No, please target 3.1. We add depreciation-free support of stable Symfony versions in all supported branches. Usually, we don't add deprecations in patch releases, but for this one, we'll make an exception because we're not responsible for this deprecation, it's one that has been introduced at the interface level by Symfony directly.

@mrossard mrossard force-pushed the normalizer-getSupportedTypes branch from 16e7f32 to 01effad Compare July 20, 2023 13:43
@mrossard mrossard requested a review from dunglas July 20, 2023 13:57
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, we're almost there!

To fix the remaining deprecation notices, you need to add the @group legacy annotation on tests testing the hasCacheableSupportsMethod() method (btw, it would be nice to add unit tests for getSupportedTypes()).

Normalizers from the TestBundle must also be upgraded to getSupportedTypes().

@mrossard mrossard force-pushed the normalizer-getSupportedTypes branch from 01effad to 189d167 Compare July 21, 2023 11:33
@mrossard mrossard force-pushed the normalizer-getSupportedTypes branch from 189d167 to 8989521 Compare July 22, 2023 07:27
@mrossard mrossard requested a review from dunglas July 22, 2023 07:47
@mrossard
Copy link
Contributor Author

This version should be better. I didn't add tests for getSupportedTypes() (yet?), but as they just return static arrays and are by design covered by functionnal tests I'm not sure how important those would be....?

@melroy89
Copy link

Thank you so much for this PR!

@dunglas dunglas merged commit db2cc95 into api-platform:3.1 Jul 24, 2023
@dunglas
Copy link
Member

dunglas commented Jul 24, 2023

Thank you @mrossard and @Oipnet!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants