Changed taskExec to use symfony process placeholders

[Alenore]

Overview

This pull request:

• Fixes a bug
• Adds a feature
• Breaks backwards compatibility
• Has tests that cover changes
• Adds or fixes documentation

Summary

Changed taskExec to use symfony process placeholders

Description

taskExec still used some deprecated functions that should have been deleted in Robo 2.0.
Since it didn't, and was throwing E_USER_DEPRECATED when trying to escape some specifics args/options, I've switched the feature to instead use placeholders and let Symfony Process handle the escaping itself.

Please note that :

• It now escaped arguments, simple or not : a is escaped to 'a', so it's technically not a perfect replacement. However, Symfony Process handles these use case itself, and probably already replaced them anyway even if Robo didn't. Some tests have been dropped as a result, since there's no longer a "no escape" case ;
• I've had to copy two functions from Symfony Process : mostly the escapeArgument(), and replacePlaceholders(). The former is still used in other part of the core (notably taskOpenBrowser() and git commits), while the latter is used for the getCommandDescription() to rebuild the executed command to output in the console, and in tests ;
• I've chosen to generate a hash of the arg using md5, since it's quick, will handle duplicate argument values, and is very unlikely to generate duplicates for strings the length of an argument ;
• While running the test suite on Windows, I've had to fix the environment variable set test case : env command isn't available on Win, so the test was always failing ;
• I've updated most comments mentioning the old arguments escaping behaviour, but I may have missed some.

Feel free to ask if you need any clarifications.

[greg-1-anderson]

What are the b/c breaks here? If variations in argument escaping are not considered a b/c break (as long as the same taskExec calls run the same way), then are there any b/c breaks?

[Alenore]

There's no breaking change per se on the execution level, but there is to the format of the commands themselves: if someone implemented a custom check of the command (for instance via unit testing, which I had to update in this case), it'll break.

[Alenore]

You mentionned in #1023 that this PR didn't fix that specific issue, however the whole point of that PR was to delegate the arguments escaping to Process, while still providing a way to escape arguments the same way Process does if people still want to escape it beforehand.
To be clear, #1023 is fixed with these changes, it was actually the first case I've tested against.

[greg-1-anderson]

Yes, you're right, taskExec is fixed, but the escape method needs to remain because it's public, and other code might be calling it directly. You'll need to rebase to get the change that removed the call to trigger error.

[Alenore]

That method was updated with the latest from Process, so there's still a public static ProcessUtils::escapeArgument() method.
Since the method has been updated, I cannot just include the other PR changes. Besides, escapeArgument() now has a reason to exist and should not be removed at all, since it's used to recompose the command description.

[greg-1-anderson]

You're right, I was confused by the diff. I am also not used to the new GitHub feature where I have to approve the tests to run. I'm going to do that now.

[greg-1-anderson]

OK this should be about ready to be merged now; I just want to give it one more review to make sure I didn't miss anything else or make any other mistakes.

[Alenore]

The escape() function was incorrectly set as non static, just fixed that.

[greg-1-anderson]

Since escapeArgument is only used for display purposes in taskExec, maybe we should retain the existing implementation. The new version might have some benefits for Windows, but since it will only be used by code that calls escape directly, it would make an odd edge case if the changed version caused problems.

[greg-1-anderson]

I'm undecided about this; if we don't update the implementation, then the printed string won't exactly match the string that Symfony is actually exec'ing, which could be odd. Then again, you could also get into that situation if you ended up with an older version of Symfony that has a different implementation of escape. I'm not sure if our Composer version constraints on our dependencies allows that, though.

[Alenore]

Symfony Process is set at 4.4.11 min, and escapeArgument has been changed to this implementation since at least 4.1.7.
Besides, that method was set as deprecated and meant to be removed in Robo 2.0, with a E_USER_DEPRECATED triggered. I don't think anybody would be really surprised it finally gets changed.

[greg-1-anderson]

Now I am wondering if this PR is in fact breaking, since you have changed the contract on getCommand(). This isn't just relevant for unit tests; Robo uses getCommand() for a number of things, and if this method starts returning a string that contains placeholder instead of a string with escaped arguments, then code that called getCommand and passed it to some process execution function would fail.

Would it work out to make getCommandDescription() continue to call / return getCommand(), have getCommand() call replacePlaceholders, and add a new method getCommandTemplate() that returns the command string with placeholders, and use that in Task\Exec::run()?

[Alenore]

So I've just tested outside of taskExec, and it does break on other commands : most command include a custom run() function, so placeholders don't get replaced.
Something I've noticed on other commands is that getCommand() is often used to display the command to be executed, when getCommandDescription() should be used instead.

In any case, there's more than Exec::run() that needs replacing, but every run() method that uses the ExecOnCommand trait, since they all get their arguments from CommandArguments and need to have their placeholders replaced.
As far as I know, only Exec has a custom process creation, every other calling the ExecCommand::executeCommand() method. It would simply be a matter of overwriting it in an ExecOneCommand::executeCommand() method that would handle placeholders.

So to sum things up :

• Add a getCommandTemplate() method ;
• Change ExecOneCommand::executeOneCommand() to replace placeholders.

Sounds good?

[greg-1-anderson]

Yes, that sounds good.