Align project settings with the other repositories #537

lcobucci · 2017-08-01T23:01:29Z

Requiring PHP 7.1+, dropping HHVM and simplifying some stuff on the test suite.

alcaeus · 2017-08-02T06:22:59Z

lib/Doctrine/DBAL/Migrations/Tools/Console/Command/AbstractCommand.php

     * @param InputInterface $input
     * @param OutputInterface $output
     * @return mixed
     */
-    protected function askConfirmation($question, InputInterface $input, OutputInterface $output)
+    protected function askConfirmation(string $question, InputInterface $input, OutputInterface $output)


Technically, this is a BC break - people might have overridden this method to add their own logic.

Indeed, will remove the type declaration.

Ocramius · 2017-08-02T09:44:59Z

lib/Doctrine/DBAL/Migrations/Migration.php

+                    ->write($sql, $direction);
+    }
+
+    protected function createSqlFileWriter(string $path): SqlFileWriter


private plz

I can't... and it really hurts me.
We were using mockery a big nasty black magic to overload the instantiation of that class. I didn't wan't to do a huge design change to make it work properly with only PHPUnit mocks.

Suggestion?

Got a link to the test? :-\

https://github.com/doctrine/migrations/pull/537/files#diff-5a519f363afe6bb6f788510621f527f0L147

Which now is https://github.com/doctrine/migrations/pull/537/files#diff-5a519f363afe6bb6f788510621f527f0R188

And allow us to remove this: https://github.com/doctrine/migrations/pull/537/files#diff-5a519f363afe6bb6f788510621f527f0L53

@lcobucci urgh. I am really reluctant to introduce a new protected method that will be abused till eternity :S

A huge test change would be more acceptable than this, IMO...

Ok, I'll go down the rabbit hole on this one. However it would probably not be only a test change.

Ocramius · 2017-08-02T09:45:30Z

lib/Doctrine/DBAL/Migrations/Version.php

+                    ->write($sqlQueries, $direction);
+    }
+
+    protected function createSqlFileWriter(string $path): SqlFileWriter


Unless designed for inheritance, private

Same reason as #537 (comment)

mikeSimonson

Thanks for the PR.
Great update.

mikeSimonson · 2017-08-02T10:21:13Z

composer.json

        "doctrine/coding-standard": "dev-master",
-        "mockery/mockery": "^0.9.4",
-        "johnkary/phpunit-speedtrap": "~1.0@dev",


@lcobucci Why are you removing speedtrap ?

Because the stable version doesn't support PHPUnit 6. Also I don't see much value on it right now, the only reason we had slow tests on the suite was the fact we were using mockery overload instance feature and that require tests to run in isolated processes, which has been removed.

I can add it back though 😄

mikeSimonson · 2017-08-02T10:22:38Z

lib/Doctrine/DBAL/Migrations/Tools/Console/Command/AbstractCommand.php

-        if (!$this->getHelperSet()->has('question')) {
-            return $this->getHelper('dialog')->askConfirmation($output, '<question>' . $question . '</question>', false);
-        }
-


There are tests that are tied to this BC feature that you can remove too.

Our coverage report said we weren't covering this flow, can you point out the test to me?

migrations/tests/Doctrine/DBAL/Migrations/Tests/Tools/Console/Command/AbstractCommandTest.php

Lines 309 to 336 in b82d683

public function testAskConfirmation()

{

$input = $this->getMockBuilder(ArrayInput::class)

->setConstructorArgs([[]])

->setMethods(['getOption'])

->getMock();

/**

* This test is testing a deprecated method.

* PHPunit convert those deprecations errors into tests failures.

* You can either use \PHPUnit_Framework_Error_Deprecated::$enabled = false;

* or use the @ operator to suppress the error.

* The advantage of the later is that it also remove the error message from the phpunit output.

*/

if (class_exists("Symfony\\Component\\Console\\Helper\\DialogHelper"))

{

@$helper = new DialogHelper();

@$this->assertTrue($this->invokeAbstractCommandConfirmation($input, $helper));

@$this->assertFalse($this->invokeAbstractCommandConfirmation($input, $helper, "n"));

}

if (class_exists("Symfony\\Component\\Console\\Helper\\QuestionHelper")) {

$helper = new QuestionHelper();

$this->assertTrue($this->invokeAbstractCommandConfirmation($input, $helper));

$this->assertFalse($this->invokeAbstractCommandConfirmation($input, $helper, "n"));

}

}

