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 - Adapter Signatures Compatibility #712

Merged

Conversation

michaeljoelphillips
Copy link
Contributor

@michaeljoelphillips michaeljoelphillips commented Dec 12, 2020

This PR takes a stab at the first item listed in #701 concerning PHP8 support.

What should be done about the missing constants on \ReflectionClassConstant for PHP < 8.0? In this PR I am declaring them on Roave\BetterReflection\Reflection\ReflectionClassConstant. I am assuming that the expectation for users on PHP7 will be to have some access to constants for the filter values, but I am open to feedback.

Since they are currently undocumented, here are the constants for reference:

php > $reflection = new ReflectionClass(ReflectionClassConstant::class);
php > var_dump($reflection->getConstants());
array(3) {
  ["IS_PUBLIC"]=>
  int(1)
  ["IS_PROTECTED"]=>
  int(2)
  ["IS_PRIVATE"]=>
  int(4)
}

@michaeljoelphillips michaeljoelphillips force-pushed the adapter-signature-compatibility branch from 57e525a to b4e0780 Compare December 12, 2020 07:44
@Ocramius Ocramius added dependencies Pull requests that update a dependency file enhancement labels Dec 14, 2020
@Ocramius Ocramius added this to the 4.12.0 milestone Dec 14, 2020
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Patch looks good, but we need to get rid of the design implications of core reflection API leaking into the new API: filtering logic should therefore be moved from the src/Reflection to the src/Reflection/Adapter logic, to avoid having ugly/complex logic like bitmask filtering exposed in the BC-compliant API.

src/Reflection/ReflectionClassConstant.php Outdated Show resolved Hide resolved
src/Reflection/ReflectionClass.php Outdated Show resolved Hide resolved
src/Reflection/ReflectionClass.php Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

Moved to 4.13.0. Reminder to self: change base branch after 4.12.0 is released

@Ocramius Ocramius changed the base branch from 4.12.x to 4.13.x December 14, 2020 18:20
@michaeljoelphillips
Copy link
Contributor Author

@Ocramius, I moved the filter logic to the adapter layer for both ReflectionClass and ReflectionObject and added tests. Thanks for the feedback.

@Ocramius
Copy link
Member

@michaeljoelphillips can you expand on

strategy:
matrix:
dependencies:
- "lowest"
- "highest"
- "locked"
php-version:
- "7.4"
operating-system:
- "ubuntu-latest"
- "windows-latest"
to add php: 8.0 to it, perhaps?

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

The addition of PHP 8.x to CI will happen separately, I suppose :-)

Thanks for the clarification comments and work on this, @michaeljoelphillips!

@Ocramius Ocramius merged commit 8d94aa3 into Roave:4.13.x Jan 7, 2021
@Ocramius Ocramius modified the milestones: 4.13.0, 5.0.0 Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants