[B/C Break] Make the Annotations package optional in the builder API

[mbabker]

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	yes
Deprecations?	no
Tests pass?	no
Fixed tickets	N/A
License	MIT

The below are the changes needed to (in theory) make the Annotations package an optional thing (note that Annotations is hard required by the dev PHPCR ODM and PHPBench dependencies, so right now it's not possible to actually not install the package without playing with Composer)

[mbabker]

Rebased, should be good to go now. I've also gone ahead and moved the Annotations package to require-dev.

The high CI fails are unrelated, if I had to take a guess it's related to the ORM 2.16 release as the CI from when #1494 was merged passed but both this and #1497 are now failing and the only difference in the dependency lists between those runs is that new release. If I had to guess, https://github.com/doctrine/orm/pull/10547 is the root cause as the CI fails are changed IDs between objects, so the test probably needs an update to accommodate that change in behavior.

[scyzoryck]

Looks good for me. Personally I would release it after adding support for ORM 3.0. @goetas what do you think? Should we release it as a new major version?

[kevinpapst]

Bump: one of the last major packages that still has a hard dependency on doctrine/annotations. Would be nice to get rid of it. Anything I can do to help?

[scyzoryck]

I will release it on Monday morning. Thanks @mbabker for your work on this ticket!

[Steveb-p]

I think it should not be a big issues. JMS Bundle seems to not using those classes, so it will affect only people who is using package without Bundle.

@scyzoryck We are, in fact, using it with bundle and replacing the implementation with our own. And given that we have stable releases with this serializer as a dependency, we will have projects of our partners breaking.

[scyzoryck]

Hello! Thanks for feedback! Yes, we are aware about potential affect on some users - but expect that BC break will affect only few of them - with easy fix and easy way to fix that before deploying to production. I'm sorry that your work will be affected too.

The package itself is not backed by any big companies - it is mainly voluntary work of few people with limited amount of free time. Unfortunately sometimes we need to make the some tough decisions to bring more exciting features and ensure long stability of the project. We always tries to deliver best value and features for others. If you found it useful for your project - it is always a good idea to support this project with contribution or funding :)

Best!

[Steveb-p]

@scyzoryck Understandable :sweat_smile: I kinda just wanted to note that it broke the software that we are giving to others. Given the broad experience spread in our little PHP world, it will take a little more or less for different developers.

I hope I didn't offend you, as my intent is only to note that this particular choice had some impact, nothing more :) I actually deleted a more playful part of the comment when writing, and text is rather bad at conveying intent :)

As for a potential resolution and/or fix for similar things happening in the future, you might consider going the same way as Symfony often does - by removing the argument from the interface altogether, and adding it as a comment. They do that often for things they deprecate.

Similarly maybe to how Symfony changed the argument order in EventDispatcherInterface a couple years ago: https://github.com/nicolas-grekas/symfony/blob/75369dabb8af73b0d0ad7f206d85c08cf39117f8/src/Symfony/Component/EventDispatcher/EventDispatcherInterface.php#L32

Not ideal, but allows some flexibility without introducing major release, I guess?

[scyzoryck]

Thanks for suggestion! I know Symfony approach - however it do not work so well when removing required dependencies - so it would not be super useful in that case 😓

[mbabker]

I think just shipping this as is and calling it out as the B/C break it was without a major version bump was probably the best least disruptive option. Yeah, there was a chance it was going to impact someone, but there was no code change that wouldn't have.

Fully removing the argument would've introduced static analysis issues (passing more arguments than defined), commenting the argument is generally Symfony's signal that a new argument is being added to the signature, and Symfony has a history of treating moving dependencies from require to require-dev as soft breaks to avoid breaking apps relying on transient dependencies. I think if I would've made this PR a full "deprecate all annotations support" PR I'd have done the remove the argument from the signature bit, but as is this should be the last purposeful B/C issue until a 4.0 release comes together.

[Steveb-p]

Allow me to have a differing opinion on the matter, and respond. Note that those are my personal opinions and I fully value the approach you have taken (and work put into removing annotation dependency). I just don't agree with the notion of "there would always be some impact".

...Yeah, there was a chance it was going to impact someone, but there was no code change that wouldn't have.

With all due respect, I disagree with that. This could've been solved without breaking the implementations.

Thanks for suggestion! I know Symfony approach - however it do not work so well when removing required dependencies - so it would not be super useful in that case 😓

...commenting the argument is generally Symfony's signal that a new argument is being added to the signature...

The example I've given is exactly when a required argument was being removed from the interface, and arguments were swapping places (event name argument becoming an optional one).

...Fully removing the argument would've introduced static analysis issues (passing more arguments than defined)...

Yes, but no. That could've been solved by adding a stub file, where the newer interface declaration could be used to give static analysis correct information about expected class shape. This has been done in phpstan/phpstan-symfony as well, even specifically for the mentioned EventDispatcher changes.

...and Symfony has a history of treating moving dependencies from require to require-dev as soft breaks to avoid breaking apps relying on transient dependencies...

I agree that this is what often happens with Symfony, as those soft-breaks happened to us too. I would not have anything against annotation package removal, as this would indicate that a direct dependency was missing from one's library/project.

I'll be happy to provide you with static analysis compatible interface declaration (including the mentioned stub) if you're willing to take it into consideration.

[ebitkov]

Can someone please explain how I re-enable annotation support for the moment, as long as some packages still switching to attributes? The documentation wasn't really clear about it.

note: I'm initializing the serializer like SerializerBuilder::create()->build();

edit: As @Levivb suggested, requiring doctrine/annotations seems to do the trick.

[Levivb]

@ebitkov I'de suggest requiring doctrine/annotations explicitly yourself in your composer.json. That way, the JMS Serializer will automatically pick up the AnnotationReader

[boesing]

Any reason on why this was not a new major? I mean, this is not boesing/typed-arrays with 18k overall installs but jms/serializer which supposed to be THE serializer (along with the symfony one I guess) which has almost 100 million installs overall.

IMHO there is responsibility for packages like this one and just "yah lets merge and put it in the changelog" is not what I was expecting from a project this size.

Just wanted to drop this here, might be helpful for future releases...

[goetas]

As pointed out by @scyzoryck in https://github.com/schmittjoh/serializer/pull/1471#issuecomment-1963918460 this is a project run by volunteers working in their limited free time.

Releasing a new major version would have forced us to upgrade a few other projects such as fos rest, nelmio apidoc, Hateoas bundle... As you can see, it would force us to do a lot more work than what it seems.

I was aware that the changes were a possible BC break for some projects, but was the best trade off option I had in mind given the limited time we have.

I'm sorry that the bc break impacted some users, hopefully tests were able to catch the issue and no disruption in production happened.