Add MappingDriver::getDriver()

[Geekimo]

This PR fixes a BC break with #1284 that doesn't allow getting MappingDriverChain with the newly introduced MappingDriver class.
This PR will help solve issue 841 on the Symfony maker bundle.

[weaverryan]

In MakerBundle, we're a bit "greedy" - we look specifically for the MappingDriverChain so that we can loop over all of them to find out which "mapping type" a specific entity is using. Here is the logic: https://github.com/symfony/maker-bundle/blob/a9ff8a8600b6354d79fd16c72c24757dcb086c3c/src/Doctrine/DoctrineHelper.php#L89-L110

In 2.3, since the mapping driver is NOT A chain anymore... we kind of get confused and can't figure out which mapping type is allowed. If there is a cleaner way for us to figure that out, that would be idea. Otherwise, this PR seems reasonable - it gives access to the inner MappingDriverInterface.

Thanks!

[stof]

Note that this does not actually restore BC, as it still requires changes in packages inspecting the driver (but it allows them to adapt).

A BC solution might be to implement the feature using a listener on the loadClassMetadata event rather than decorating the driver.

[nicolas-grekas]

A BC solution might be to implement the feature using a listener on the loadClassMetadata event rather than decorating the driver.

Not sure why I did not consider this option earlier. PR welcome I guess. Maybe @Geekimo you'd like to give it a try?

Meanwhile, I also think that MakerBundle should not be relying on specific wiring details at runtime.

Here is a fix on MakerBundle to solve this on its side, building on DI instead:
symfony/maker-bundle#844

[nicolas-grekas]

A BC solution might be to implement the feature using a listener on the loadClassMetadata event rather than decorating the driver.

I had a look and this wouldn't work, as we need to hook before ClassMetadataFactory::completeIdGeneratorMapping().

I'm closing here, thank you for sending the PR @Geekimo!

[stof]

@nicolas-grekas why closing this PR ?

[stof]

MakerBundle is not the only one that would benefit from this PR (see #1305)

[nicolas-grekas]

Because the right fix should be to wire deps appropriately, not hacking against DI principles...

[nicolas-grekas]

But we can reopen as a new feature if you think that's appropriate. I was closing mostly because this was labeled as a fix for a BC break, while the fix belongs only to maker-bundle.

[stof]

to me, re-opening that would help solving the issue in the Gedmo extensions (solving that in a clean way would require major changes in the extensions which might not be BC for users of the extensions using XML or Yaml mapping)

[nicolas-grekas]

Works for me

[ostrolucky]

I'm with @nicolas-grekas. We merge this, all libraries with their assumptions will need to be changed anyway and when we swap implementation again all of these libraries will need to be changed again? I don't like that.

We didn't do a BC break here, it's a problem of libraries going with naive solution and coding to implementation instead of contract, assuming implementation will never change. If they don't mind doing dirty, they can as well go with ReflectionClass and get the property themselves, no need for this change in doctrine-bundle which pretends everything is good after that and we totally won't swap this implementation in future again which will again break their naive ->getDriver() calls. These libraries were broken already, because they assume nobody out there has decorated drivers. Additionally, this is not a bug fix but new feature. We can as well think about cleaner solution/hook. Suggestions for that welcome, especially from SOLID folks, who like to discourage inheritance and hence break all of these instanceof checks.

[stof]

@ostrolucky I agree that the solution implemented in the Gedmo extensions 10 years ago was not clean (it should never have used the doctrine mapping file itself as a place to store the custom extended mapping, even though that made the extensions look like they are tightly integrated with the ORM mapping from a user point of view, and even though the XSD file of the ORM is specifically designed to allow such extensions). But without a way to inspect the driver and keep their guessing logic, the library would have to force a BC break on all their users to be able to load its mapping from somewhere else.

[Geekimo]

@stof You should take a look on @nicolas-grekas 's PR on Symfony Maker Bundle. It is really interesting and it's sometimes a good thing to force old libs to migrate to a cleaner code.

[stof]

@Geekimo the MakerBundle was fixed by inspecting the Symfony DI container. That's not an option for the Gedmo extensions, which have an EntityManager instance in the event listener. And forcing to pass the mapping configuration explicitly to the listener would require breaking BC in the extensions, while the existing version would have to stay incompatible with maintained versions of DoctrineBundle.

[nicolas-grekas]

I think @stof has a point, merging this is pragmatic.
@Geekimo would you mind fixing the review comments please?

[Geekimo]

Of course @nicolas-grekas , this will be done today.

[ostrolucky]

I guess it's ok as temporary workaround, but merging this will discourage ecosystem to come up with proper fix. Hence, before we merge this, I want to hear a longterm plan for a fix which fixes this without having the current drawbacks, which is specifically general inability to recognize the original driver type for decorated drivers.

[Geekimo]

IMO 3rd party libraries that used to work with MappingDriverChain should stick to 2.2.4, then release a new version that fully support 2.3.0 and upcoming, if this part of the code wasn't supposed to be used like that.
Do we have some list of theses libs ? We currently have gedmo, and the maker bundle was fixed by @nicolas-grekas .

[ostrolucky]

I mean if we change implementation again, these projects will be broken again. We need to think for a future and try to avoid this by having some sort of official extension point for libraries like Gedmo. This might not involve doctrine-bundle, but maybe ORM. Still, I think there should be a plan formulated for this before we merge this.

[nicolas-grekas]

The MappingDriver class here is not internal, so we won't change it without a deprecation + new major. This means that libs that rely on instanceof MappingDriver will be OK. What can happen is that we change the way we wire the class. But in this case, there is nothing we can do here - it's lib's job to build a CI to ensure that the wiring invariants they rely on are really invariant...

[ostrolucky]

Well one way would be if getParentDriver() would be part of Driver interface in ORM. For default implementation it would of course return null. Maybe not optimal solution, but you can see it can be done. There are ways to solve this without us breaking ecosystem each time we change wiring...

[stof]

Well, if you want a long term solution for the Gedmo extensions, I guess you should contact the maintainers of the extensions to involve them.

[ostrolucky]

Well this isn't for Gedmo only, Gedmo is just one of the known affected ones. But ok. @lbotsch, @comfortablynumb, @gordonslondon, @everzet, @dbu thoughts?

[AkenRoberts]

To be candid, Doctrine Extensions (Gedmo) is on life support. There is little to no chance that any major overhauls will be made to its internals in the near future unless a magical maintainer shows up to help me. I don't have the bandwidth. ☹️

My goal right now would be to make sure Extensions keeps working, even if it's an ugly hack that doesn't address any fragility. It sounds like an instanceof MappingDriver check + this method or reflection would get the job done.

[dbu]

my suggestion would be: i would merge it as it sounds a useful temporary solution. and open an issue on gedmo (without the expectation that it gets addressed by AkenRoberts - your position on maintenance state is fair and sane) so that people hopefully are aware.

and mark the getDriver method as deprecated so that we remove them in the next major version.

[ostrolucky]

But what's the longterm fix?

[nicolas-grekas]

I think we don't need one. Doctrine-bundle will do its moves, we don't have to save ppl from themselves.
(note that I wouldn't deprecate the method, it's trivial to maintain it anyway.)
Let's merge as is, that's the most pragmatic to me :)

[dmaicher]

Fine from my side

[ostrolucky]

Does that mean this is not temporary solution but long term one that we need to support forever? That's the only conclusion if long term plan/solution without this method doesn't exist. So let's stop kidding ourselves and stop saying "this this temporary" and "they are shooting themselves in the foot" if consumers literally don't have any better option even on drawing board.

[nicolas-grekas]

Ppl already have the solution, see eg that PR I did in maker-bundle.
We're also never bound "long term", since we can always deprecate.
This means to me there is no need to think too much here, since everybody will keep being able to move freely in the end.

[ostrolucky]

As pointed out before, maker bundle solution works only in context of Symfony. Decoupled libraries like gedmo have no solution other than relying on method we add here.