8000 [Bridge/Doctrine] Reset the EM lazy-proxy instead of the EM service by nicolas-grekas · Pull Request #19203 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Bridge/Doctrine] Reset the EM lazy-proxy instead of the EM service #19203

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
Jul 1, 2016
Merged

[Bridge/Doctrine] Reset the EM lazy-proxy instead of the EM service #19203

merged 1 commit into from
Jul 1, 2016

Conversation

nicolas-grekas
Copy link
Member
@nicolas-grekas nicolas-grekas commented Jun 28, 2016 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? no
Fixed tickets -
License MIT
Doc PR -

This makes the entity manager resettable by resetting its proxy, which should be more robust than resetting its service.
See first comments in #19192
Ping @stof

@nicolas-grekas nicolas-grekas mentioned this pull request Jun 28, 2016
Closed
stof
stof reviewed Jun 28, 2016
View reviewed changes
src/Symfony/Bridge/Doctrine/ManagerRegistry.php
}
$manager->setProxyInitializer(
function (&$wrappedInstance, LazyLoadingInterface $manager) use ($name) {
$this->container->set($name, null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not solve the issue, as it is what will be deprecated

@stof
Copy link
Member
stof commented Jun 28, 2016

Status: needs work

@nicolas-grekas
Copy link
Member Author

Updated
Status: needs review

@nicolas-grekas nicolas-grekas force-pushed the lazy-reset branch 2 times, most recently from 358a6bf to 6bfdb82 Compare June 28, 2016 16:34
GuilhemN
GuilhemN reviewed Jun 28, 2016
View reviewed changes
src/Symfony/Bridge/Doctrine/ManagerRegistry.php
if (isset($this->aliases[$name = strtolower($name)])) {
$name = $this->aliases[$name];
}
$method = 'get'.strtr($name, $this->underscoreMap).'Service';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it possible instead to access the container methods map ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW i guess we should somehow deprecate using another container than the one provided by symfony ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method maps is now used also. We already are enforcing a symfony container, see type hint on ContainerAwareTrait::setContainer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course but the type hint is targeting the interface, and we can't know its internals.

@nicolas-grekas nicolas-grekas force-pushed the lazy-reset branch 2 times, most recently from 2efb9d2 to 0315985 Compare June 29, 2016 07:43
@nicolas-grekas
8000 Copy link
Member Author
nicolas-grekas commented Jun 29, 2016 •
edited
Loading

Implementation details & discussion following @Ener-Getick comments:

the $container property is of type ContainerInterface but this interface doesn't provide any mean to get access to proxy initializers used for lazy services instanciation. My stand in this PR is that it is fine in this very case to get over the public interfaces and use the protected properties directly: the bridge is a Symfony managed package so that we have all the required tests & ci to ensure we won't break anything in the long run.
As far as custom container implementations are used into the wild, they should still be supported if they use the protected properties as expected. If this really exists (a custom incompatible container that generates LazyProxyInterfaces services), we can think later of new interfaces to allow what we're doing here but using public methods.

Yet, I think it's just fine to do direct access. Let's be pragmatic.

@stof
Copy link
Member
stof commented Jun 29, 2016

@nicolas-grekas would be great to debug CI failures

@nicolas-grekas
Copy link
Member Author

@stof see #19217 (btw, votes welcomed on both this and that PRs ;) )

@fabpot
Copy link
Member
fabpot commented Jul 1, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit c581cd4 into symfony:master Jul 1, 2016
fabpot added a commit that referenced this pull request Jul 1, 2016
@fabpot
feature #19203 [Bridge/Doctrine] Reset the EM lazy-proxy instead of t…
62f0dbf 
…he EM service (nicolas-grekas)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Bridge/Doctrine] Reset the EM lazy-proxy instead of the EM service

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | no
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This makes the entity manager resettable by resetting its proxy, which should be more robust than resetting its service.
See first comments in #19192
Ping @stof

Commits
-------

c581cd4 [Bridge/Doctrine] Reset the EM lazy-proxy instead of the EM service
@stof
Copy link
Member
stof commented Jul 13, 2016

@nicolas-grekas you forgot the second half of this task: opening a PR on DoctrineBundle to mark the EM services as lazy so that a proxy is generated for them.

@stof stof mentioned this pull request Jul 13, 2016
Open
@fprochazka fprochazka mentioned this pull request Jul 15, 2016
Open
@nicolas-grekas nicolas-grekas deleted the lazy-reset branch July 18, 2016 05:35
@nicolas-grekas nicolas-grekas mentioned this pull request Jul 24, 2016
Merged
@ro0NL ro0NL mentioned this pull request Aug 19, 2016
Closed
@fabpot fabpot mentioned this pull request Oct 27, 2016
Merged
@gregoirepaqueron
Copy link

How to set entity managers lazy by default doctrine configuration ?

My conf :

doctrine:
    orm:
        auto_generate_proxy_classes: "%kernel.debug%"
        default_entity_manager: default
        entity_managers:
            sso:
                connection: sso
                mappings:
                    GsoiJNumAdminMainBundle:
                        type:     "annotation"
                        dir:      "Entity/SSO"
                        prefix:   "MyPrefix\\Entity\\SSO"

When i want to reset the manager, i got the deprecation message :
User Deprecated: Resetting a non-lazy manager service is deprecated since Symfony 3.2 and will throw an exception in version 4.0. Set the "doctrine.orm.sso_entity_manager" service as lazy and require "symfony/proxy-manager-bridge" in your composer.json file instead.

with this code :

$this->container->get('doctrine')->resetManager('sso');

@stof
Copy link
Member
stof commented Apr 13, 2017

install ocramius/proxy_manager so that a proxy can be generated to actually make the service lazy (requiring the bridge itself won't do the trick when using the fullstack repo as it replaces the bridge but does not enforce installing the library).
DoctrineBundle already marks the service as lazy (if you use the latest version of the bundleà

@gregoirepaqueron
Copy link

Thank you @stof for your explanation!

@software-artificer software-artificer mentioned this pull request May 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Deprecation Feature Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nicolas-grekas @stof @fabpot @gregoirepaqueron @GuilhemN @carsonbot
0