Cloning taskRsync does not work since 1.0.0

gabor-udvari commented 7 years ago

Steps to reproduce

The guide on the webpage states that you can clone an Rsync task to do a dryrun first and then a real run later, eg.:

$rsync = $this->taskRsync()
  ->fromPath('src/')
  ->toPath('example.com:/var/www/html/app/')
  ->archive()
  ->excludeVcs()
  ->progress()
  ->stats();

$dryRun = clone $rsync;
$dryRun->dryRun()->run();
if ('y' === $this->ask('Do you want to run (y/n)')) {
  $rsync->run();
}

Expected behavior

The above code does work up until 0.7.2, but since 1.0.0 it does not work anymore.

Actual behavior

Since 1.0.0 the cloning will not return a new object, but it is a reference to the original one, and the --dryrun attribute inserted by the dryRun() function will be inserted to the original object too. Also the target and source attribute inserted by the run() function will also remain, therefore the second run will double the target and source, and the resulting rsync command will fail:

[Remote\Rsync] Running rsync --archive --exclude .git --exclude .svn --exclude .hg --progress --stats --dry-run src/ 'example.com:/var/www/html/app/' src/ 'example.com:/var/www/html/app/'
Unexpected remote arg: 'example.com:/var/www/html/app/'

As a side note, the command will be printed out twice since 1.0.0, the Common\ExecTrait will print out the command if it is not set to silent, therefore the printing can be removed from Task\Remote\Rsync. But that is easily fixed, the difficult is how we can achieve the cloning and dry run in 1.0.0.

System Configuration

Ubuntu 16.04, PHP 7.1.5

greg-1-anderson commented 7 years ago

The clone is probably not working because taskRsync() is now returning a wrapper object (a collection builder) rather than the rsync object itself. The clone returns a new copy of the builder, but the new builder still has references to the original task objects.

Robo 1.x has a simulated mode that works for all tasks. It would be possible to put the builder into simulated mode, run it to get the message, prompt, and then take the builder out of simulated mode and run it again. While this would work, it would be fairly awkward to implement.

Instead, I think it would probably be preferable to add a confirm() method to the builder. Your usage pattern would then look like this:

$result = $this->taskRsync()
  ->fromPath('src/')
  ->toPath('example.com:/var/www/html/app/')
  ->archive()
  ->excludeVcs()
  ->progress()
  ->stats()
  ->confirm('Do you really want to do this?')
  ->run();

The behavior, then, would be that, at run() time, every task before the confirmed task (in this case the rsync is the confirmed task, and there are no tasks before it); then, the confirm operation would run the confirmed task in simulated mode, prompt, and then continue as usual if confirmed. If denied, the confirm method would throw a user cancelled exception.

The only other question, then, is what to do in non-interactive mode? Perhaps -n causes all confirm() messages to abort unless --yes is also specified.

PRs welcome and appreciated.

greg-1-anderson commented 7 years ago

This would be a good use for defer() once #580 is merged.

greg-1-anderson commented 7 years ago

Actually, this is a little more difficult than I first realized. The advantage of --simulate is that each task does not need to individually support simulation. The disadvantage, though, is that the system must know upfront, before the configure methods are called, that it is in simulation mode.

Maybe the builder could cache all of the configure calls for the current task, just in case that information was needed (e.g. for a confirm() method).

pniederlag commented 7 years ago

dry-run is a pretty well known feature specific to rsync, as it will give you a detailed list of files that would be synched.

IMHO it would be nice to keep this nice little feature as written in the documentation.

A general simulation feature imho is another great featuer though :->

greg-1-anderson commented 7 years ago

Tasks can override the behavior of --simulate; the rsync task could potentially use dry-run when printing its simulation summary.

I'd prefer a solution that is not rsync-specific.

dreerr commented 7 years ago

Any solution on how to deal with this issue yet?

greg-1-anderson commented 7 years ago

I have not been working on this. A pull request would be welcome.

antonmarin commented 6 years ago

taskExec. seems same problem

greg-1-anderson commented 6 years ago

Another possible interim solution would be to provide a "clone" helper method that recognized and handled the wrapper object.

antonmarin commented 6 years ago

Maybe just implement __clone() in CollectionBuilder?

vworldat commented 6 years ago

I also think adding __clone() is the way to go here. From what I can see this also needs to be implemented for Robo\Collection\Collection as well as all classes implementing Robo\Collection\WrappedTaskInterface. I think that way any tasks (not only the rsync one) may safely be cloned and behave as expected.

consolidation / robo