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

A whole slew of changes for 3.0.x! See the summary. #1079

Merged
merged 14 commits into from
Nov 28, 2020

Conversation

TomHAnderson
Copy link
Member

@TomHAnderson TomHAnderson commented Nov 23, 2020

Q A
Type improvement
BC Break no

Summary

  • Updated box to 3.9.1
  • Allow for PDO namespace changes after DBAL 2.11
  • Reformatted composer.json and removed config platform of 7.2
  • Fixed at() calls in tests
  • Allow organize_migrations to be set to false with a new 'none' value
  • Add composer-normalize

Please note the branch name is now out of sync with this PR.

@TomHAnderson TomHAnderson force-pushed the feature/coding-standard-8.2 branch 2 times, most recently from 1f1d65a to eee6eb6 Compare November 24, 2020 02:22
@TomHAnderson TomHAnderson changed the title [wip] Upgraded to coding standard 8.2 Upgraded to coding standard 8.2; update box; allow new dbal pdo namespaces Nov 24, 2020
@TomHAnderson TomHAnderson changed the title Upgraded to coding standard 8.2; update box; allow new dbal pdo namespaces Upgraded to coding standard 8.2; update box; allow new dbal pdo namespaces for unit tests Nov 24, 2020
@stof
Copy link
Member

stof commented Nov 24, 2020

is it expected to change the indentation of composer.json ?

@TomHAnderson
Copy link
Member Author

I ran composer.json through the npm package json because it was formatted with multiple properties per line. That was intentional

@TomHAnderson TomHAnderson force-pushed the feature/coding-standard-8.2 branch from eee6eb6 to 4f6bea5 Compare November 24, 2020 08:39
@TomHAnderson TomHAnderson changed the title Upgraded to coding standard 8.2; update box; allow new dbal pdo namespaces for unit tests update box to 3.9.1; allow new dbal pdo namespaces for unit tests; remove php 7.2 composer config platform Nov 24, 2020
@TomHAnderson TomHAnderson changed the title update box to 3.9.1; allow new dbal pdo namespaces for unit tests; remove php 7.2 composer config platform update box to 3.9.1; allow new dbal pdo namespaces for unit tests; remove php 7.2 composer config platform; reformat composer.json Nov 24, 2020
@TomHAnderson TomHAnderson changed the title update box to 3.9.1; allow new dbal pdo namespaces for unit tests; remove php 7.2 composer config platform; reformat composer.json A whole slew of changes! See the summary. Nov 24, 2020
@TomHAnderson TomHAnderson changed the title A whole slew of changes! See the summary. A whole slew of changes for 3.0.x! See the summary. Nov 24, 2020
self::assertRegExp('/^Foo\\\\Version[0-9]{14}$/', $fqcn);

if (method_exists($this, 'assertMatchesRegularExpression')) {
$this->assertMatchesRegularExpression('/^Foo\\\\Version[0-9]{14}$/', $fqcn);
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should use the static implementation, so self

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't play well with phpstan

Copy link
Member Author

Choose a reason for hiding this comment

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

More info: method_exists cannot take self as the first parameter. So if you use $this for method_exists then use self for assertMatchesRegularExpression phpstan has a problem with that.

Copy link
Member

@greg0ire greg0ire Nov 25, 2020

Choose a reason for hiding this comment

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

What about using the class name (can't use $this::class for now since it's PHP 8 but TestCase::class should do the trick)?

Copy link
Member Author

@TomHAnderson TomHAnderson Nov 25, 2020

Choose a reason for hiding this comment

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

Nope. You see, phpstan is clearing (for lack of a better term) the use of $this after the method_exists function and seeing that it won't be called passes.

        if (method_exists(TestCase::class, 'assertMatchesRegularExpression')) {
            self::assertMatchesRegularExpression('/^Foo\\\\Version[0-9]{14}$/', $fqcn);

Results in

 ------ ----------------------------------------------------------------------------------------------------------------------------------
  Line   tests/Doctrine/Migrations/Tests/Generator/ClassNameGeneratorTest.php
 ------ ----------------------------------------------------------------------------------------------------------------------------------
  20     Call to an undefined static method Doctrine\Migrations\Tests\Generator\ClassNameGeneratorTest::assertMatchesRegularExpression().
 ------ ----------------------------------------------------------------------------------------------------------------------------------

@TomHAnderson
Copy link
Member Author

This repo needs to be set to a minimum PHP version of 7.3 in a future version so we can remove the check for the DBAL version which is deprecated and remove the logic for using using a regex assertion.

@TomHAnderson TomHAnderson force-pushed the feature/coding-standard-8.2 branch from 26b6502 to 2ac5b0f Compare November 24, 2020 22:08
.github/workflows/continuous-integration.yml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@greg0ire greg0ire added this to the 3.0.2 milestone Nov 26, 2020
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

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

looks good! thanks a lot @TomHAnderson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants