[Finder] Command::addAtIndex() fails with Command instance argument #14385
Conversation
…P error when Command instance is passed
| public function testShellCommand() | ||
| { | ||
| $cmd = Command::create()->add('--force'); | ||
| $this->assertSame('--force', $cmd->join()); |
There was a problem hiding this comment.
This is not related to the issue, is it?
There was a problem hiding this comment.
It's just a sanity check before checking problem case. Command class is not tested anywhere directly, so I thought it would be a good idea.
There was a problem hiding this comment.
Maybe it's a better idea to split this into more than one test then.
By the way, it could be interesting to use index 1 instead of 2 in the second call of addAtIndex() to ensure that you can also insert in the middle.
There was a problem hiding this comment.
@xabbuh How about introducing separate Shell\CommandTest.php suite to cover Command class then? I'd be happy to work on it as it's one of the least covered classes in a whole component.
There was a problem hiding this comment.
@xabbuh Can you reply to my comment? I'd be happy to refactor the tests, but I need guidance on what effect are you interested in.
There was a problem hiding this comment.
@thunderer A dedicated CommandTest class sounds like a good idea to me.
| $cmd->add('--force'); | ||
| $cmd->add('--run'); | ||
|
|
||
| $this->assertSame('--force --run', (string)$cmd); |
There was a problem hiding this comment.
missing space: (string) $cmd
There was a problem hiding this comment.
Thanks, I'll fix this in one batch when @xabbuh gets back to me with his thoughts. :)
There was a problem hiding this comment.
Sorry, I missed this. Yes, please change as suggested. :)
|
@xabbuh Can you comment on the current state of this PR? |
|
👍 Can you rebase and fix CS issues? |
| @@ -13,6 +13,7 @@ | |||
|
|
|||
| use Symfony\Component\Finder\Finder; | |||
| use Symfony\Component\Finder\Adapter; | |||
| use Symfony\Component\Finder\Shell\Command; | |||
|
ping @thunderer |
Apache rewrite module renames client request header (`HTTP_`) by prepending `REDIRECT_` to it. http basic authentication and http digest authentication are properly processed in REDIRECT_ form, while bearer is processed in HTTP_ form, but dropped in REDIRECT_ form.
There is no translation writer format named 'xliff', but 'xlf' only. So the TranslationUpdateCommand can't be called with 'output-format' == 'xliff' and the version info will never be shown.
Using https for symfony.com/search stops chrome (and eventually firefox) from warning us about "Mixed Content" when developing sites that use SSL for the entire site.
…it has an empty value
The `SwitchUserEvent` is triggered in case an account is switched. This works okay while switching to the user, but on exit the `SwitchUserEvent` is triggered again with the original User. That User was not initialized by the provider yet. load user by UserInterface instead of username
In AZ, as in TR, pluralization is always 0: 0 kitab (zero books) 1 kitab (1 book) 3 kitab (3 books) 104 kitab (104 books) Apparently ZF ruleset was wrong in the first place :)
|
@thunderer althought it's not me who's gonna be merging it, I expect it might be hard. Are you sure you rebased with the latest 2.3? In the worst case create a new PR. |
… argument (thunderer) This PR was squashed before being merged into the 2.3 branch (closes #15223). Discussion ---------- [Finder] Command::addAtIndex() fails with Command instance argument | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14384 | License | MIT | Doc PR | - Fixed reported bug #14384 and added test case for it. This is a second PR as previous #14385 went bad after I failed to rebase 2.3 branch properly. Commits ------- 2aff566 [Finder] Command::addAtIndex() fails with Command instance argument
Fixed reported bug #14384 and added test case for it.