[DependencyInjection] Add support an integer return for default_index_method #40337
Conversation
maranqz
commented
Mar 1, 2021
| Q | A |
|---|---|
| Branch? | 5.x for features |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Tickets | #40319 |
| License | MIT |
| Doc PR | TODO |
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
Should I put notice about this feature in docs ? |
| } | ||
|
|
||
| return $defaultIndex; | ||
| return (string) $rm->invoke(null); |
There was a problem hiding this comment.
I would not unconditionally cast to string here. that could have weird effects with other things than integers that you want to support here.
There was a problem hiding this comment.
if (\is_int($defaultIndex)){ // or \is_numeric?
$defaultIndex = (string) $defaultIndex;
}
if (!\is_string($defaultIndex)) {
....Is that better?
src/Symfony/Component/DependencyInjection/Compiler/PriorityTaggedServiceTrait.php
Outdated
Show resolved
Hide resolved
|
I do not understand how I should fix 7.4 tests.
|
There was a problem hiding this comment.
Awesome!
Great feature, great implementation, great that you added both test and change log.
I like this. 👍
The test failures is not related to your PR so you can ignore them. Good thing that you checked.
I dont think this feature needs to be documented. It is almost as I think it should be a bugfix...
Let's see what some other maintainer think about adding docs or not.
|
Thank you @maranqz. |