Skip to content
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

[9.x] Exactly match scheduled command --name in schedule:test #41528

Merged
merged 3 commits into from Mar 18, 2022
Merged

[9.x] Exactly match scheduled command --name in schedule:test #41528

merged 3 commits into from Mar 18, 2022

Conversation

ziadoz
Copy link
Contributor

@ziadoz ziadoz commented Mar 17, 2022

I've stripped the PHP and Artisan binary paths off of the command names here. This means instead of looking at the end of the string, which could accidentally match the wrong command, an exact match can be done.

@taylorotwell
Copy link
Member

Can you provide further explanation of scenarios where this improves things and before / after examples? Mark as ready for review when you have done so please.

@taylorotwell taylorotwell marked this pull request as draft March 17, 2022 18:45
@ziadoz
Copy link
Contributor Author

ziadoz commented Mar 17, 2022

Sure, I can explain.

I used Str::endsWith() when I added --name to this command because I hadn't realised I could remove the PHP and Artisan binaries from the command name strings. With that gone an exact match on the command name is possible.

So before if you had a command named cmd:apple then --name=e would match it because it ends in e. After this PR --name=e will match nothing, and you'll have to explicitly do --name=cmd:apple instead.

@ziadoz ziadoz marked this pull request as ready for review March 17, 2022 21:46
@taylorotwell taylorotwell merged commit 2428d1d into laravel:9.x Mar 18, 2022
@ziadoz ziadoz deleted the schedule-test-name-exact-match branch March 18, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants