Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Remove useless checks in descriptors#21341
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
709aae7 to6205e1aCompare| $tableRows[] =array('Autowiring Types',$autowiringTypesInformation); | ||
| $autowiringTypes =$definition->getAutowiringTypes(); | ||
| if (count($autowiringTypes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As pointed out multiple times on some other PRs by@nicolas-grekas , thecount is useless and seems to be avoided thus in the Symfony codebase. Could you take advantage of this PR to change it here too?
Could also be inlined using a ternary without loosing readability here IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indded.
ogizanagi commentedJan 19, 2017
Status: Reviewed |
| if ($definition->getFactoryMethod(false)) { | ||
| $serviceXML->setAttribute('factory-method',$definition->getFactoryMethod(false)); | ||
| } | ||
| if ($definition->getFactoryMethod(false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's wrong. FactoryClass, FactoryService and FactoryMethod areremoved in 3.0. So this must still be conditional here (and the code should beremoved when dropping support for DI 2.8)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof We're in the 2.8 branch and it requiresonly Symfony 2.8 (it doesn't work with 3.0). It must indeed be dropped when merging in 3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh, we used to support 3.0.x in the past
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After double checking:
It should be as is in 2.8, conditional in 3.0 and 3.1 and finally dropped in 3.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you check when and why we dropped support for 3.0 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks like it has never been supported. The DI dependency was bumped to 2.8 (with no explicit support for 3.0) in73f0ee2 without obvious reasons and stayed as is since 2 years.
| $serviceXML->setAttribute('synchronized',$definition->isSynchronized(false) ?'true' :'false'); | ||
| } | ||
| $serviceXML->setAttribute('shared',$definition->isShared() ?'true' :'false'); | ||
| $serviceXML->setAttribute('synchronized',$definition->isSynchronized(false) ?'true' :'false'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same issue here.isSynchronized is removed in 3.0
| if (method_exists($definition,'isSynchronized')) { | ||
| $tableRows[] =array('Synchronized',$definition->isSynchronized(false) ?'yes' :'no'); | ||
| } | ||
| $tableRows[] =array('Synchronized',$definition->isSynchronized(false) ?'yes' :'no'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
broken here
| ."\n".'- Synthetic:'.($definition->isSynthetic() ?'yes' :'no') | ||
| ."\n".'- Lazy:'.($definition->isLazy() ?'yes' :'no') | ||
| ."\n".'- Shared:'.($definition->isShared() ?'yes' :'no') | ||
| ."\n".'- Synchronized:'.($definition->isSynchronized(false) ?'yes' :'no') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
broken here
| 'synthetic' =>$definition->isSynthetic(), | ||
| 'lazy' =>$definition->isLazy(), | ||
| 'shared' =>$definition->isShared(), | ||
| 'synchronized' =>$definition->isSynchronized(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
broken here
dunglas commentedJan 19, 2017
Removed some useless counts. |
| $data['tags'] =array(); | ||
| if (count($definition->getTags())) { | ||
| foreach ($definition->getTags()as$tagName =>$tagData) { | ||
| if ($tags =$definition->getTags()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thisif becomes useless too.
nicolas-grekas commentedJan 24, 2017
👍 |
fabpot commentedJan 24, 2017
Does it mean that@stof comments were wrong or fixed but Github does not show that? Sorry, I haven't re-read the PR. |
dunglas commentedJan 24, 2017
@fabpot yes but we need to be very careful when merging to other branches (I can do the merge if you want). |
fabpot commentedJan 24, 2017
@dunglas Ok, I Iet you merge this one and then merge to other branches, that will be much easier. |
…dunglas)This PR was merged into the 2.8 branch.Discussion----------[FrameworkBundle] Remove useless checks in descriptors| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aAs pointed out by@ogizanagi in#21315 (comment), some code in FrameworkBundle's descriptors is useless because the bundle works only with DI 2.8.Commits-------c759345 [FrameworkBundle] Remove useless checks in descriptors
As pointed out by@ogizanagi in#21315 (comment), some code in FrameworkBundle's descriptors is useless because the bundle works only with DI 2.8.