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

Property hooks can be marked final - expose that in reflection API #1474

Merged
merged 2 commits into from
Dec 30, 2024

Conversation

kukulich
Copy link
Collaborator

No description provided.

@kukulich kukulich added the bug label Dec 30, 2024
@Ocramius Ocramius added this to the 6.50.0 milestone Dec 30, 2024
@Ocramius Ocramius self-assigned this Dec 30, 2024
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.

Looking good!

Comment on lines +310 to 322
$modifiers = 0;

if ($node instanceof MethodNode) {
$modifiers += $node->isStatic() ? CoreReflectionMethod::IS_STATIC : 0;
$modifiers += $node->isPublic() ? CoreReflectionMethod::IS_PUBLIC : 0;
$modifiers += $node->isProtected() ? CoreReflectionMethod::IS_PROTECTED : 0;
$modifiers += $node->isPrivate() ? CoreReflectionMethod::IS_PRIVATE : 0;
$modifiers += $node->isAbstract() ? CoreReflectionMethod::IS_ABSTRACT : 0;
}

$modifiers += $node->isFinal() ? CoreReflectionMethod::IS_FINAL : 0;

return $modifiers;
Copy link
Member

Choose a reason for hiding this comment

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

BTW, no idea how PHP optimizes this, but it's a giant cascade of ternaries D:

I wonder if this could be a single bitwise or operation:

$modifiers = $modifiers |
    ($node->isStatic() & CoreReflectionMethod::IS_STATIC) |
    ($node->isPublic() & CoreReflectionMethod::IS_PUBLIC) |
    ($node->isProtected() & CoreReflectionMethod::IS_PROTECTED) |
    ($node->isPrivate() & CoreReflectionMethod::IS_PRIVATE) |
    ($node->isAbstract() & CoreReflectionMethod::IS_ABSTRACT);

Not for this patch, just for future use-cases. The method calls here are still the biggest offender.

@Ocramius
Copy link
Member

Promoted from bug to improvement, since this augments hooks, rather than fixing a regression.

@Ocramius Ocramius marked this pull request as ready for review December 30, 2024 12:42
@Ocramius Ocramius changed the title Property hook can be final Property hooks can be marked final - expose that in reflection API Dec 30, 2024
@Ocramius Ocramius merged commit 3aaa022 into Roave:6.50.x Dec 30, 2024
30 checks passed
@Ocramius
Copy link
Member

Thanks @kukulich!

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.

2 participants