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.

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

[DI] Allow setting any public non-initialized services #24418

Merged
merged 1 commit into from Oct 4, 2017

Conversation

Projects
None yet
9 participants
@nicolas-grekas
Member

nicolas-grekas commented Oct 4, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23311, #23772
License MIT
Doc PR -

I think we went a bit too far with this deprecation and that we should relax the rule: setting a pre-defined service is OK if it has not already been initialized.
This will allow setting them in test cases, as reported several times (see linked issues).

@sroze

sroze approved these changes Oct 4, 2017

Yes! 👍

@chalasr

chalasr approved these changes Oct 4, 2017

@stof

stof approved these changes Oct 4, 2017

@nicolas-grekas nicolas-grekas merged commit d314b1f into symfony:3.3 Oct 4, 2017

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

nicolas-grekas added a commit that referenced this pull request Oct 4, 2017

bug #24418 [DI] Allow setting any public non-initialized services (ni…
…colas-grekas)

This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Allow setting any public non-initialized services

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23311, #23772
| License       | MIT
| Doc PR        | -

I think we went a but too far with this deprecation and that we should relax the rule: setting a pre-defined service is OK *if* it has not already been initialized.
This will allow setting them in test cases, as reported several times (see linked issues).

Commits
-------

d314b1f [DI] Allow setting any public non-initialized services

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-set-uninited branch Oct 4, 2017

@fabpot fabpot referenced this pull request Oct 5, 2017

Merged

Release v3.3.10 #24452

@mpoiriert

This comment has been minimized.

Show comment
Hide comment
@mpoiriert

mpoiriert Mar 12, 2018

I it possible that I have the same kind of issue with a ContainerBuilder that is compiled ?

I need to compile the container builder since I am using the autowire feature and since the container is compiled I cannot set the service event if it didn't have been initialize.

Should this be change too ?

 public function set($id, $service)
 {
        $id = $this->normalizeId($id);

        if ($this->isCompiled() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
            // setting a synthetic service on a compiled container is alright
            throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a compiled container is not allowed.', $id));
        }

        unset($this->definitions[$id], $this->aliasDefinitions[$id], $this->removedIds[$id]);

        parent::set($id, $service);
    }

mpoiriert commented Mar 12, 2018

I it possible that I have the same kind of issue with a ContainerBuilder that is compiled ?

I need to compile the container builder since I am using the autowire feature and since the container is compiled I cannot set the service event if it didn't have been initialize.

Should this be change too ?

 public function set($id, $service)
 {
        $id = $this->normalizeId($id);

        if ($this->isCompiled() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
            // setting a synthetic service on a compiled container is alright
            throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a compiled container is not allowed.', $id));
        }

        unset($this->definitions[$id], $this->aliasDefinitions[$id], $this->removedIds[$id]);

        parent::set($id, $service);
    }
@glennpratt

This comment has been minimized.

Show comment
Hide comment
@glennpratt

glennpratt Mar 20, 2018

I'm here with the same question as @mpoiriert. I have some expensive dependencies to avoid in integration tests, but I can't change them without opting for a separate config or a public synthetic service.

glennpratt commented Mar 20, 2018

I'm here with the same question as @mpoiriert. I have some expensive dependencies to avoid in integration tests, but I can't change them without opting for a separate config or a public synthetic service.

@mpoiriert

This comment has been minimized.

Show comment
Hide comment
@mpoiriert

mpoiriert Mar 20, 2018

This what I end up doing if it can help (it's a hack but at least I was able to move forward.

            $container->compile();

            // If we set a service before the container is compile will lose it's definition and the compile will fail
            // https://github.com/symfony/symfony/pull/24418#issuecomment-372436045
            foreach($services as $id => $service) {
                // Than once the container is compile we cannot replace a service set by definition.
                // It's why we fake this by setting the synthetic flag to true
                $container->getDefinition($id)->setSynthetic(true);
                $container->set($id, $service);
            }

mpoiriert commented Mar 20, 2018

This what I end up doing if it can help (it's a hack but at least I was able to move forward.

            $container->compile();

            // If we set a service before the container is compile will lose it's definition and the compile will fail
            // https://github.com/symfony/symfony/pull/24418#issuecomment-372436045
            foreach($services as $id => $service) {
                // Than once the container is compile we cannot replace a service set by definition.
                // It's why we fake this by setting the synthetic flag to true
                $container->getDefinition($id)->setSynthetic(true);
                $container->set($id, $service);
            }
@glennpratt

This comment has been minimized.

Show comment
Hide comment
@glennpratt

glennpratt Mar 20, 2018

@mpoiriert thanks!

    public function testOnlyOverwriteService(string $id, $object)
    {
        if (method_exists( $this->container, 'getDefinition')) {
            $this->container->getDefinition($id)->setSynthetic(true);
        }
        $this->container->set($id, $object);
    }

This is the little test helper I wrote for this purpose, appears to work with a compiled container and a dumped container. This back door may not be intentional I suppose.

glennpratt commented Mar 20, 2018

@mpoiriert thanks!

    public function testOnlyOverwriteService(string $id, $object)
    {
        if (method_exists( $this->container, 'getDefinition')) {
            $this->container->getDefinition($id)->setSynthetic(true);
        }
        $this->container->set($id, $object);
    }

This is the little test helper I wrote for this purpose, appears to work with a compiled container and a dumped container. This back door may not be intentional I suppose.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 20, 2018

Member

Please don't comment on closed issues/PRs. Instead, open an issue, or a PR if you want to propose a patch.

Member

nicolas-grekas commented Mar 20, 2018

Please don't comment on closed issues/PRs. Instead, open an issue, or a PR if you want to propose a patch.

@dbu

This comment has been minimized.

Show comment
Hide comment
@dbu

dbu Mar 20, 2018

Contributor

to add to the problem solving: another option could be to set the services public in test only. see for example: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/4e7f5930611f24950bab6e936e958a01095bd39f/tests/Functional/DependencyInjection/ServiceTest.php#L66

Contributor

dbu commented Mar 20, 2018

to add to the problem solving: another option could be to set the services public in test only. see for example: https://github.com/FriendsOfSymfony/FOSHttpCacheBundle/blob/4e7f5930611f24950bab6e936e958a01095bd39f/tests/Functional/DependencyInjection/ServiceTest.php#L66

@mpoiriert

This comment has been minimized.

Show comment
Hide comment
@mpoiriert

mpoiriert Mar 20, 2018

I'll open another issue tonight

@dbu It does work even if the service is public (I have set all my service public to test that)

mpoiriert commented Mar 20, 2018

I'll open another issue tonight

@dbu It does work even if the service is public (I have set all my service public to test that)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 20, 2018

Member

See #26499 also (but please note that comments on closed issues are just lost comments.)

Member

nicolas-grekas commented Mar 20, 2018

See #26499 also (but please note that comments on closed issues are just lost comments.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment