Task assembly #284

greg-1-anderson · 2016-03-07T18:25:51Z

This PR introduces the concept of task assembly, which is done by way of the TaskAssembler, a helper object that helps do dependency injection on tasks that are created at runtime (as opposed to during container initialization, where the DI container can be used for this purpose).

The old way to build tasks looks like this:

    protected function taskExtract($filename)
    {
        return new Extract($filename);
    }

With task assembly, it now looks like this:

    protected function taskExtract($filename)
    {
        return $this->taskAssembler()->assemble(
            '\Robo\Task\Archive\Extract',
            [$filename]
        );
    }

The benefit of using the task assembler is threefold:

It will call setLogger & c. as necessary on the task, after it is constructed, reducing coupling between the different areas of the code.
It recognizes tasks that implement CompletionInterface, and ensures their complete() method will be called at shutdown time, if the task is run().
It also supports 'simulated' tasks, which print out what they would do when run() without actually making any persistent changes.

This PR does not break backwards compatibility with existing external Robo tasks. Existing tasks that continue to use the old builder pattern will still work; however, they will silently ignore the --simulate flag, and execute instead. To break backwards compatibility and force old tasks to update, all that needs to be done is remove one of the last references to the Config class, and then all task printing operations will fail for tasks that have not updated to the new mechanism.

… us to reduce internal coupling, and also support temporary and simulated tasks in a uniform way.

Nitpick-CI · 2016-03-07T18:25:58Z

src/Task/Development/loadTasks.php

@@ -36,7 +45,10 @@ protected function taskSemVer($pathToSemVer = '.semver')
     */
    protected function taskServer($port = 8000)
    {
-        return new PhpServer($port);
+        return $this->taskAssembler()->assemble(


Line indented incorrectly; expected at least 12 spaces, found 8

I don't see what's wrong about the formatting of this line. Looks the same as all of the others like it. No tabs that I can see. Odd.

greg-1-anderson · 2016-03-07T19:52:06Z

The accessor methods added here were showing up as commands, so I added a filter to automatically remove accessors from command lists. It would also be possible to add an annotation to opt-out of command selection.

greg-1-anderson · 2016-03-09T05:00:36Z

It also seems that we could replace task assemblers with direct use of a DI container. This would be particularly convenient if we used a container that supported inflectors, such as League\Container.

Doing this would substantially change the syntax of Robo, though; $this->taskExec() would become $container->get('taskExec'). I don't know if that would be a definite improvement, but I thought I'd mention it as a possibility. Maybe defer until later, and keep the proposed implementation in this PR to maintain compatibility? I could go either way.

UPDATE: I think I'd prefer to avoid tying this part of the code to the specific container implementation. It will reduce coupling between the code to rely only on constructor / setter injection.

greg-1-anderson · 2016-03-09T17:06:05Z

See the discussion in the Drush issue queue on the thought process I went through to select a proposed DI container for Drush. Aurora DI could work; it's syntax is fine, but I didn't really like the names of its methods. I like league/container quite a bit more than Symfony DI.

Nitpick-CI · 2016-03-10T01:19:22Z

src/Log/ResultPrinter.php

-    {
-        $this->logger = $logger;
-    }
+   use LoggerAwareTrait;


Line indented incorrectly; expected at least 4 spaces, found 3

greg-1-anderson · 2016-03-10T01:23:48Z

Switched to league/container for DI. The inflector feature allows us to automatically inject a logger into any object (managed by the DI container) simply by having it implement LoggerAwareInterface and use LoggerAwareTrait. This gives us the convenience of auto-wiring, but in a more controlled, declarative way.

DavertMik · 2016-03-13T00:15:27Z

composer.json

@@ -21,7 +21,7 @@
        "symfony/console": "~2.5|~3.0",
        "symfony/process": "~2.5|~3.0",
        "symfony/filesystem": "~2.5|~3.0",
-        "symfony/dependency-injection": "~2.5|~3.0"
+        "league/container": "~2"


good choice )

DavertMik · 2016-03-13T00:38:19Z

I really like your choice of container

But I think If we choose it, maybe we can use it in full and not include task assembler class. The overall concept seems like factory method, so it can be moved to the container.

Doing this would substantially change the syntax of Robo, though; $this->taskExec() would become $container->get('taskExec'). I don't know if that would be a definite improvement, but I thought I'd mention it as a possibility.

Yes, this will be much cleaner if container would manage creation of tasks, and inject everything. Probably you can use http://container.thephpleague.com/factory-closures/ to implement simulated behaviors.

Also I'd ask you to use ::class constant instead of strings in class names. They really save ass, in refactorings, and they really look nicer.

And thanks for your work. I really like the way it is moving, but I'm still desore to find a way to keep things simpler )

Nitpick-CI · 2016-03-16T22:48:17Z

src/Common/TaskIO.php

@@ -14,6 +16,25 @@
 */
 trait TaskIO
 {
+    use LoggerAwareTrait;
+
+    static $gaveDeprecationWarning = false;


Visibility must be declared on property $gaveDeprecationWarning

Nitpick-CI · 2016-03-16T23:02:32Z

RoboFile.php

+
+        if (is_dir($tmpPath)) {
+            $this->say("Created a temporary directory at $tmpPath");
+        }


Expected 1 space after closing brace; newline found

greg-1-anderson · 2016-03-17T02:13:42Z

The latest technique does not quite work due to a limitation in League/Container. I have submitted a PR to rectify the situation. I suspect the maintainer might want to see the strictly backwards-compatible implementation of this.

greg-1-anderson · 2016-03-17T03:12:43Z

Submitted a backwards compatible version: thephpleague/container#94

philipobenito · 2016-03-17T08:38:38Z

Hope this is agreeable thephpleague/container#94 (comment)

… multiple providers may be registered.

greg-1-anderson · 2016-03-17T16:17:07Z

Thanks, @philipobenito! Works great & is totally awesome. This will be great for both Robo and Drush.

philipobenito · 2016-03-17T16:19:42Z

Brilliant stuff, I'll get you a tag sorted in the next few mins.

greg-1-anderson · 2016-03-17T16:35:16Z

@DavertMik this is now using the tagged release of league/container. I think this should now meet all of your expectations per our last chat -- and, against all odds, even maintains bc (although --simulate won't work for folks who keep using the deprecated bc loadTasks conventions). Take a look and let me know. Excited to see this merged soon!

greg-1-anderson · 2016-03-17T16:37:58Z

Not captured in the discussion above (from chat): we want to preserve loadTasks for parameter type hinting, and autocomplete in IDEs. However, now that we have combined the service provider into the loadTasks file, it isn't so bad to have both. The pattern for making new tasks is pretty apparent from the existing examples; I'll also update the documentation on this.

DavertMik · 2016-03-19T21:07:27Z

Awesome ❤️ ❤️ ❤️

Task assembly

greg-1-anderson added 2 commits March 7, 2016 08:23

Create a task assembly step, and use it in all loadTasks. This allows…

cea3129

… us to reduce internal coupling, and also support temporary and simulated tasks in a uniform way.

Improve styling of simulator output.

084e165

Nitpick-CI reviewed Mar 7, 2016
View reviewed changes

greg-1-anderson added 2 commits March 7, 2016 10:33

Fix Nitpick CI issues.

3d59092

Automatically exclude accessor methods from command list.

dcf1b03

Use reverse styling instead of assuming the terminal colors.

f0ae7e9

greg-1-anderson mentioned this pull request Mar 7, 2016

TaskLib, PSR-3 logger, and interim DI container. #283

Closed

Switch to league/container for Dependency Injection.

dfff660

Nitpick-CI reviewed Mar 10, 2016
View reviewed changes

Adjust whitespace for NitPick.

d9c20af

Remove test code left in by accident.

ec5d60f

DavertMik reviewed Mar 13, 2016
View reviewed changes

Nitpick-CI reviewed Mar 16, 2016
View reviewed changes

greg-1-anderson added 2 commits March 16, 2016 15:54

SimpleServiceProvider does not need to be abstract.

ac88ae9

Move File ServiceProviders into loadTasks.

9b31b4e

Nitpick-CI reviewed Mar 16, 2016
View reviewed changes

greg-1-anderson force-pushed the task-assembly branch from 00d525f to 9b31b4e Compare March 16, 2016 23:22

Move all of the ServiceProvider classes into loadTasks.

a1515f6

Fix a couple of style problems.

d141895

Use AbstractSignatureServiceProvider in SimpleServiceProvider so that…

699d388

… multiple providers may be registered.

greg-1-anderson mentioned this pull request Mar 17, 2016

Drop support for PHP 5.4 #259

Closed

Use league/container 2.2

cf079a1

DavertMik added a commit that referenced this pull request Mar 19, 2016

Merge pull request #284 from Codegyre/task-assembly

ccda129

Task assembly

DavertMik merged commit ccda129 into tasklib-2 Mar 19, 2016

DavertMik deleted the task-assembly branch March 19, 2016 21:07

greg-1-anderson mentioned this pull request Mar 21, 2016

Restore rsync test backed out in previous merge. #291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task assembly #284

Task assembly #284

greg-1-anderson commented Mar 7, 2016

Nitpick-CI Mar 7, 2016

greg-1-anderson Mar 7, 2016

greg-1-anderson commented Mar 7, 2016

greg-1-anderson commented Mar 9, 2016

greg-1-anderson commented Mar 9, 2016

Nitpick-CI Mar 10, 2016

greg-1-anderson commented Mar 10, 2016

DavertMik Mar 13, 2016

DavertMik commented Mar 13, 2016

Nitpick-CI Mar 16, 2016

Nitpick-CI Mar 16, 2016

Nitpick-CI Mar 16, 2016

greg-1-anderson commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

philipobenito commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

philipobenito commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

DavertMik commented Mar 19, 2016

Task assembly #284

Task assembly #284

Conversation

greg-1-anderson commented Mar 7, 2016

Nitpick-CI Mar 7, 2016

Choose a reason for hiding this comment

greg-1-anderson Mar 7, 2016

Choose a reason for hiding this comment

greg-1-anderson commented Mar 7, 2016

greg-1-anderson commented Mar 9, 2016

greg-1-anderson commented Mar 9, 2016

Nitpick-CI Mar 10, 2016

Choose a reason for hiding this comment

greg-1-anderson commented Mar 10, 2016

DavertMik Mar 13, 2016

Choose a reason for hiding this comment

DavertMik commented Mar 13, 2016

Nitpick-CI Mar 16, 2016

Choose a reason for hiding this comment

Nitpick-CI Mar 16, 2016

Choose a reason for hiding this comment

Nitpick-CI Mar 16, 2016

Choose a reason for hiding this comment

greg-1-anderson commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

philipobenito commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

philipobenito commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

greg-1-anderson commented Mar 17, 2016

DavertMik commented Mar 19, 2016