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

Allows both doctrine/common 3.x & doctrine/persistence 2.x #349

Merged
merged 1 commit into from Sep 1, 2020
Merged

Allows both doctrine/common 3.x & doctrine/persistence 2.x #349

merged 1 commit into from Sep 1, 2020

Conversation

fruitwasp
Copy link
Contributor

@fruitwasp fruitwasp commented Jun 25, 2020

Q A
Type improvement
BC Break yes
Fixed issues

Summary

Thoughts?

@greg0ire
Copy link
Member

Thoughts?

I think I already expressed them in another of your PRs, but IMO you should start contributing a maximum of things to the stable branch first. This applies to allowing common 3 and persistence 2 I think.

@fruitwasp
Copy link
Contributor Author

what about allowing doctrine/mongodb-odm 2.1.x? Required to allow doctrine/persistence 2.x (see doctrine/mongodb-odm#2204

Ok to drop doctrine/mongodb-odm 1.x too?

@greg0ire
Copy link
Member

greg0ire commented Jun 27, 2020

what about allowing doctrine/mongodb-odm 2.1.x?

If both versions can be handled by a single version of data-fixtures, why not?

Ok to drop doctrine/mongodb-odm 1.x too?

Preferrably in a subsequent PR.

@garak
Copy link
Contributor

garak commented Jul 8, 2020

Related PR in mongodb-odm has been merged

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Jul 23, 2020
This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

Allow doctrine/persistence 2

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | N/A
| License       | MIT
| Doc PR        | N/A

Replaces #37416.

This PR unblocks the installation of `doctrine/persistence` version 2.

Notes:
* I had to bump `doctrine/data-fixtures` because composer would otherwise downgrade to an ancient alpha release with incomplete version constraints. This package is a dev dependency for our tests, so I think we're good here.
  **edit**: Moved to #37640.
* Until doctrine/data-fixtures#349 has been resolved, Doctrine/Bridge cannot be tested with version 2 of `doctrine/persistence`.

Commits
-------

cd22fe6 Allow doctrine/persistence 2
@fruitwasp
Copy link
Contributor Author

tests seem to fail on registering annotations. The previously used AnnotationDriver::registerAnnotationClasses does no longer exist? Anyone got an idea on how to proceed?

@greg0ire
Copy link
Member

The error message says it Call to undefined method MongoDB\Collection::getIndexInfo(), how do does this relate with registering annotations?

@fruitwasp fruitwasp changed the base branch from master to 1.4.x August 24, 2020 10:10
@fruitwasp fruitwasp marked this pull request as ready for review August 24, 2020 10:11
@fruitwasp fruitwasp changed the title Adds support for doctrine/common version 3.x Allows both doctrine/common 3.x & doctrine/persistence 2.x Aug 24, 2020
@fruitwasp
Copy link
Contributor Author

Targets the 1.4.x stable branch now. Will bump doctrine/mongodb-odm in another pull request

Comment on lines +18 to +19
"doctrine/common": "^2.13|^3.0",
"doctrine/persistence": "^1.3.3|^2.0"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like none of the builds is installing versions fitting to the version constraints ^3.0 and ^2.0. How can we be sure that the newer versions will work too?

Copy link
Member

Choose a reason for hiding this comment

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

How can they be newer? 3 and 2 are the latest versions…

Copy link
Contributor Author

@fruitwasp fruitwasp Aug 25, 2020

Choose a reason for hiding this comment

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

Both doctrine/common 3 and doctrine/persistence 2 will not install on dev, because of doctrine/mongodb-odm 1.3.x (latest version is 2.1.x) not having support for doctrine/common 3.

A few options:

  1. Add support for doctrine/mongodb-odm 2.x & drop support for doctrine/mongodb-odm 1.3.x in this pull request. They don't go together nicely (tried to in a previous version of this pull request)
  2. After this pull request is merged upstream, go for the first option but on the dev branch instead.
  3. Replace doctrine/common in doctrine/mongodb-odm 1.3.x with the new doctrine packages like done in 2.x](doctrine/mongodb-odm@21501c4) could try something like this?

