Allow doctrine/persistence 2.x

[fruitwasp]

• Allows doctrine/persistence 2.x
• Updates namespace of Doctrine persistence classes in tests.

[ostrolucky]

forward compatibility layer is present in 1.3, but no backwards compatibility layer is present in 2.0 AFAIK. We cannot suddenly start wiring incompatible classes, it will break projects that expect us wiring common\persistence compatible services.

[greg0ire]

Maybe create service with the old FQCN as service id, but the new FQCN as class, and deprecate those in favor of services with the new FQCN as service id (and class)?

[ostrolucky]

I would rather drop support for persistence 1.x

[fruitwasp]

Agree on dropping support for 1.x. Apart from this, it requires support for 2.x in the symfony/doctrine-bridge too, as this bundle has a dependency on it. Will target master. symfony/symfony@master...fruitwasp:doctrine-bridge your thoughts?

[ostrolucky]

That could work. Plan is that next minor doctrine-bundle release will by synced with next minor symfony release.

[greg0ire]

IMO if you don't add the || ^2.0 and require a recent enough version of persistence, you can contribute a lot of the changes in this PR to the stable branch, provided you add deprecated aliases with the old names. I would start there.

[nusphere]

the actual release ob Symfony allow to install doctrine/persistence 2.x #1190 - this breaks some update processes

[stof]

Note that the title of this PR is not describing what it does. currently, it does not only add support for 2.x, but it also removes support for 1.x

[nicolas-grekas]

Keeping support for v1 would be great for the community.

[ostrolucky]

Maybe I'm missing something, but I don't see how can we solve issue I mentioned for persistence 2.x and at the same time keep support for v1

[greg0ire]

@ostrolucky what's wrong with #1181 (comment) ?

[ostrolucky]

Problem with that is people who inject common\persistence\* will get incompatible persistence\* like I mentioned earlier

[greg0ire]

it will break projects that expect us wiring common\persistence compatible services.

Are you thinking of projects that require doctrine/common but not (yet) doctrine/persistence? If yes, a solution would be to require doctrine/common 3 first. That would force said packages to express their dependency to doctrine/persistence, as mentioned in https://github.com/doctrine/common/releases/tag/3.0.0

[ostrolucky]

doctrine/common 3.0 requires doctrine/persistence 2.0, so end result is same - unsupported doctrine/persistence 1.x

[greg0ire]

Ah right indeed. @nicolas-grekas persistence 1 and 2 have very similar constraints, so I think it wouldn't actually be that big of a deal. We might want to require common 3 or at least conflict with anything below that to be on the safe side.

[stof]

common\persistence\* class names are class aliases for the new name in doctrine 1.3+.

If your code relies on these aliases, it is indeed not compatible with doctrine/persistence 2.x. But that's solved by requiring doctrine/persistence 1.x in that package (which should be in your dependencies anyway if you rely on it).
this is not a blocker for making DoctrineBundle compatible with both 1.x and 2.x (with the right lower bound on 1.x to ensure we have access to the new names)

[greg0ire]

which should be in your dependencies anyway if you rely on it

That's the thing, you could be relying on common\persistence\* and have only a version of doctrine/common older than 2.10, that would be valid (although less and less likely I suppose). Even having something newer and still only have "doctrine/common": ~2.10 is valid, having that namespace available is part of the contract of common 2.

[stof]

@greg0ire no. Having that namespace available is part of the contract of doctrine/persistence 1.4+ as that's where they are implemented (the 1.4 version number is from my memory. Check the changelog to validate it). We could make DoctrineBundle conflict with older versions.

[greg0ire]

it's 1.3 and I think you are referring to the namespace w/o Common, which I wasn't referring to with "that namespace". So to sum up, you could:

• have package A require doctrine/common 2 and be a legit user of Common\Persistence\*
• require A in your project
• also require doctrinebundle

But I just noticed that doctrine/common 2 requires doctrine/persistence 1, so I would say we are in fact not at risk here 👍

[ostrolucky]

So what needs to be done?

[greg0ire]

Since the early versions of the BC layer are buggy, I'd use ^1.3.7 || ^2.0 as a constraint. My comment about aliases can be ignored I think, since the definitions changed in this PR are not public (right?). Also, not sure why the build fails so badly, maybe it's because of the constraint?

[ostrolucky]

Then @fruitwasp sorry about that but can you please change back constraint to allow persistence ^1.3.3?

[fruitwasp]

Travis coverage test failing seems to be unrelated?

[greg0ire]

I'm trying to address it in #1195 , it has to do with Symfony 5.2

[greg0ire]

@fruitwasp please retarget to 2.0.x as I asked in #1181 (comment), it should fix the build.

[greg0ire]

Nevermind, I did it for you.

[greg0ire]

Although this looks like it should target 2.1.x, I choose to target 2.0.x because it should fix #1197 , in which upgrade the ORM and Common results in this bundle being dowgraded to 2.0.2, which does not have an explicit dependency on Common or Persistence => 💥

[greg0ire]

@fruitwasp this should no longer be a draft, right?

[greg0ire]

Woops sorry, @ostrolucky pointed out to me that the correct branch for this is 2.1.x

[greg0ire]

Thanks @fruitwasp !