Use SymfonyStyle for command output #6655

OskarStark · 2017-08-29T13:41:06Z

I started to implement SymfonyStyle to the commands, I will follow up with the other commands if you like this.

Here are some screenshots:

Is the branch the correct one or shall I switch?

Cheers, Oskar

OskarStark · 2017-08-30T08:06:07Z

fyi @javiereguiluz ;-)

javiereguiluz · 2017-08-30T08:14:19Z

lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/UpdateCommand.php

-        $output->writeln('           Use the incremental update to detect changes during development and use');
-        $output->writeln('           the SQL DDL provided to manually update your database in production.');
-        $output->writeln('');
+        $this->io->caution(<<<EOT


In case you prefer it, most methods accept both strings and arrays of strings, so this code could be:

$this->io->caution([ 'This operation should not be executed in a production environment!', '', 'Use the incremental update to detect changes during development and use', 'the SQL DDL provided to manually update your database in production.', ]);

I saw this afterwards 👍

I can rework this if the guys like the PR and prefer the array syntax.

Thank you for your feedback!

Array syntax looks better in my opinion

lcobucci

@OskarStark some small suggestions here, thanks for your contribution, the command's output is indeed looking better.

In terms of branching, this is correct 😉.

lcobucci · 2017-08-30T09:09:52Z

lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/UpdateCommand.php

@@ -39,6 +40,11 @@
 class UpdateCommand extends AbstractCommand
 {
    /**
+     * @var SymfonyStyle
+     */
+    protected $io;


Maybe ui or userInterface is a better name?

Also, wouldn't it be better to have this property in the AbstractCommand

I can change this ofc, but I use $io everywhere and it was mentioned in the official blog post, too. $ui is a bit missleading IMO

@javiereguiluz what do you think, how do you use it?

Official blog post: https://symfony.com/blog/new-in-symfony-2-8-console-style-guide

I just suggested that because IMO we're actually interacting with the CLI user interface 😄

lcobucci · 2017-08-30T09:10:19Z

lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/UpdateCommand.php

-        $output->writeln('           Use the incremental update to detect changes during development and use');
-        $output->writeln('           the SQL DDL provided to manually update your database in production.');
-        $output->writeln('');
+        $this->io->caution(<<<EOT


Array syntax looks better in my opinion

lcobucci · 2017-08-30T09:16:14Z

lib/Doctrine/ORM/Tools/Console/Command/SchemaTool/UpdateCommand.php

@@ -109,13 +115,15 @@ protected function configure()
     */
    protected function executeSchemaCommand(InputInterface $input, OutputInterface $output, SchemaTool $schemaTool, array $metadatas)
    {
+        $this->io = new SymfonyStyle($input, $output);


This could also be part of the AbstractCommand#execute()

lcobucci · 2017-08-30T09:25:01Z

@OskarStark just a remark, please make sure you send test updates too.

OskarStark · 2017-08-31T06:44:45Z

Ok, lets check the build 😄

Ocramius · 2017-08-31T08:05:43Z

lib/Doctrine/ORM/Tools/Console/Command/ClearCache/CollectionRegionCommand.php

@@ -36,21 +37,24 @@
 class CollectionRegionCommand extends Command
 {
    /**
+     * @var SymfonyStyle
+     */
+    protected $ui;


From what I see, ui is not needed to be retained in internals: please convert it to a local variable wherever possible. If multiple private methods rely on it, please pass it around as a call parameter.

Ocramius · 2017-08-31T08:06:12Z

lib/Doctrine/ORM/Tools/Console/Command/ClearCache/CollectionRegionCommand.php

        $em          = $this->getHelper('em')->getEntityManager();
        $ownerClass  = $input->getArgument('owner-class');
        $assoc       = $input->getArgument('association');
        $ownerId     = $input->getArgument('owner-id');
        $cache       = $em->getCache();

-        if ( ! $cache instanceof Cache) {
+        if (!$cache instanceof Cache) {


Please revert these spacing changes

Ocramius · 2017-08-31T08:06:21Z

lib/Doctrine/ORM/Tools/Console/Command/ClearCache/CollectionRegionCommand.php

@@ -36,21 +37,24 @@
 class CollectionRegionCommand extends Command
 {
    /**
+     * @var SymfonyStyle


SymfonyStyle|null

Ocramius · 2017-08-31T08:07:01Z

lib/Doctrine/ORM/Tools/Console/Command/ClearCache/EntityRegionCommand.php

@@ -36,20 +37,23 @@
 class EntityRegionCommand extends Command
 {
    /**
+     * @var SymfonyStyle


SymfonyStyle|null

Ocramius · 2017-08-31T08:07:30Z

lib/Doctrine/ORM/Tools/Console/Command/ClearCache/MetadataCommand.php

@@ -39,23 +40,27 @@
 class MetadataCommand extends Command
 {
    /**
+     * @var SymfonyStyle


SymfonyStyle|null

OskarStark · 2017-09-27T10:32:45Z

Hmm, this is a bit strange.
on my local machine, all tests are running, if my shell is big, if i minimize it it delivers the same error results like travis:

5) Doctrine\Tests\ORM\Tools\Console\Command\ClearCacheEntityRegionCommandTest::testClearCacheEntryName
Failed asserting that '\n
 // Clearing second-level cache entry for entity                                \n
 // "Doctrine\Tests\Models\Cache\Country" identified by                         \n
 // "1"                                                                         \n
\n
' contains " // Clearing second-level cache entry for entity "Doctrine\Tests\Models\Cache\Country" identified by".

The SymfonyStyle add line breaks if needed.

any idea @Ocramius @javiereguiluz ?

OskarStark · 2017-09-27T10:37:11Z

I found this: https://stackoverflow.com/questions/39190610/how-to-test-console-command-output-when-terminal-widths-differ/40347595#40347595

putenv('COLUMNS=120');

WDYT?

OskarStark · 2017-09-27T10:48:05Z

lets check if 142055c makes travis happy 🎉

OskarStark · 2017-09-27T10:50:41Z

Fails:

oskar.stark:~/dev/misc/doctrine2 (symfony-style)$ env COLUMNS=10 php -d memory_limit=-1 vendor/bin/phpunit --group=test
PHPUnit 6.4-ga1b5663a4 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.8
Configuration: /Users/oskar.stark/dev/misc/doctrine2/phpunit.xml.dist

.F                                                                  2 / 2 (100%)

Time: 484 ms, Memory: 40.00MB

There was 1 failure:

1) Doctrine\Tests\ORM\Tools\Console\Command\ClearCacheCollectionRegionCommandTest::testClearByOwnerEntityClassName
Failed asserting that '\n
 // Cleari\n
 // ng    \n
 // second\n
 // -level\n
 // cache \n
 // for   \n
 // collec\n
 // tion  \n
 //       \n
 // "Doctr\n
 // ine\Te\n
 // sts\Mo\n
 // dels\C\n
 // ache\S\n
 // tate#c\n
 // ities"\n
 // </info\n
 // >     \n
\n
' contains " // Clearing second-level cache for collection "Doctrine\Tests\Models\Cache\State#cities"".

/Users/oskar.stark/dev/misc/doctrine2/tests/Doctrine/Tests/ORM/Tools/Console/Command/ClearCacheCollectionRegionCommandTest.php:79

FAILURES!
Tests: 2, Assertions: 6, Failures: 1.

works:

oskar.stark:~/dev/misc/doctrine2 (symfony-style)$ env COLUMNS=120 php -d memory_limit=-1 vendor/bin/phpunit --group=test
PHPUnit 6.4-ga1b5663a4 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.8
Configuration: /Users/oskar.stark/dev/misc/doctrine2/phpunit.xml.dist

..                                                                  2 / 2 (100%)

Time: 474 ms, Memory: 40.00MB

OK (2 tests, 6 assertions)

OskarStark · 2017-09-27T12:53:44Z

Looks like 2 travis builds are still failing.... I don't know why.

Scrutinizer tests pass, after setting the COLUMNS env in .scrutinizer.yml

OskarStark · 2017-09-28T09:31:24Z

https://github.com/doctrine/doctrine2/pull/6655/files#diff-354f30a63fb0907d4ad57269548329e3R12 fixes most of the travis builds, but 7.1 and 7.2 builds are still failing and I don't know why 😕 ❗️

OskarStark · 2017-11-09T07:04:29Z

I rebased my branch, but don't know if travis is working now

OskarStark · 2017-11-09T07:29:27Z

The 7.1 and 7.2 are failing if there is a custom MYSQL_VERSION set.
Looks like the COLUMNS env var is ignored, do you spawn any sub commands or sth like this, where the COLUMNS env var is not used anymore?

At the start of the build, the COLUMNS env var is exported

OskarStark · 2017-11-17T09:52:39Z

any news?

OskarStark · 2017-11-21T12:36:01Z

Not a single comment/feedback since the end of august, that makes me sad.... 😞

If you don't like it I will close the PR, otherwise I need some help how to fix it

Ocramius

Didn't get to do any proper OSS since August either, sorry.

From my end, the patch looks simple/solid

lcobucci · 2017-11-21T12:43:50Z

Not a single comment/feedback since the end of august, that makes me sad.... 😞
If you don't like it I will close the PR, otherwise I need some help how to fix it

@OskarStark is not about liking it or not, it's simply a matter of time. All maintainers all quite busy with paid work and we're doing our best to at least release bug fixes. I know that it's not ideal but I hope you understand our situation...

greg0ire · 2017-11-21T14:12:00Z

Other similarity between failing jobs: sudo: required

greg0ire · 2017-11-21T14:12:49Z

Possible solution: https://stackoverflow.com/questions/36133265/unable-to-get-env-variable-value-in-a-before-install-script-with-travis#36161645

OskarStark · 2017-11-22T06:52:35Z

damn, nothing is working 👎

greg0ire · 2017-11-22T07:40:53Z

You could try to add sudo: required to another build to see if this it what causes the problem

OskarStark · 2017-11-22T07:43:52Z

updated

greg0ire · 2017-11-22T07:52:42Z

Looks like it is! That's progress 👍

OskarStark · 2017-11-23T09:27:43Z

there are some parts of the code that are not being covered by tests, can you please check that? https://scrutinizer-ci.com/g/doctrine/doctrine2/inspections/cf59ead0-6eb9-4a28-b114-91d5328ba6e0/code-structure/test-coverage might be helpful

I will have a look

You're formatting some parts of the code in the same commit of your changes, that's a bit confusing to review (specially via mobile 😂), can you please revert those changes or split them into a different commit?

damn, i did before, but squashed all commits, thought this was already done 😞
Shall I rework the PR and split everything in different commits?

there're some additions that don't follow our code standards (like the spacing before and after the negation operator), can you please check that?

ofc, I will check, do you have a tool for that like StyleCi or a PHP-CS-Fixer config?

I think you forgot to define the constant in phpunit.xml.dist

done

lcobucci · 2017-11-23T09:34:15Z

Shall I rework the PR and split everything in different commits?

I think it's fine to have 2 commits one that formats the old code and one that introduces SymfonyStyle, it should be simple to do that with reverting (soft) your commit and using git add -p but if I can help you on that later.

ofc, I will check, do you have a tool for that like StyleCi or a PHP-CS-Fixer config?

Yes, we use phpcs but we haven't configured it for the ORM yet 😭

OskarStark · 2017-11-23T09:35:49Z

I think it's fine to have 2 commits one that formats the old code and one that introduces SymfonyStyle, it should be simple to do that with reverting (soft) your commit and using git add -p but if I can help you on that later.

don't worry, I will do it, but how to proceed with Scrutinizer? set COLUMNS or not?

Yes, we use phpcs but we haven't configured it for the ORM yet 😭

ok, i checked for the space before and after !

lcobucci · 2017-11-23T10:10:47Z

don't worry, I will do it, but how to proceed with Scrutinizer?

@OskarStark I've merged the scrutinizer config to not run the tests there, please rebase your branch to sync with master

set COLUMNS or not?

Don't change anything on scrutinizer config =)

OskarStark · 2017-11-23T10:54:55Z

@OskarStark I've merged the scrutinizer config to not run the tests there, please rebase your branch to sync with master

rebased and splitted my commits

lcobucci

@OskarStark great job with the commit organisation (and thanks for fixing gramar mistakes as well)!

I got some final nitpickings. For things related coding standard stuff I've just merged #6844 in, so that might help you check for possible issues (along with the tool that @morozov created).

lcobucci · 2017-11-23T12:29:51Z

lib/Doctrine/ORM/Tools/Console/Command/ClearCache/ResultCommand.php

@@ -105,6 +100,12 @@ protected function execute(InputInterface $input, OutputInterface $output)
            $message = ($result) ? 'Successfully flushed cache entries.' : $message;
        }

-        $output->writeln($message);
+        if (!$result) {


You forgot to fix CS here

lcobucci · 2017-11-23T12:31:47Z

tests/Doctrine/Tests/ORM/Tools/Console/Command/ClearCacheCollectionRegionCommandTest.php

@@ -53,7 +53,10 @@ public function testClearAllRegion()
            ], ['decorated' => false]
        );

-        $this->assertEquals('Clearing all second-level cache collection regions' . PHP_EOL, $tester->getDisplay());
+        $this->assertContains(
