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

[PHP8] Add initial compatibility #1106

Merged
merged 9 commits into from
Dec 4, 2020
Merged

Conversation

mfn
Copy link
Collaborator

@mfn mfn commented Dec 3, 2020

Summary

  • the codebase seems compatible except the one place where a deprecated ReflectionParameter->getClass() was called
    • replaced it with ->getType()
    • seems it also improve the type declarations which are generated (test GeneratePhpdocWithFqn)
  • Lumen 6 and 7 don't support PHP8, so excluded from integration tests
  • friendsofphp/php-cs-fixer is (not yet) PHP 8 compatible, thus removed it from the unit tests matrix
    Same was already done for vimeo/psalm in the past
  • mockery minimum version had to be bumped to 1.3.3 for PHP8 compat
    figured the bump is fine as it's a dev dependency only anyway
  • spatie/phpunit-snapshot-assertions required ^4 for PHP8 compat, so it's added
  • in one test we checked the literal PHP errors, which changed from PHP < 8 and PHP 8, so added an if condition to DynamicRelations/Test.php

The biggest issue though is that doctrine/dbal with prefer-lowest in the test matrix is NOT compatible with PHP8, it throws some method declaration mismatch exception.

  • Our minimum is 2.3 which also supports PHP 7.2
  • the minimum for PHP8 compat is dbal 2.12.0, however starting with dbal 2.11.0 only PHP 7.3 is supported, clashing with our minimum supported version of 7.2 (for Laravel/Lumen 6)

=> therefore I decided to not bump doctrine/dbal in this PR and just don't run the unit test matrix on PHP8 with prefer-lowest until we've a better solution.

The biggest issue though is that doctrine/dbal with prefer-lowest in the test matrix is NOT compatible with PHP8, it throws some method declaration mismatch exception.

I wonder though why this wouldn't be handled by composer dependency resolution correctly, IDK TBH.

Links

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

mfn added 8 commits December 3, 2020 01:34
The composer.json constraint is unbound, so it's already "allowed" at least.
- not necessary anyway
- not compatible with PHP8 currently
This is the minimum version also supporting PHP8
Some lower version requirements like doctrone/dbal won't work and
would require at least dbal 2.12.0, which in turn doesn't support PHP 7.2
anymore.

So instead of bumping dbal and excluding PHP 7.2 users, we ignore the
lowest version for PHP 8 for the time being.
@mfn mfn self-assigned this Dec 3, 2020
@@ -424,7 +424,7 @@ protected function getPropertiesFromTable($model)

$database = null;
if (strpos($table, '.')) {
list($database, $table) = explode('.', $table);
[$database, $table] = explode('.', $table);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated but Phpstorm just insists in changed it 💥

Copy link
Owner

@barryvdh barryvdh left a comment

Choose a reason for hiding this comment

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

Nice!

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

Successfully merging this pull request may close these issues.

[PHP8] Could not analyze class X, exception: method ReflectionParameter::getClass() is deprecated
2 participants