8000 [DoctrineBridge] [DX] Update exception text in ManagerRegistry to avoid confusion. by Simperfit · Pull Request #31047 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DoctrineBridge] [DX] Update exception text in ManagerRegistry to avoid confusion. #31047

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
Merged
merged 1 commit into from
Apr 15, 2019
Merged

[DoctrineBridge] [DX] Update exception text in ManagerRegistry to avoid confusion. #31047

merged 1 commit into from
Apr 15, 2019

Conversation

Simperfit
Copy link
Contributor
Q A
Branch? master
Bug fix? no
New feature? yesish
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29659
License MIT
Doc PR

Since the last PR was closed and the ticket is still open, taking it since it was already done by Nicolas in the comments.

@carsonbot carsonbot added Status: Needs Review DoctrineBridge DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Apr 10, 2019
@Simperfit
Copy link
Contributor Author

Failure is not related

jewome62
jewome62 suggested changes Apr 10, 2019
View reviewed changes
@Simperfit Simperfit force-pushed the dx/avoid-confusion-in-manager-registry branch from 54cc1b6 to ad4e2b4 Compare April 10, 2019 09:04
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 10, 2019
@Simperfit
Copy link
Contributor Author

Status: Needs Review

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.2 April 15, 2019 09:42
@nicolas-grekas nicolas-grekas force-pushed the dx/avoid-confusion-in-manager-registry branch from ad4e2b4 to 9ade232 Compare April 15, 2019 09:42
@nicolas-grekas
Copy link
Member

Thank you @Simperfit.

@nicolas-grekas nicolas-grekas merged commit 9ade232 into symfony:4.2 Apr 15, 2019
nicolas-grekas added a commit that referenced this pull request Apr 15, 2019
@nicolas-grekas
minor #31047 [DoctrineBridge] [DX] Update exception text in ManagerRe…
76647a7 
…gistry to avoid confusion. (Simperfit)

This PR was submitted for the master branch but it was merged into the 4.2 branch instead (closes #31047).

Discussion
----------

[DoctrineBridge] [DX] Update exception text in ManagerRegistry to avoid confusion.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yesish <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29659   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        |

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Since the last PR was closed and the ticket is still open, taking it since it was already done by Nicolas in the comments.

Commits
-------

9ade232 [DoctrineBridge] [DX] Update exception text in ManagerRegistry::resetService to avoid confusion.
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Apr 15, 2019
@Simperfit Simperfit deleted the dx/avoid-confusion-in-manager-registry branch April 16, 2019 06:24
@guilliamxavier
Copy link
Contributor

@nicolas-grekas: Sorry but that just made the error message even more misleading in my case:

Resetting a non-lazy manager service is not supported. Declare the "doctrine.orm.default_entity_manager" service as lazy.

but it is already lazy, says my bin/console debug:container doctrine.orm.default_entity_manager. The actual thing to do is composer require symfony/proxy-manager-bridge (then there is no error anymore), but that information is no longer in the error message.

The problem is that ProxyManager\Proxy\LazyLoadingInterface is defined not by symfony/proxy-manager-bridge but by ocramius/proxy-manager, and I have ocramius/proxy-manager installed (because required by doctrine/migrations required by doctrine/doctrine-migrations-bundle required by symfony/orm-pack required in my composer.json) without having symfony/proxy-manager-bridge installed, so the interface exists but it is not sufficient.

Couldn't the code check that symfony/proxy-manager-bridge is actually installed (given it also requires ocramius/proxy-manager)?

@nicolas-grekas
Copy link
Member

Nice suggestion, PR welcome :)

@guilliamxavier
Copy link
Contributor

@nicolas-grekas: I'm not sure how to do. Should I replace interface_exists(LazyLoadingInterface::class) with class_exists(RuntimeInstantiator::class) (adding a use Symfony\Bridge\ProxyManager\LazyProxy\Instantiator\RuntimeInstantiator;)? Or maybe keep the first and append the second with &&? Or another approach entirely?
Also, PR on branch 4.3?

@nicolas-grekas
Copy link
Member

Please open an issue or a PR so that we can keep track of the discussion.

@guilliamxavier guilliamxavier mentioned this pull request Jan 20, 2020
Merged
nicolas-grekas added a commit that referenced this pull request Jan 23, 2020
@nicolas-grekas
minor #35396 [DoctrineBridge] [DX] Improve condition for exception te…
a1e105e 
…xt in ManagerRegistry to avoid confusion (guilliamxavier)

This PR was submitted for the 4.4 branch but it was merged into the 4.3 branch instead.

Discussion
----------

[DoctrineBridge] [DX] Improve condition for exception text in ManagerRegistry to avoid confusion

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yesish
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #31047 (comment)
| License       | MIT
| Doc PR        | -

(Targetting 4.4 because 4.3 is already almost EOM)

Commits
-------

0d47fdf [DoctrineBridge] [DX] Improve condition for exception text in ManagerRegistry to avoid confusion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jewome62 jewome62 jewome62 requested changes

Assignees
No one assigned
Labels
Bug DoctrineBridge DX DX = Developer eXperience (anything that improves the experience of using Symfony) Status: Needs Review
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@Simperfit @nicolas-grekas @guilliamxavier @jewome62 @carsonbot
0