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

fix EntityManager::create deprecations #2669

Merged
merged 1 commit into from
Sep 8, 2023
Merged

fix EntityManager::create deprecations #2669

merged 1 commit into from
Sep 8, 2023

Conversation

DubbleClick
Copy link
Contributor

No description provided.

@mbabker
Copy link
Contributor

mbabker commented Aug 17, 2023

You'd need to bump the minimum ORM version to 2.13, as well. In 2.12 and earlier, the constructor is protected, so those versions have to use the (now deprecated) create() method.

@DubbleClick
Copy link
Contributor Author

You'd need to bump the minimum ORM version to 2.13, as well. In 2.12 and earlier, the constructor is protected, so those versions have to use the (now deprecated) create() method.

Good catch, fixed.

doc/annotations.md Outdated Show resolved Hide resolved
@DubbleClick
Copy link
Contributor Author

I assume phpunit hiccuped for no particular reason as I see no correlation to the changes in the PR. Would you trigger workflows again @phansys ?

@DubbleClick
Copy link
Contributor Author

Well, I have no idea what php 7.2's problem is.

Copy link
Collaborator

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

DriverManager::getConnection() receives an EventManager as third argument, I haven't checked why it only fails using the lowest dependencies.

composer.json Outdated
@@ -56,7 +56,7 @@
"doctrine/dbal": "^2.13.1 || ^3.2",
"doctrine/doctrine-bundle": "^2.3",
"doctrine/mongodb-odm": "^2.3",
"doctrine/orm": "^2.10.2",
"doctrine/orm": "^2.13.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we should also change the conflict section to set this version since we are not testing < 2.13.0 anymore and that would require a changelog and release it as a minor 🤔

@franmomu
Copy link
Collaborator

franmomu commented Aug 19, 2023

DriverManager::getConnection() receives an EventManager as third argument, I haven't checked why it only fails using the lowest dependencies.

hmm probably it is not necessary to pass the eventmanager to the EntityManager

Update: see #2669 (comment)

@franmomu
Copy link
Collaborator

franmomu commented Aug 19, 2023

DriverManager::getConnection() receives an EventManager as third argument, I haven't checked why it only fails using the lowest dependencies.

apparently the third argument of the EntityManager constructor was added in 2.14, that's probably why it fails when installing the lowest dependencies.

I think we should move the EventManager argument to the DriverManager::getConnection() call and it should be fine (and do not pass it to EntityManager::__construct()).

Update: Connection::getEventManager() is deprecated, so I guess we should just bump to 2.14

@DubbleClick
Copy link
Contributor Author

Bumped the minimum requirement and also added the conflict. I'd have never realised the third argument was only added in 2.14, thanks for looking into it.

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (4010c0c) 79.60% compared to head (cb30629) 79.60%.

❗ Current head cb30629 differs from pull request most recent head 87b614b. Consider uploading reports for the commit 87b614b to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2669   +/-   ##
=======================================
  Coverage   79.60%   79.60%           
=======================================
  Files         161      161           
  Lines        8415     8415           
=======================================
  Hits         6699     6699           
  Misses       1716     1716           

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@franmomu franmomu left a comment

Choose a reason for hiding this comment

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

@phansys should we defer this to next minor version? we are dropping support of doctrine/orm < 2.14 so I guess a changelog and a minor version is needed

@phansys
Copy link
Collaborator

phansys commented Aug 19, 2023

@phansys should we defer this to next minor version? we are dropping support of doctrine/orm < 2.14 so I guess a changelog and a minor version is needed

Indeed.

@franmomu franmomu requested a review from phansys September 8, 2023 08:32
CHANGELOG.md Outdated
@@ -18,6 +18,8 @@ a release.
---

## [Unreleased]
### Removed
- Support for `doctrine/orm` < 2.14
Copy link
Collaborator

@phansys phansys Sep 8, 2023

Choose a reason for hiding this comment

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

IMO, if we are not dropping support for a major version, we don't need an entry here.
On the other hand, if these changes are intended to fix deprecations, I think we should add an entry in the Fixed section explaining what deprecations are fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 in this case these deprecations are only triggered in tests

composer.json Outdated
@@ -71,7 +71,7 @@
"conflict": {
"doctrine/dbal": "<2.13.1 || ^3.0 <3.2",
"doctrine/mongodb-odm": "<2.3",
"doctrine/orm": "<2.10.2 || 2.16.0 || 2.16.1",
"doctrine/orm": "<2.14.0 || 2.16.0 || 2.16.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"doctrine/orm": "<2.14.0 || 2.16.0 || 2.16.1",
"doctrine/orm": "2.16.0 || 2.16.1",

Copy link
Collaborator

@franmomu franmomu Sep 8, 2023

Choose a reason for hiding this comment

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

Here we need at least 2.14 to use the EntityManager constructor. I left the 2.10.2 version since it was there because #2272.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad, I didn't see that the "doctrine/orm": "^2.14.0" constraint is in the require-dev section.
I'm sorry, I think you can use your original approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what's the plan here, keep the 2.14 in the require, but <2.10.2 in the conflict? Anything I can do?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I should've kept the original <2.14 conflict 😞, but let's solve it in #2639

@franmomu franmomu merged commit 512695c into doctrine-extensions:main Sep 8, 2023
@franmomu
Copy link
Collaborator

franmomu commented Sep 8, 2023

thanks @DubbleClick!

@DubbleClick DubbleClick deleted the em_create branch September 8, 2023 12:23
@DubbleClick
Copy link
Contributor Author

You're most welcome :)

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

Successfully merging this pull request may close these issues.

4 participants