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] fix reporting bindings on overriden services as unused #29597

Merged
merged 1 commit into from Dec 24, 2018

Conversation

Projects
None yet
4 participants
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 13, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28326
License MIT
Doc PR -
@GuilhemN

This comment has been minimized.

Copy link
Contributor

GuilhemN commented Dec 15, 2018

I'm afraid this won't work in most cases: we deep copy bindings in the loaders (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L523). So instead we need to store bindings id and check against this list in the pass.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-fix-bind branch from 5405676 to e07ad2b Dec 17, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 17, 2018

@GuilhemN thanks for the review! Updated.

@GuilhemN
Copy link
Contributor

GuilhemN left a comment

Nice improvement 👍 thank you!

@nicolas-grekas nicolas-grekas merged commit e07ad2b into symfony:3.4 Dec 24, 2018

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 Dec 24, 2018

bug #29597 [DI] fix reporting bindings on overriden services as unuse…
…d (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] fix reporting bindings on overriden services as unused

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

Commits
-------

e07ad2b [DI] fix reporting bindings on overriden services as unused
@przemyslaw-bogusz

This comment has been minimized.

Copy link

przemyslaw-bogusz commented Dec 30, 2018

If I am not mistaken, this fix has created a new problem in ResolveBindingsPass.

The newly added puts all bindings that are set within a file into the usedBindings array. Before that, usedBindings array was empty as set in the property's declaration.

$this->usedBindings = $container->getRemovedBindingIds();

This means that aftewards, when processValue is called, the unusedBindings array remains empty, because the following condition is not met.
} elseif (!isset($this->usedBindings[$bindingId])) {
$this->unusedBindings[$bindingId] = array($key, $this->currentId);
}

As a result, InvalidArgumentException (informing about an unused binding) is not thrown, even if a binding is actually not used. I have come upon this, because I was just about to make a PR solving #27828, but after I fetched the symfony code to make sure it is up to date my fix stopped working. I believe I can find a new solution, solving both problems.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 30, 2018

The current code prefers a false negative to the previous false positive.
Being more accurate could mean resolving bindings in the loaders, where there is full context about them.
Or maybe you have another idea. I'd be happy to review it!

This was referenced Jan 6, 2019

mmarynich added a commit to mmarynich/symfony that referenced this pull request Jan 11, 2019

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-fix-bind branch Jan 11, 2019

mmarynich added a commit to mmarynich/symfony that referenced this pull request Jan 11, 2019

Revert "bug symfony#29597 [DI] fix reporting bindings on overriden se…
…rvices as unused (nicolas-grekas)"

This reverts commit 44e9a91, reversing
changes made to 91b28ff.

fabpot added a commit that referenced this pull request Jan 13, 2019

bug #29853 Revert "bug #29597 [DI] fix reporting bindings on override…
…n services as unused" (mmarynich)

This PR was merged into the 3.4 branch.

Discussion
----------

Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused"

This reverts commit 44e9a91, reversing
changes made to 91b28ff.

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

4.2.2 release changed the way tagged service are injected

As asked by @nicolas-grekas #29836 (comment)

Commits
-------

b3e17d2 Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused (nicolas-grekas)"

xabbuh added a commit that referenced this pull request Jan 14, 2019

Merge branch '3.4' into 4.1
* 3.4:
  fix compatibility with Twig >= 2.6.1
  [Form] SA fix
  fix compatibility with PHPUnit 4.8
  remove return type hint for PHP 5 compatibility
  Component CssSelector tests
  [DebugClassLoader] Readd findFile() method
  [Console] Fix composer.json suggest/provide
  Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused (nicolas-grekas)"
  Fixed exception wording
  Fix SwiftMailerHandler to support Monolog's latest reset functionality

fabpot added a commit that referenced this pull request Jan 14, 2019

Merge branch '4.1' into 4.2
* 4.1:
  bump required Twig version
  fix compatibility with Twig >= 2.6.1
  [Form] SA fix
  fix compatibility with PHPUnit 4.8
  remove return type hint for PHP 5 compatibility
  SCA: minor code tweaks
  Component CssSelector tests
  [DebugClassLoader] Readd findFile() method
  [Console] Fix composer.json suggest/provide
  Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused (nicolas-grekas)"
  Fixed exception wording
  Fix SwiftMailerHandler to support Monolog's latest reset functionality

fabpot added a commit that referenced this pull request Jan 14, 2019

Merge branch '4.2'
* 4.2:
  bump required Twig version
  fix compatibility with Twig >= 2.6.1
  [Form] SA fix
  fix compatibility with PHPUnit 4.8
  remove return type hint for PHP 5 compatibility
  SCA: minor code tweaks
  Component CssSelector tests
  [DebugClassLoader] Readd findFile() method
  [Console] Fix composer.json suggest/provide
  Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused (nicolas-grekas)"
  Fixed exception wording
  Fix SwiftMailerHandler to support Monolog's latest reset functionality

xabbuh added a commit to xabbuh/symfony that referenced this pull request Jan 14, 2019

fix test after revert of bugfix
In symfony#29853 the bugfix made in symfony#29597 was reverted as it did not work as
expected. This fixture file has been modified after the 3.4 branch was
merged up to account for the changes made in symfony#2957 and must now be
reverted to the former state too.

fabpot added a commit that referenced this pull request Jan 15, 2019

minor #29886 [DependencyInjection] fix test after revert of bugfix (x…
…abbuh)

This PR was merged into the 4.2 branch.

Discussion
----------

[DependencyInjection] fix test after revert of bugfix

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

In #29853 the bugfix made in #29597 was reverted as it did not work as
expected. This fixture file has been modified after the 3.4 branch was
merged up to account for the changes made in #2957 and must now be
reverted to the former state too.

Commits
-------

81f63b1 fix test after revert of bugfix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment