#11586 Fix duplicated crontab 2>&1 expression #11587
Conversation
| $arguments = array_map('escapeshellarg', $arguments); | ||
| do { | ||
| $command = preg_replace('/\s?2>&1\s?\|/', ' |', $command, -1, $count); | ||
| } while ($count > 0); |
There was a problem hiding this comment.
You don't need a cycle to replace arbitrary amount of occurrences: https://regex101.com/r/kgZCj5/1
|
|
||
| $this->assertEquals( | ||
| $expectedCommand, | ||
| $commandRenderer->render('php -r %s 2>&1 2>&1 | grep %s', []) |
There was a problem hiding this comment.
No more than one tested method call per test method, please.
|
@orlangur reviewed and updated, used dataProvider for tests, and removed cycle for replacing. |
| ); | ||
| } | ||
|
|
||
| public function testRenderWithoutArguments() | ||
| public function testRenderDataProvider() |
There was a problem hiding this comment.
Don't start data provider name with test. Describe what it actually returns, like commandsDataProvider.
There was a problem hiding this comment.
My fault, I'm changing that
| $command = preg_replace('/\s?2>&1\s?\|/', ' |', $command, -1, $count); | ||
| } while ($count > 0); | ||
|
|
||
| $command = preg_replace('/(\s?2>&1)+\s?\|/', ' |', $command); | ||
| $command = preg_replace('/\s?\||$/', ' 2>&1$0', $command); |
There was a problem hiding this comment.
Please combine into just one preg_replace call.
| ); | ||
| $expectedCommand = "php -r %s 2>&1 | grep %s 2>&1"; | ||
| $expectedCommandArgs = "php -r " . escapeshellarg($testArgument) . " 2>&1 | grep " | ||
| . escapeshellarg($testArgument2) . " 2>&1"; |
There was a problem hiding this comment.
Avoid logic in tests. Looks like escapeshellarg does not do anything here.
|
@orlangur Updated according to the review, finally used this regexp: https://regex101.com/r/kgZCj5/2 |
adrian-martinez-interactiv4
force-pushed
the
FR#CRON-INSTALL-PARSE-STDERR
branch
from
b1a6acd to
7c54e6d
Compare
|
Oh, great 👍 Kudos for rewriting it as one clean commit. |
Description
Installing or removing crontab via command adds 2>&1 repeatedly, even for crontab entries not related with Magento, if they match a regular expression. See details in #11586.
Fixed Issues (if relevant)
Manual testing scenarios
As explained in #11586
Contribution checklist