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

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

Merged
merged 1 commit into from
Feb 24, 2024

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Feb 5, 2023

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)

@@ -9,5 +9,5 @@

interface DriverFactoryInterface
{
public function createDriver(array $metadataDirs, Reader $annotationReader): DriverInterface;
public function createDriver(array $metadataDirs, ?Reader $annotationReader = null): DriverInterface;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one line is why there is a B/C break; widening the annotation reader argument to allow null causes compile errors in classes if they don't do the same.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@scyzoryck scyzoryck 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 contribution!

@@ -9,5 +9,5 @@

interface DriverFactoryInterface
{
public function createDriver(array $metadataDirs, Reader $annotationReader): DriverInterface;
public function createDriver(array $metadataDirs, ?Reader $annotationReader = null): DriverInterface;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

$driver = new AnnotationDriver($annotationReader, $this->propertyNamingStrategy, $this->typeParser);
if ($annotationReader) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder is we should modify ChainDriver too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The instance at line 76 just outside this diff or something about the class itself?

There isn't a prepend method on the chain, so the implementation I have now ended up being the simplest one considering that when metadata paths are given, the factory pushes the YAML and XML drivers up in priority over the annotations driver (and the same goes for the default ordering in the bundle service definitions).

@mbabker
Copy link
Contributor Author

mbabker commented Aug 1, 2023

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, doctrine/orm#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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

scyzoryck commented Feb 24, 2024

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

@scyzoryck scyzoryck merged commit bf11053 into schmittjoh:master Feb 24, 2024
17 of 18 checks passed
@mbabker mbabker deleted the optional-annotations branch February 24, 2024 17:10
@Steveb-p
Copy link

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
Copy link
Collaborator

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
Copy link

Steveb-p commented Feb 26, 2024

@scyzoryck Understandable 😅 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
Copy link
Collaborator

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
Copy link
Contributor Author

mbabker commented Feb 26, 2024

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
Copy link

Steveb-p commented Feb 26, 2024

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
Copy link

ebitkov commented Feb 27, 2024

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
Copy link

Levivb commented Feb 27, 2024

@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
Copy link
Contributor

boesing commented Mar 8, 2024

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
Copy link
Collaborator

goetas commented Apr 4, 2024

As pointed out by @scyzoryck in #1471 (comment) 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.

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.

8 participants