Migrate to symfony/console #23

weierophinney · 2017-04-10T16:45:37Z

This patch migrates the tooling to use symfony/console instead of zend-stdlib's ConsoleHelper. The reasons are:

We were needing to write argument parsing routines for each and every command, and each differed.
We were needing to write "help" functionality for each command.
We were creating a plethora of entry points. We should have one, with many commands available.
While we could use zf-deploy, using symfony/console will likely get more contributors to the project, due to its ubiquity in the PHP ecosystem. In the end, I also discovered that I ended up writing less code overall in order to create commands, and had better, simpler options for re-use when re-purposing existing command endpoints to re-use the same commands as used by the main entry point.

All existing commands continue to work. I have updated the expressive tool to use the following patterns:

migrate:*: the various tools dedicated to migration.
module:*: the various tools dedicated to module manipulation.
middleware:*: the tools dedicated to middleware manipulation
(currently only creation).

TODO

Update unit tests.
Add deprecation warnings to original, now legacy, scripts.

This patch migrates the tooling to use symfony/console instead of zend-stdlib's `ConsoleHelper`. The reasons are: - We were needing to write argument parsing routines for each and every command, and each differed. - We were needing to write "help" functionality for each command. - We were creating a plethora of entry points. We should have one, with many commands available. - While we could use zf-deploy, using symfony/console will likely get more contributors to the project, due to its ubiquity in the PHP ecosystem. In the end, I also discovered that I ended up writing less code overall in order to create commands, and had better, simpler options for re-use when re-purposing existing command endpoints to re-use the same commands as used by the main entry point. All existing commands continue to work. I have updated the `expressive` tool to use the following patterns: - `migrate:*`: the various tools dedicated to migration. - `module:*`: the various tools dedicated to module manipulation. - `middleware:*`: the tools dedicated to middleware manipulation (currently only creation).

- Removed obsolete `Command` and `Help` tests - Re-arranged `Module` tests and updated namespaces and class names

michalbundyra

I like update to symfony/console! Many things is already done there so it simplify our library :) Couple of things to change and should be fine. Of course updating tests as you pointed is needed. Thanks!

michalbundyra · 2017-04-10T17:02:41Z

src/Module/CreateCommand.php

