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.

Sign up for GitHub

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

Jump to bottom

Make doctrine/annotations dependency optional #2498

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Make doctrine/annotations dependency optional #2498

merged 1 commit into from
Jan 2, 2024

Conversation

franmomu
Copy link
Contributor

Q A
Type task
BC Break no
Fixed issues

Summary

Similar to doctrine/orm#8787, we have PHP attributes and XML mapping, there is no need to be a hard dependency.

@franmomu franmomu added the Task label Dec 27, 2022
@franmomu franmomu added this to the 2.5.0 milestone Dec 27, 2022
malarzm
malarzm reviewed Dec 27, 2022
View reviewed changes
Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

This warrants an UPDATE file entry IMO. On the other hand I'm thinking if it's worth doing in a minor version.

composer.json
@@ -49,6 +49,9 @@
"symfony/cache": "^4.4 || ^5.0 || ^6.0",
"vimeo/psalm": "^4.30.0"
},
"conflict": {
"doctrine/annotations": "<1.12 || >= 3.0"
},
"suggest": {
"ext-bcmath": "Decimal128 type support"
},
Copy link
Member

Choose a reason for hiding this comment

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

doctrine/annotations could be added to the suggest section

Copy link
Member

Choose a reason for hiding this comment

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

Having them in the suggestions can help if people read them and still need annotations. Older PHP versions still need them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 added

composer.json Outdated
@@ -49,6 +49,9 @@
"symfony/cache": "^4.4 || ^5.0 || ^6.0",
"vimeo/psalm": "^4.30.0"
},
"conflict": {
"doctrine/annotations": "<1.12 || >= 3.0"
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking, but one operator has an extra space while the other one does not ;)

SenseException
SenseException approved these changes Dec 27, 2022
View reviewed changes
Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

After adding annotations to composer-suggest this is 👍

@IonBazan IonBazan requested a review from malarzm January 6, 2023 09:36
@IonBazan
Copy link
Member

IonBazan commented Jan 8, 2023

This will currently affect the AttributeDriver too: https://github.com/doctrine/mongodb-odm/blob/2.4.x/lib/Doctrine/ODM/MongoDB/Mapping/Driver/AttributeDriver.php as it relies on Doctrine\Common\Annotations\Reader. How about we refactor that first to be completely independent from doctrine/annotations? We could even drop that package in v3.0.

@greg0ire
Copy link
Member

greg0ire commented Jan 8, 2023

Here is how you could proceed: doctrine/orm#9671

@franmomu franmomu marked this pull request as draft January 9, 2023 07:57
@franmomu
Copy link
Contributor Author

franmomu commented Jan 9, 2023

This will currently affect the AttributeDriver too: https://github.com/doctrine/mongodb-odm/blob/2.4.x/lib/Doctrine/ODM/MongoDB/Mapping/Driver/AttributeDriver.php as it relies on Doctrine\Common\Annotations\Reader. How about we refactor that first to be completely independent from doctrine/annotations? We could even drop that package in v3.0.

👍 I thought they were independents

alcaeus
alcaeus previously requested changes Jan 18, 2023
View reviewed changes
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Slapping a "request changes" on this for good measure. As noted before, we first need to make the attribute driver work independently of annotations, then we can make the dependency optional.

That said, I'm very much in favour of removing the hard requirement, and I would also do so in a minor version. We can then think of dropping support for annotations entirely in 3.x.

@IonBazan IonBazan modified the milestones: 2.5.0, 2.6.0 Mar 3, 2023
@IonBazan IonBazan changed the base branch from 2.5.x to 2.6.x March 3, 2023 03:33
@demiankatz demiankatz mentioned this pull request Mar 6, 2023
Closed
@alcaeus alcaeus modified the milestones: 2.6.0, 2.7.0 Nov 2, 2023
@franmomu franmomu changed the base branch from 2.6.x to 2.7.x December 31, 2023 12:27
@franmomu franmomu marked this pull request as ready for review December 31, 2023 12:28
malarzm
malarzm approved these changes Jan 2, 2024
View reviewed changes
composer.json
@@ -49,7 +49,11 @@
"symfony/cache": "^5.4 || ^6.0 || ^7.0",
"vimeo/psalm": "^5.9.0"
},
"conflict": {
"doctrine/annotations": "<1.12 || >=3.0"
Copy link
Member

Choose a reason for hiding this comment

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

FTR conflicting here is IMO OK just because we control the lib and said it's feature complete. Otherwise I'd be against having a conflict section as one could use the lib we're conflicting with, but not use it with ODM.

@malarzm malarzm requested a review from alcaeus January 2, 2024 13:10
@malarzm malarzm dismissed alcaeus’s stale review January 2, 2024 13:10

Prerequisites have been met

@malarzm malarzm merged commit c9b7f6b into doctrine:2.7.x Jan 2, 2024
16 checks passed
@malarzm malarzm removed the Task label Jan 2, 2024
@malarzm
Copy link
Member

malarzm commented Jan 2, 2024

I took the liberty and slapped a "BC Break" label for visibility in the release notes.

Thanks @franmomu!

@franmomu franmomu deleted the option_annotations branch January 2, 2024 16:57
@franmomu
Copy link
Contributor Author

franmomu commented Jan 2, 2024

I took the liberty and slapped a "BC Break" label for visibility in the release notes.

👍 thanks @malarzm

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

@malarzm malarzm malarzm approved these changes

@IonBazan IonBazan IonBazan approved these changes

@SenseException SenseException SenseException approved these changes

@alcaeus alcaeus Awaiting requested review from alcaeus

Assignees
No one assigned
Labels
BC Break
Projects
None yet
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.
6 participants
@franmomu @IonBazan @greg0ire @malarzm @alcaeus @SenseException