Skip to content

Remove annotation support #52

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

Merged
merged 2 commits into from
Jun 25, 2025
Merged

Remove annotation support #52

merged 2 commits into from
Jun 25, 2025

Conversation

Spea
Copy link
Contributor

@Spea Spea commented Jun 24, 2025

Drops support for serializer annotations. Type annotations still have to be parsed in order to properly set the types in the metadata.

@Spea Spea force-pushed the drop-annotation-support branch from 8df506e to 06b18d2 Compare June 24, 2025 17:52
@Spea Spea marked this pull request as ready for review June 24, 2025 17:53
Copy link
Member

@dbu dbu 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 this cleanup!

i see one problem with this: jms serializer still supports annotations. it might confuse people massively when suddenly their annotations are ignored. i wonder if we need to keep reading the jms annotations if present, and only drop our own annotations support (which should error with properly configured php, when trying to use a no longer existing annotation)

Support for JMS annotations will be kept for now (until the library itself drops the support), but since attributes is a known wording in the PHP world now, we change `annotation(s)` to `attribute(s)`
@Spea Spea force-pushed the drop-annotation-support branch from 06b18d2 to bc00e05 Compare June 25, 2025 12:27
@Spea
Copy link
Contributor Author

Spea commented Jun 25, 2025

thanks for this cleanup!

i see one problem with this: jms serializer still supports annotations. it might confuse people massively when suddenly their annotations are ignored. i wonder if we need to keep reading the jms annotations if present, and only drop our own annotations support (which should error with properly configured php, when trying to use a no longer existing annotation)

I was already hesitant in removing annotation support for JMS anyway, so I agree that we should keep it (for now) and only remove it if JMS itself decides to remove it as well. I reverted the changes that removed the annotation support, but I still went with renaming annotation(s) to attribute(s).

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot. yes i agree with the renaming, that is a potential BC break so we need to do this now.
when jms ditches annotations, we can do another major version.

in #48 , we had the idea to make attributes optional and not require the library anymore. then we would have to change the attribute parser to not attempt to read annotations if the library is not available. if you are up for that, would be cool to change it. but i am fine keeping it if you don't have the time to invest into it.

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@
* Adjusted code to not trigger warnings with PHP 8.4
* Removed deprecated `getDeserializeFormat` method from date time
* Add attribute support for the `@Preferred` annotation
* Remove annotation support for the `Preferred` class and only allow using it as an attribute
Copy link
Member

Choose a reason for hiding this comment

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

we should merge this with the above line:

* Replaced `@Preferred` annotation with the `#Preferred` attribute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense to not have two entries for the same change. I fixed this and amended to the original commit.

We no longer want to support annotations for the `Preferred` class, so we adjust the namespace and naming of the parser itself.
@Spea Spea force-pushed the drop-annotation-support branch from bc00e05 to b921e07 Compare June 25, 2025 14:03
@Spea
Copy link
Contributor Author

Spea commented Jun 25, 2025

in #48 , we had the idea to make attributes optional and not require the library anymore. then we would have to change the attribute parser to not attempt to read annotations if the library is not available. if you are up for that, would be cool to change it. but i am fine keeping it if you don't have the time to invest into it.

I'll gladly do this, but if its fine for you I'd rather do that in a separate PR.

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

yeah agree with separate merge requests.

thanks!

@dbu dbu merged commit b289780 into liip:2.x Jun 25, 2025
7 checks passed
@Spea
Copy link
Contributor Author

Spea commented Jun 26, 2025

in #48 , we had the idea to make attributes optional and not require the library anymore. then we would have to change the attribute parser to not attempt to read annotations if the library is not available. if you are up for that, would be cool to change it. but i am fine keeping it if you don't have the time to invest into it.

I'll gladly do this, but if its fine for you I'd rather do that in a separate PR.

@dbu I was thinking about this a bit more, and I wonder if we can actually do this, since there is still the PhpTypeParser that takes care of checking the types.

@dbu
Copy link
Member

dbu commented Jun 26, 2025

oh, you are very right. and i don't see us dropping the php type annotations anytime soon. afaik there is still no other way to e.g. do typed arrays.

then i think we don't need to do anything more on that for now.

from my point of view, this would be ready to tag a release. should i go for it or did you want to do something else before i tag?

@Spea
Copy link
Contributor Author

Spea commented Jun 26, 2025

from my point of view, this would be ready to tag a release. should i go for it or did you want to do something else before i tag?

Fine for me. I'm still working on the discriminator support, but that might take a bit longer. Though nothing is pressuring us since the serializer also has to go to a new major version when using the metadataparser 2.x, correct?

Anyway, I think its fine to do a release and we add discriminator support in 2.1 then.

@Spea Spea deleted the drop-annotation-support branch June 26, 2025 09:12
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.

2 participants