Skip to content

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

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

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

merged 1 commit into from
Dec 24, 2018

Conversation

nicolas-grekas
Copy link
Member

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 -

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 13, 2018
@nicolas-grekas nicolas-grekas mentioned this pull request Dec 13, 2018
Closed
@GuilhemN
Copy link
Contributor

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
Copy link
Member Author

@GuilhemN thanks for the review! Updated.

GuilhemN
GuilhemN approved these changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@GuilhemN GuilhemN left a comment

Choose a reason for hiding this comment

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

Nice improvement 👍 thank you!

@nicolas-grekas nicolas-grekas merged commit e07ad2b into symfony:3.4 Dec 24, 2018
nicolas-grekas added a commit that referenced this pull request Dec 24, 2018
@nicolas-grekas
bug #29597 [DI] fix reporting bindings on overriden services as unuse…
44e9a91 
…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
Copy link
Contributor

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.

symfony/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php

Line 37 in 1d3ce9b

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

This means that aftewards, when processValue is called, the unusedBindings array remains empty, because the following condition is not met.

symfony/src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php

Lines 84 to 86 in 1d3ce9b

} 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
Copy link
Member Author

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
Merged
Merged
Merged
mmarynich added a commit to mmarynich/symfony that referenced this pull request Jan 11, 2019
@mmarynich
Revert "bug symfony#29597 [DI] fix reporting bindings on overriden se…
05388c0 
…rvices as unused"

This reverts commit e07ad2b
@nicolas-grekas nicolas-grekas deleted the di-fix-bind branch January 11, 2019 15:35
mmarynich added a commit to mmarynich/symfony that referenced this pull request Jan 11, 2019
@mmarynich
Revert "bug symfony#29597 [DI] fix reporting bindings on overriden se…
b3e17d2 
…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
@fabpot
bug #29853 Revert "bug #29597 [DI] fix reporting bindings on override…
ea68e28 
…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
@xabbuh
Merge branch '3.4' into 4.1
00b73da 
* 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
@fabpot
Merge branch '4.1' into 4.2
6d37740 
* 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
@fabpot
Merge branch '4.2'
508602d 
* 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
@xabbuh
fix test after revert of bugfix
81f63b1 
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.
@xabbuh xabbuh mentioned this pull request Jan 14, 2019
Merged
fabpot added a commit that referenced this pull request Jan 15, 2019
@fabpot
minor #29886 [DependencyInjection] fix test after revert of bugfix (x…
18bb3e1 
…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
@xabbuh xabbuh mentioned this pull request Jan 16, 2019
Closed
This was referenced Jan 29, 2019
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuilhemN GuilhemN GuilhemN approved these changes

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolas-grekas @GuilhemN @przemyslaw-bogusz @carsonbot