Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let commands write errors to STDERR if available (fix issue 2270) #2317

Open
wants to merge 8 commits into
base: 0.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions phpcs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@

<rule ref="CakePHP"/>

<rule ref="SlevomatCodingStandard.Namespaces.UnusedUses">
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests do not contain all necessary imports (uses) right now because without this PHPCS will complain.
I added the missing imports to MigrateTest and will leave the rest for someone else to clean up.

<properties>
<property name="searchAnnotations" value="true"/>
</properties>
</rule>

<arg value="nps"/>

<file>src/</file>
Expand Down
16 changes: 14 additions & 2 deletions src/Phinx/Console/Command/AbstractCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\ConsoleOutputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use UnexpectedValueException;

Expand Down Expand Up @@ -439,7 +440,7 @@
}

if (!$this->getConfig()->hasEnvironment($environment)) {
$output->writeln(sprintf('<error>The environment "%s" does not exist</error>', $environment));
self::getErrorOutput($output)->writeln(sprintf('<error>The environment "%s" does not exist</error>', $environment));

return false;
}
Expand Down Expand Up @@ -478,7 +479,7 @@
}
$output->writeln('<info>using database</info> ' . $name, $this->verbosityLevel);
} else {
$output->writeln('<error>Could not determine database name! Please specify a database name in your config file.</error>');
self::getErrorOutput($output)->writeln('<error>Could not determine database name! Please specify a database name in your config file.</error>');

Check warning on line 482 in src/Phinx/Console/Command/AbstractCommand.php

View check run for this annotation

Codecov / codecov/patch

src/Phinx/Console/Command/AbstractCommand.php#L482

Added line #L482 was not covered by tests

return false;
}
Expand All @@ -492,4 +493,15 @@

return true;
}

/**
* Returns the error output to use
*
* @param \Symfony\Component\Console\Output\OutputInterface $output Output
* @return \Symfony\Component\Console\Output\OutputInterface
*/
protected static function getErrorOutput(OutputInterface $output): OutputInterface
{
return $output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output;
}
}
8 changes: 4 additions & 4 deletions src/Phinx/Console/Command/ListAliases.php
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ function ($alias, $class) use ($maxAliasLength, $maxClassLength) {
);
} else {
$output->writeln(
sprintf(
'<error>No aliases defined in %s</error>',
Util::relativePath($this->config->getConfigFilePath())
)
'<comment>warning</comment> no aliases defined in ' . Util::relativePath(
$this->config->getConfigFilePath()
),
$this->verbosityLevel
);
}

Expand Down
7 changes: 1 addition & 6 deletions src/Phinx/Console/Command/Migrate.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
namespace Phinx\Console\Command;

use DateTime;
use Exception;
use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
Expand Down Expand Up @@ -92,12 +91,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$this->getManager()->migrate($environment, $version, $fake);
}
$end = microtime(true);
} catch (Exception $e) {
$output->writeln('<error>' . $e->__toString() . '</error>');

return self::CODE_ERROR;
} catch (Throwable $e) {
$output->writeln('<error>' . $e->__toString() . '</error>');
self::getErrorOutput($output)->writeln('<error>' . $e->__toString() . '</error>');

return self::CODE_ERROR;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Phinx/Db/Adapter/SQLiteAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public function connect(): void
*/
public static function getSuffix(array $options): string
{
if ($options['name'] === self::MEMORY) {
if (($options['name'] ?? '') === self::MEMORY) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produced a notice when running tests (with error_reporting = E_ALL in php.ini)

return '';
}

Expand Down
6 changes: 3 additions & 3 deletions tests/Phinx/Console/Command/ListAliasesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,18 @@ public function testListingAliases($file, $hasAliases)
],
['decorated' => false]
);
$this->assertEquals(AbstractCommand::CODE_SUCCESS, $exitCode);
$this->assertSame(AbstractCommand::CODE_SUCCESS, $exitCode);

$display = $commandTester->getDisplay(false);

if ($hasAliases) {
$this->assertStringNotContainsString('No aliases defined in ', $display);
$this->assertStringNotContainsString('no aliases defined in ', $display);
$this->assertStringContainsString('Alias Class ', $display);
$this->assertStringContainsString('================ ==================================================', $display);
$this->assertStringContainsString('MakePermission Vendor\Package\Migration\Creation\MakePermission ', $display);
$this->assertStringContainsString('RemovePermission Vendor\Package\Migration\Creation\RemovePermission', $display);
} else {
$this->assertStringContainsString('No aliases defined in ', $display);
$this->assertStringContainsString('no aliases defined in ', $display);
}
}
}
121 changes: 92 additions & 29 deletions tests/Phinx/Console/Command/MigrateTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,31 +3,29 @@