Copy link
Member

Choose a reason for hiding this comment

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

Option 3 sounds best, can you manage that?

Copy link
Contributor Author

@fruitwasp fruitwasp Aug 25, 2020

Choose a reason for hiding this comment

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

Worth a try ;)

Quite the task tho. A little help would be nice. Several classes (1) are no longer available due to the removal of doctrine/common. They will have to be replaced in a way, for which I've found docs https://www.doctrine-project.org/2018/07/12/common-2-9-and-dbal-2-8-and-orm-2-6-2.html

For example, to get proxying to work, we'll have to backport changes like doctrine/mongodb-odm#1875 This results in BC breaks, which I don't think is acceptable on this stable branch. Am I missing something?

(1) Classes no longer available;

Doctrine\Common\Proxy\AbstractProxyFactory
Doctrine\Common\Proxy\ProxyDefinition
Doctrine\Common\Proxy\ProxyGenerator
Doctrine\Common\Util\ClassUtils
Doctrine\Common\Util\Debug
Doctrine\Common\Proxy\Proxy

Will give supporting both doctrine/mongodb-odm 1.x & 2.x another try too at the end of the week.

Copy link
Member

@greg0ire greg0ire Aug 26, 2020

Choose a reason for hiding this comment

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

The classes above are still available in common v3. Why not upgrade to common 3 + replacement packages in v1 of doctrine/mongodb-odm?

Copy link
Member

@greg0ire greg0ire Aug 30, 2020

Choose a reason for hiding this comment

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

@fruitwasp after taking a deeper look to this, I think option 1 might be best in fact… Apparently doctrine/mongodb-odm is just a dev dependency, which means people can already install v2 alongside doctrine fixtures, right?

Copy link
Member

Choose a reason for hiding this comment

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

With data-fixtures as a dependency in a project, its require-dev parts won't be affected by a v2 constraint. Suggestion 2 would be possible.

How will this affect the build? Will there be one build that uses e.g. Common ^2.13 and a different one ^3.0? The compatibility with the code of all the given constraints of doctrine/common and doctrine/persistence should be tested in builds.

Copy link
Member

Choose a reason for hiding this comment

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

@SenseException now that #351 is merged, we have the prefer lowest build that uses 1 and 2, while all other builds use 2 and 3.

@weaverryan
Copy link
Contributor

weaverryan commented Aug 30, 2020

Hi!

Giving this a gentle bump :). This library (and DoctrineFixturesBundle) is no longer installable out-of-the-box on Symfony 5 apps (ref):

composer create-project symfony/skeleton testing_data_fixtures
cd testing_data_fixtures
# will lock doctrine/common on v3
composer require doctrine/doctrine-bundle doctrine/orm

# this will fail because we're locked at doctrine/common v3
composer require doctrine/data-fixtures

It looks like there is a MongoDB issue that we're still checking into? Is that all?

Thanks!

@greg0ire
Copy link
Member

It looks like there is a MongoDB issue that we're still checking into? Is that all?

I think this package needs to be made compatible with doctrine/mongodb-odm 2, yeah. When that's done, then the contents of this PR should be enough to fix the issue.

@alcaeus alcaeus mentioned this pull request Aug 31, 2020
@alcaeus
Copy link
Member

alcaeus commented Aug 31, 2020

@greg0ire see #351.

@greg0ire
Copy link
Member

greg0ire commented Sep 1, 2020

#351 has been merged, let's trigger a new build

@greg0ire greg0ire closed this Sep 1, 2020
@greg0ire greg0ire reopened this Sep 1, 2020
@fruitwasp
Copy link
Contributor Author

Looks like doctrine/common 3 & /persistence have been installed in the builds. Great work!

@greg0ire greg0ire merged commit 1aa44a4 into doctrine:1.4.x Sep 1, 2020
@greg0ire
Copy link
Member

greg0ire commented Sep 1, 2020

Thanks @fruitwasp !

@greg0ire greg0ire added this to the 1.4.4 milestone Sep 1, 2020
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.

None yet

7 participants