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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
],
"require": {
"php": "^7.2 || ^8.0",
"doctrine/common": "^2.11",
"doctrine/persistence": "^1.3.3"
"doctrine/common": "^2.13|^3.0",
"doctrine/persistence": "^1.3.3|^2.0"
Comment on lines +18 to +19
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

@SenseException SenseException Aug 31, 2020 •
edited
Loading

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.

},
"conflict": {
"doctrine/phpcr-odm": "<1.3.0"
Expand Down
6 changes: 0 additions & 6 deletions lib/Doctrine/Common/DataFixtures/ProxyReferenceRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

namespace Doctrine\Common\DataFixtures;

use Doctrine\Common\Util\ClassUtils;
use Doctrine\Common\Version;
use function file_exists;
use function file_get_contents;
use function file_put_contents;
Expand All @@ -31,10 +29,6 @@ class ProxyReferenceRepository extends ReferenceRepository
*/
protected function getRealClass($className)
{
if (Version::compare('2.2.0') <= 0) {
return ClassUtils::getRealClass($className);
}

if (substr($className, -5) === 'Proxy') {
return substr($className, 0, -5);
}
Expand Down