[DoctrineBridge] Integrate doctrine/deprecations #41408
Conversation
ostrolucky
force-pushed
the
doctrine-deprecations
branch
from
fb9f9c2
to
ae174a6
Compare
|
This won't automatically solve it for all doctrine libraries. |
|
That's not the intention and wouldn't be wanted by users either. Intention is provide integration for bundles. Bundles shouldn't have to integrate this separately by copy & pasting it to each bundle, that's what we have bridge here for. |
|
@ostrolucky but FrameworkBundle should not have to depend on the issue is that we have one package named |
There was a problem hiding this comment.
I'm against this change. The bridge is a legacy thing that should be moved to the doctrine namespace. In the end, it's our own bundle architecture we're abstracting here, which has no place in the Symfony repository.
In my opinion, it's up to end users to think about how they want to collect various deprecations. While Symfony itself uses trigger_deprecation, other libraries may have their own way of announcing these, and only the user knows what to do with the various deprecations.
That said, since each bundle has to make a decision on how to handle this (ODM is not using doctrine/deprecations and most likely won't ever) we shouldn't put this change in the bridge. Users of the DoctrineBundle will have this configured automatically. Users of Symfony that use annotations or any other library explicitly should configure the Doctrine Deprecations library themselves (e.g. if they're only using the doctrine/annotations package). If they use doctrine packages implicitly, they don't need to do anything since they won't be interested in the deprecations anyways. That covers it from a Symfony perspective, so there's no need for this change.
|
Thanks for opening the PR so that we can have this conversation. If I understand @stof's objections, a way to account for them would be to move this to FrameworkBundle. BUT now that I read @alcaeus' reasoning, I think we might better decide to NOT do anything, neither in this repository, NOR in DoctrineBundle. The doctrine/deprecation library defaults to being a no-op, and explains that if ppl want to have to have some reports about the deprecations, they should enable it. This is just fine! |
BTW, IF we're looking for a place to configure the global state managed by the lib, the only place where this would make sense is in the runtime component to me. |
|
@nicolas-grekas either that, or in a recipe for the |
It doesn't have to. It's one line of code, you don't need to require bridge for that. I've put it in bridge because bridge is what you use if you need doctrine-bundle or odm bundle, which is most symfony framework users. And that's also when you use most doctrine packages. But this change might not make sense for users if you don't use those bundles. I for one wouldn't enable this mode if I use doctrine/deprecations as the only doctrine package.
They won't. I have created this PR in order for this to be done. If it's rejected in name of coupling, same reason will be used for not integrating it in doctrine-bundle. You know, whole "why should frameworkbundle require doctrine-bundle if you use doctrine/annotations only" thing.
I've never seen this view point acknowledged by Symfony maintainers. If that's right, issue should be created here so getting rid of doctrine-bridge to-do is visible and acknowledged.
Yes it is, my patch is just providing default which makes sense in context of doctrine bundles. User can always change it in their own bootstrap code. Whole consistency is paramount thing. Symfony framework users expect deprecations are visible in profiler out of the box. Btw may I remind I was repeatedly asked to provide this integration in doctrine-bundle? And now suddenly it's too coupled even if it was in doctrine-bridge, which is more generic than doctrine-bundle. I'm fine with wherever you put it, if somewhere, but stop asking doctrine-bundle to do this. And next time let's have this conversation before creating a patch too. |
I'm not saying that this is too coupled. I'm saying that putting that into doctrine-bridge is not generic enough, because this bridge only covers a subset of doctrine libraries that will use doctrine/deprecations to trigger their deprecations (and so would not solve the issue if the project is using one of the no-covered libraries).
Well, the doctrine-bridge is not entirely legacy. Parts of it are (the abstract DI extension sharing some logic between DoctrineBundle and DoctrineMongodbODMBundle definitely is). Other parts (a form type integrating with doctrine/persistence for instance) totally belong to a symfony bridge. The cleaner architecture for those other bridges is that we should not have one doctrine-bridge bridging |
|
If doctrine-bridge isn't generic enough ( |
Users using doctrine/annotations generally rely on FrameworkBundle to provide them a configured reader. So I would say that they would also expect FrameworkBundle to configure the way the annotation reader reports deprecations. |
|
If all users expect Doctrine packages to show deprecations, why doesn't |
|
@wouterj all Symfony users expect it to be reported in the Symfony way (or at least that's our assumption). that's still not the same than "all users" |
|
My preference would be for doctrine/deprecation to embed a Symfony bundle and have it enabling deprecations for dev+test by default in a recipe. |
|
Thank you all for the discussion, I think we've settled on this topic. |
|
Current situation: Doctrine ORM users have to find out that Doctrine has its own deprecation triggering mechanism, and they have to add (minimal) code along their bootstrapping path to make |
This automatically enables deprecations triggered via doctrine/deprecations in standard symfony deprecations mode. Currently, symfony error-handler and phpunit-bridge are not aware of such deprecations with stock configuration of doctrine/deprecations. That might be a bug from user's POV. This patch fixes that.
We don't want this in a bundle, because all bundles using doctrine-bridge shouldn't have to reimplement this separately.