https://github.com/doctrine/migrations/blob/b82d683019fea695119b1972b07be77378186aff/tests/Doctrine/DBAL/Migrations/Tests/Tools/Console/Command/DialogSupport.php

mikeSimonson · 2017-08-02T10:23:22Z

.travis.yml

@@ -42,5 +42,4 @@ before_install:
  - if [[ ! $GITHUB_TOKEN ]]; then echo "no github token"; fi

 after_script:
-  - if [[ $TRAVIS_PHP_VERSION = '5.6' ]]; then php vendor/bin/coveralls -v ; fi


I am still using it. Is it a problem to keep it ?

It's not a big problem to keep it, I just saw on https://coveralls.io/github/doctrine/migrations that the last build was March 2015 so I assumed it was useless here as well

Are you sure we're using for doctrine/migrations? Do you want me to put it back?

mikeSimonson · 2017-08-02T10:26:37Z

.travis.yml

  - 7.1
-  - nigthly


I would like to keep nightly in there with allowed failures.

For some weird reason Travis was failing to run php nightly (version simply wasn't available) and the pipeline didn't work even though that one as allowed to fail. I will check it again.

That error seems unrelated, looks like you force-pushed the branch before Travis was able to grab the commit. :P

the pipeline didn't work even though that one as allowed to fail

Maybe there's a difference betwen errored and failed state.

I added it back, but I had a different error before. Let's see if it happens again.

Now it works... I'm probably getting crazier 😜

mikeSimonson · 2017-08-02T10:27:42Z

.travis.yml

-  fast_finish: true
-  include:
-    - php: 5.6
-      env: deps=low


We also need to keep a 7.1 run with deps low in the matrix.

It's here: https://github.com/doctrine/migrations/pull/537/files#diff-354f30a63fb0907d4ad57269548329e3R26

lcobucci · 2017-08-20T02:31:50Z

@Ocramius @mikeSimonson I think this is good to go, please take a look 😉

mikeSimonson · 2017-08-20T19:58:38Z

phpunit.xml.dist

-             charset="UTF-8" yui="true" highlight="true" lowUpperBound="5"
-             highLowerBound="70" />
-        <log type="coverage-clover" target="build/logs/clover.xml"/>
-    </logging>


Is there any reason to remove those two configurations ?
They are quite handy to work locally.

The rest of the PR is GTG for me.

@mikeSimonson you can always create your customized phpunit.xml locally: it overrides the .dist file and allows you to have custom settings.

Exactly 😁

Ocramius · 2017-08-21T06:20:53Z

lib/Doctrine/DBAL/Migrations/FileQueryWriter.php

+    }
+
+    /**
+     * TODO: move SqlFileWriter's behaviour to this class - and kill it with fire (on the next major release)


Can you spawn an issue for this TODO?

I was just waiting to get it merged to open the issueS 😜

Ocramius · 2017-08-21T06:21:31Z

lib/Doctrine/DBAL/Migrations/FileQueryWriter.php

+        $result = $writer->write($queriesByVersion, $direction);
+
+        // SqlFileWriter#write() returns `bool|int` but all clients expect it to be `bool` only
+        return $result !== false && $result > 0;


Isn't a (bool) cast sufficient?

Yes it is, silly me...

Ocramius · 2017-08-21T06:21:44Z

lib/Doctrine/DBAL/Migrations/FileQueryWriter.php

+     */
+    public function write(string $path, string $direction, array $queriesByVersion) : bool
+    {
+        $writer = new SqlFileWriter(


What about the injected writer?

I don't get what you mean, this is the implementation of the injected writer. I just don't wanted to implement everything here at this moment, it feels way too far from the scope of the PR.

Ah, missed that you are actually using $this->outputWriter 🤦‍♂️

Since we're actually not using it anymore.

Fixing a small issue that was preventing us to have at least the level 3.

Droping Mockery and making the test suite way faster.

Since we're now requiring >=3.3.

Since we're requiring 2.6 now.

So that we can remove hard dependencies from the code.

By relying on the configured query writer

Review changes applied

Ocramius · 2017-08-25T07:07:09Z

🚢

lcobucci added the Improvement label Aug 1, 2017

lcobucci added this to the 1.6 milestone Aug 1, 2017

lcobucci self-assigned this Aug 1, 2017

lcobucci force-pushed the align-project-settings branch 2 times, most recently from 4a886b9 to 1fa9490 Compare August 1, 2017 23:07

lcobucci requested review from mikeSimonson and Ocramius August 1, 2017 23:10

alcaeus reviewed Aug 2, 2017

View reviewed changes

lcobucci force-pushed the align-project-settings branch from ef96412 to 10963f8 Compare August 2, 2017 08:44

Ocramius suggested changes Aug 2, 2017

View reviewed changes

mikeSimonson previously requested changes Aug 2, 2017

View reviewed changes

lcobucci force-pushed the align-project-settings branch from 10963f8 to d036a11 Compare August 2, 2017 17:10

lcobucci requested review from Ocramius and mikeSimonson August 2, 2017 20:04

lcobucci mentioned this pull request Aug 8, 2017

Ability to change the class template #535

Merged

lcobucci force-pushed the align-project-settings branch from d036a11 to 96c507f Compare August 20, 2017 02:22

mikeSimonson reviewed Aug 20, 2017

View reviewed changes

Ocramius approved these changes Aug 21, 2017

View reviewed changes

Ocramius assigned Ocramius and unassigned lcobucci Aug 21, 2017

Ocramius suggested changes Aug 21, 2017

View reviewed changes

Ocramius assigned lcobucci and unassigned Ocramius Aug 21, 2017

lcobucci added 6 commits August 21, 2017 10:45

Remove coveralls configuration

5bb9a16

Since we're actually not using it anymore.

Add phpstan configuration

6ce9a3c

Fixing a small issue that was preventing us to have at least the level 3.

Require PHP 7.1 and drop HHVM

9f307aa

Upgrade to PHPUnit 6

0636cc6

Droping Mockery and making the test suite way faster.

Remove compatibility with symfony/console 2.x

de6211d

Since we're now requiring >=3.3.

Fix CS and add type declaration in some private methods

f57374a

lcobucci added 4 commits August 21, 2017 10:45

Remove DBAL 2.4 compatibility

213666e

Since we're requiring 2.6 now.

Add an injectable query writer

fd09643

So that we can remove hard dependencies from the code.

Add query writer to configuration object

42ad1db

Remove coupling with SqlFileWriter

972783a

By relying on the configured query writer

lcobucci force-pushed the align-project-settings branch from 96c507f to 972783a Compare August 21, 2017 08:46

lcobucci requested review from Ocramius and mikeSimonson August 21, 2017 08:46

Ocramius approved these changes Aug 25, 2017

View reviewed changes

Ocramius merged commit 17f61e9 into master Aug 25, 2017

Ocramius deleted the align-project-settings branch August 25, 2017 07:07

Ocramius assigned Ocramius and unassigned lcobucci Aug 25, 2017

lcobucci mentioned this pull request Aug 26, 2017

Move SqlFileWriter's behaviour to FileQueryWriter #541

Closed

	public function testAskConfirmation()
	{
	$input = $this->getMockBuilder(ArrayInput::class)
	->setConstructorArgs([[]])
	->setMethods(['getOption'])
	->getMock();

	/**
	* This test is testing a deprecated method.
	* PHPunit convert those deprecations errors into tests failures.
	* You can either use \PHPUnit_Framework_Error_Deprecated::$enabled = false;
	* or use the @ operator to suppress the error.
	* The advantage of the later is that it also remove the error message from the phpunit output.
	*/
	if (class_exists("Symfony\\Component\\Console\\Helper\\DialogHelper"))
	{
	@$helper = new DialogHelper();

	@$this->assertTrue($this->invokeAbstractCommandConfirmation($input, $helper));
	@$this->assertFalse($this->invokeAbstractCommandConfirmation($input, $helper, "n"));
	}

	if (class_exists("Symfony\\Component\\Console\\Helper\\QuestionHelper")) {
	$helper = new QuestionHelper();
	$this->assertTrue($this->invokeAbstractCommandConfirmation($input, $helper));
	$this->assertFalse($this->invokeAbstractCommandConfirmation($input, $helper, "n"));
	}
	}

Align project settings with the other repositories #537

Align project settings with the other repositories #537

Conversation

lcobucci commented Aug 1, 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

mikeSimonson 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

lcobucci Aug 2, 2017 • edited

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

lcobucci commented Aug 20, 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

Ocramius commented Aug 25, 2017

lcobucci Aug 2, 2017 •

edited