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

StaticReflectionClass PHP7.2 issue #822

Closed

Conversation

alexpott
Copy link

@alexpott alexpott commented Nov 2, 2017

Unfortunately PHP7.2 \ReflectionClass has slightly different semantics which means that \Doctrine\Common\Reflection\StaticReflectionClass has issues working on both PHP7.1 (and earlier) and PHP7.2.

The error you get on PHP7.2 is:
Declaration of Doctrine\Common\Reflection\StaticReflectionClass::newInstance($args) should be compatible with ReflectionClass::newInstance(...$args)

An earlier PR on this problem was #775 (review)

@alexpott
Copy link
Author

alexpott commented Nov 2, 2017

The first commit is a tests only situation where we should see PHP7.1 pass but PHP7.2 fail.

@Majkl578
Copy link
Contributor

Majkl578 commented Nov 2, 2017

This looks like a PHP 7.2 BC break, have you reported it? https://3v4l.org/7igHL

@alexpott
Copy link
Author

alexpott commented Nov 2, 2017

@Majkl578 yep - it was reported in php/php-src#2363 (comment) - I guess the comment there suggests that the rollback should have gone to master too.

@Ocramius
Copy link
Member

Ocramius commented Nov 2, 2017

Relevant: Roave/BetterReflection#317

if (version_compare(phpversion(), '7.2', '<')) {
include __DIR__ . '/legacy/StaticReflectionClass.php';
}
else {

Choose a reason for hiding this comment

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

Let's do the version check in a trait definition and just use that trait in this class to swap out the method.

@alexpott
Copy link
Author

alexpott commented Nov 2, 2017

Thanks @Ocramius that's super simple and appears to work for both 7.1 and 7.2. Yay!

@dawehner
Copy link
Contributor

dawehner commented Nov 2, 2017

@Ocramius You are a genius. Thank you!

@Majkl578
Copy link
Contributor

Majkl578 commented Nov 2, 2017

I'd rather wait for PHP to get this fixed on their side... There's still time to fix it before 7.2 goes stable.
There's already a bug report for this: https://bugs.php.net/bug.php?id=74292

@EclipseGc
Copy link

But this won't work for 5.5 will it?

@Ocramius
Copy link
Member

Ocramius commented Nov 2, 2017 via email

@EclipseGc
Copy link

Right, I was making the point for Drupal which is still supporting 5.5 but would also like to support 7.2. The trait based approach (with an include for the 7.2 specific code) is the best solution for our needs I think.

@alexpott
Copy link
Author

alexpott commented Nov 7, 2017

So PHP have decided to rollback the BC break.

@alexpott alexpott closed this Nov 7, 2017
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.

None yet

5 participants