+        } catch (Exception\RuntimeException $e) {
+            $console = $this->getErrorConsole($output);
+            $console->writeln('<error>Error during execution:</error>', true, STDERR);
+            $console->writeln(sprintf('  <error>%s</error>', $ex->getMessage()), true, STDERR);


$ex is not defined. Here should be $e or variable in catch should be renamed (I'd prefer to use $ex for exception)

michalbundyra · 2017-04-10T17:04:57Z

src/Module/CreateCommand.php

+            $output->writeln(sprintf('<info>%s</info>', $message));
+        } catch (Exception\RuntimeException $e) {
+            $console = $this->getErrorConsole($output);
+            $console->writeln('<error>Error during execution:</error>', true, STDERR);


Now we have different signature of writeln method - from symfony, so there are two params: $messages and $options.

michalbundyra · 2017-04-10T17:09:15Z

src/Module/DeregisterCommand.php

+            $disable = new Disable($this->projectDir, $modulesPath, $composer);
+            $disable->process($module);
+        } catch (RuntimeException $ex) {
+            $console = $this->getErrorConsole();


This method should be called with parameter. It's missing here.

michalbundyra · 2017-04-10T17:09:50Z

src/Module/RegisterCommand.php

+            $enable->setMoveModuleClass(false);
+            $enable->process($module);
+        } catch (RuntimeException $ex) {
+            $console = $this->getErrorConsole();


As above - missing required param.

michalbundyra · 2017-04-10T17:14:26Z

src/ScanForErrorMiddleware/Scanner.php

@@ -34,9 +34,9 @@ class Scanner implements Countable, IteratorAggregate

    /**
     * @param string $path
-     * @param ConsoleHelper $console
+     * @param OutputInterface $console


The same should be changed also in line 21.

This patch updates all tests as follows: - Removes obsolete `Command` and `Help` tests. - Adds tests for new symfony/console commands. - Fixes test setup and expectations for classes that now depend on a symfony/console `OutputInterface` instance. - Fixes issues found during testing, including several also reported by @webimpress.

- Per @webimpress

weierophinney · 2017-04-10T21:23:54Z

All tests updated and/or new tests written; doing so addressed most of the feedback from @webimpress, and I've updated a property typehint per his feedback as well.

weierophinney · 2017-04-10T21:35:32Z

Pinging @GeeH, @akrabat, and @calevans for feedback...

Ocramius · 2017-04-10T21:36:15Z

bin/expressive

-use Zend\Stdlib\ConsoleHelper;
+use Symfony\Component\Console\Application;
+
+const VERSION = '%version%';


cough PackageVersions cough

cough PHP 5.6 cough

Ocramius · 2017-04-10T21:37:11Z

bin/expressive

-$return = $command->process(array_slice($argv, 1));
-exit($return);
+$application = new Application('expressive', VERSION);
+$application->add(new CreateMiddleware\CreateMiddlewareCommand('middleware:create'));


Application#addCommands()

Cool, wasn't aware of that one!

Ocramius · 2017-04-10T21:37:55Z

bin/expressive-migrate-original-messages

-$command = new Command($argv[0], new ConsoleHelper());
-$return = $command->process(array_slice($argv, 1));
-exit($return);
+$application = new Application('expressive-migrate-original-messages', VERSION);


Can include the main CLI app, then set the default command instead.

This is a BC measure; the goal is to eventually remove it.

Also note that the command names differ between the main expressive script and this one.

Ocramius · 2017-04-10T21:38:24Z

bin/expressive-module

-$return = $command->process(array_slice($argv, 1));
-exit($return);
+$application = new Application('expressive-module', VERSION);
+$application->add(new CreateCommand('create'));


Was this done just to keep the command names BC compliance?

Yes. Since we already have documented migration steps, I cannot break how these original scripts worked; hence the difference between command names and what commands are exposed.

Ocramius · 2017-04-10T21:39:04Z

bin/expressive-pipeline-from-config

-$command = new Command($argv[0], new ConsoleHelper());
-$return = $command->process(array_slice($argv, 1));
-exit($return);
+$application = new Application('expressive-pipeline-from-config', VERSION);


Same as above: main app, set default command in this file

See above; this is a BC measure.

Ocramius · 2017-04-10T21:46:43Z

src/MigrateOriginalMessageCalls/MigrateOriginalMessageCallsCommand.php

+                '<error>Unable to determine src directory: %s</error>',
+                $e->getMessage()
+            ));
+            return 1;


Let exceptions bubble up. If the exception message is meaningful, no additional code is needed

Ocramius · 2017-04-10T21:47:11Z

src/Module/CreateCommand.php

+            $creation = new Create();
+            $message = $creation->process($module, $modulesPath, $this->projectDir);
+            $output->writeln(sprintf('<info>%s</info>', $message));
+        } catch (Exception\RuntimeException $e) {


remove, let it bubble up

Ocramius · 2017-04-10T21:47:23Z

src/Module/DeregisterCommand.php

+        try {
+            $disable = new Disable($this->projectDir, $modulesPath, $composer);
+            $disable->process($module);
+        } catch (RuntimeException $ex) {


Remove, let it bubble up

Ocramius · 2017-04-10T21:47:33Z

src/Module/RegisterCommand.php

+            $enable = new Enable($this->projectDir, $modulesPath, $composer);
+            $enable->setMoveModuleClass(false);
+            $enable->process($module);
+        } catch (RuntimeException $ex) {


Ocramius · 2017-04-10T21:47:45Z

src/ScanForErrorMiddleware/ScanForErrorMiddlewareCommand.php

+    {
+        try {
+            $src = $this->getSrcDir($input);
+        } catch (ArgvException $e) {


@Ocramius

- per @Ocramius

@Ocramius

- per @Ocramius

@Ocramius

- Per @Ocramius

GeeH · 2017-04-11T07:54:29Z

The move to this library is amazing, we should always look to use community defacto libraries instead of having to code and support our own whatever the vendor namespace. I'm happy.

I don't have time to review the code sorry (and I don't know the library very well anyway), but...

@Ocramius

Per @Ocramius, it's simpler to allow exceptions to bubble up, as symfony/console will report the exception message always, and, when verbosity is enabled, more details including the stack trace.

weierophinney · 2017-04-11T13:16:47Z

@Ocramius I've incorporated your feedback; thanks a ton!

Combines the entry for zendframework#22 with zendframework#23.

Have it invoke `to` with a fixed `version` argument.

Ocramius · 2017-04-11T16:35:27Z

src/GenerateProgrammaticPipelineFromConfig/PipelineFromConfigCommand.php

-            return 1;
-        }
+        $generator = new Generator($output);
+        $generator->projectDir = $this->projectDir;


Argh... Missed this before, why is this necessary?

Actually... no longer necessary. It was necessary before I started mocking the various internal classes, but now it isn't. I'll nuke it in the various *Command classes.

It's still necessary for the internal classes that do filesystem operations, though I may be able to move that to constructor arguments. I'll review.

Ocramius · 2017-04-11T16:37:42Z

src/Module/CommandCommonTrait.php

+    /**
+     * Add default arguments and options used by all commands.
+     */
+    private function addDefaultOptionsAndArguments()


This should be a public static function, referenced by classes using it.

Ocramius · 2017-04-11T16:37:53Z

src/Module/CommandCommonTrait.php

+     * @param InputInterface $input
+     * @return string
+     */
+    private function getModulesPath(InputInterface $input)


This should be a public static function, referenced by classes using it.

Ocramius · 2017-04-11T16:38:38Z

src/Module/CommandCommonTrait.php

+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Input\InputOption;
+
+trait CommandCommonTrait


To be moved to a static class and marked as @internal, since we really don't want to distribute this as public/reusable API

Fair enough; I'll make that change (and the ones you suggest below) momentarily.

This includes all commands, though a few, where we needed to test path existence, required the ability to inject a path, which is now handled by a setter; when the setter is not called, `getcwd()` is then assumed. For internal classes, this is now handled as either a passed argument, or via a constructor argument.

@Ocramius

- Per @Ocramius

Ocramius · 2017-04-11T17:59:19Z

🚢

weierophinney force-pushed the feature/symfony-console branch from 4e522be to f05fd4d Compare April 10, 2017 16:49

weierophinney added this to the 0.4.0 milestone Apr 10, 2017

weierophinney added the enhancement label Apr 10, 2017

weierophinney force-pushed the feature/symfony-console branch from f05fd4d to 791864d Compare April 10, 2017 17:03

Initial test restructuring

a3b60f1

- Removed obsolete `Command` and `Help` tests - Re-arranged `Module` tests and updated namespaces and class names

michalbundyra suggested changes Apr 10, 2017

View reviewed changes

weierophinney added 2 commits April 10, 2017 16:20

Fixed property typehint

6a2717e

- Per @webimpress

Ocramius suggested changes Apr 10, 2017

View reviewed changes

weierophinney added 3 commits April 10, 2017 16:58

Use addCommands instead of multiple addCommand calls

8afc62f

- per @Ocramius

Use latest patch release of symfony/console

283068f

- per @Ocramius

Write all migrate:pipeline output to STDOUT

835a634

- Per @Ocramius

Do not worry about error output

a107bc1

Per @Ocramius, it's simpler to allow exceptions to bubble up, as symfony/console will report the exception message always, and, when verbosity is enabled, more details including the stack trace.

weierophinney added 5 commits April 11, 2017 08:21

Updated CHANGELOG for zendframework#23

15d581c

Combines the entry for zendframework#22 with zendframework#23.

Added tooling for bumping the release version of the scripts.

45d38e4

Added deprecation notices to each of the original scripts

7c076ff

Better names for test cases that expect exceptions

9f9c729

Simplify bump-version dev

6f05b90

Have it invoke `to` with a fixed `version` argument.

michalbundyra approved these changes Apr 11, 2017

View reviewed changes

Ocramius suggested changes Apr 11, 2017

View reviewed changes

weierophinney added 2 commits April 11, 2017 12:05

Refactor CommandCommonTrait to final, static class CommandCommonOptions

c57ca08

- Per @Ocramius

Ocramius approved these changes Apr 11, 2017

View reviewed changes

Ocramius self-assigned this Apr 11, 2017

Ocramius merged commit bfc8eeb into zendframework:master Apr 11, 2017

weierophinney deleted the feature/symfony-console branch April 11, 2017 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to symfony/console #23

Migrate to symfony/console #23

weierophinney commented Apr 10, 2017 •

edited

Loading

michalbundyra left a comment

michalbundyra Apr 10, 2017

michalbundyra Apr 10, 2017

michalbundyra Apr 10, 2017

michalbundyra Apr 10, 2017

michalbundyra Apr 10, 2017

weierophinney Apr 10, 2017

weierophinney commented Apr 10, 2017

weierophinney commented Apr 10, 2017

Ocramius Apr 10, 2017

weierophinney Apr 10, 2017

Ocramius Apr 10, 2017

weierophinney Apr 10, 2017

Ocramius Apr 10, 2017

weierophinney Apr 10, 2017

Ocramius Apr 10, 2017

weierophinney Apr 10, 2017

Ocramius Apr 10, 2017

weierophinney Apr 10, 2017

Ocramius Apr 10, 2017

Ocramius Apr 10, 2017

Ocramius Apr 10, 2017

Ocramius Apr 10, 2017

Ocramius Apr 10, 2017

GeeH commented Apr 11, 2017

weierophinney commented Apr 11, 2017

Ocramius Apr 11, 2017

weierophinney Apr 11, 2017

Ocramius Apr 11, 2017

Ocramius Apr 11, 2017

Ocramius Apr 11, 2017

weierophinney Apr 11, 2017

Ocramius commented Apr 11, 2017

Migrate to symfony/console #23

Migrate to symfony/console #23

Conversation

weierophinney commented Apr 10, 2017 • edited Loading

TODO

michalbundyra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Apr 10, 2017

weierophinney commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeeH commented Apr 11, 2017

weierophinney commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Apr 11, 2017

weierophinney commented Apr 10, 2017 •

edited

Loading