namespace Test\Phinx\Console\Command;

use DateTime;
use Phinx\Config\Config;
use Phinx\Config\ConfigInterface;
use Phinx\Console\Command\AbstractCommand;
use Phinx\Console\Command\Migrate;
use Phinx\Console\PhinxApplication;
use Phinx\Migration\Manager;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use RuntimeException;
use Symfony\Component\Console\Input\ArrayInput;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Output\StreamOutput;
use Symfony\Component\Console\Tester\CommandTester;

class MigrateTest extends TestCase
{
/**
* @var ConfigInterface|array
*/
protected $config = [];
private ConfigInterface $config;

/**
* @var InputInterface $input
*/
protected $input;
private InputInterface $input;

/**
* @var OutputInterface $output
*/
protected $output;
private OutputInterface $output;

protected function setUp(): void
{
Expand All @@ -50,7 +48,7 @@ protected function setUp(): void
]);

$this->input = new ArrayInput([]);
$this->output = new StreamOutput(fopen('php://memory', 'a', false));
$this->output = new StreamOutput(fopen('php://memory', 'ab'));
}

public function testExecute()
Expand All @@ -62,8 +60,8 @@ public function testExecute()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand Down Expand Up @@ -103,8 +101,8 @@ public function testExecuteWithDsn()
]);

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand All @@ -131,8 +129,8 @@ public function testExecuteWithEnvironmentOption()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand All @@ -157,8 +155,8 @@ public function testExecuteWithInvalidEnvironmentOption()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->never())
Expand All @@ -184,8 +182,8 @@ public function testDatabaseNameSpecified()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand All @@ -210,8 +208,8 @@ public function testFakeMigrate()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand All @@ -238,8 +236,8 @@ public function testMigrateExecutionOrder()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand Down Expand Up @@ -280,8 +278,8 @@ public function testMigrateMemorySqlite()
$command = $application->find('migrate');

// mock the manager class
/** @var Manager|\PHPUnit\Framework\MockObject\MockObject $managerStub */
$managerStub = $this->getMockBuilder('\Phinx\Migration\Manager')
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
Expand All @@ -301,4 +299,69 @@ public function testMigrateMemorySqlite()
]) . PHP_EOL, $commandTester->getDisplay());
$this->assertSame(AbstractCommand::CODE_SUCCESS, $exitCode);
}

public function testExecuteWithDate(): void
{
$application = new PhinxApplication();
$application->add(new Migrate());

/** @var Migrate $command */
$command = $application->find('migrate');

// mock the manager class
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->never())
->method('migrate');
$managerStub->expects($this->once())
->method('migrateToDateTime')
->with('development', new DateTime('yesterday'), false);

$command->setConfig($this->config);
$command->setManager($managerStub);

$commandTester = new CommandTester($command);
$exitCode = $commandTester->execute(
['command' => $command->getName(), '--environment' => 'development', '--date' => 'yesterday'],
['decorated' => false]
);

$this->assertStringContainsString('using environment development', $commandTester->getDisplay());
$this->assertSame(AbstractCommand::CODE_SUCCESS, $exitCode);
}

public function testExecuteWithError(): void
{
$exception = new RuntimeException('oops');

$application = new PhinxApplication();
$application->add(new Migrate());

/** @var Migrate $command */
$command = $application->find('migrate');

// mock the manager class
/** @var Manager&MockObject $managerStub */
$managerStub = $this->getMockBuilder(Manager::class)
->setConstructorArgs([$this->config, $this->input, $this->output])
->getMock();
$managerStub->expects($this->once())
->method('migrate')
->willThrowException($exception);

$command->setConfig($this->config);
$command->setManager($managerStub);

$commandTester = new CommandTester($command);
$exitCode = $commandTester->execute(
['command' => $command->getName(), '--environment' => 'development'],
['decorated' => false, 'capture_stderr_separately' => true]
);

$this->assertStringContainsString('using environment development', $commandTester->getDisplay());
$this->assertStringContainsString('RuntimeException: oops', $commandTester->getErrorOutput());
$this->assertSame(AbstractCommand::CODE_ERROR, $exitCode);
}
}
Loading