Skip to content

Add ReflectionConstant::getExtension() and ::getExtensionName() - #16603

Merged
ndossche merged 1 commit into
php:masterfrom
DanielEScherzer:reflection-constant-extension
Nov 9, 2024
Merged

Add ReflectionConstant::getExtension() and ::getExtensionName()#16603
ndossche merged 1 commit into
php:masterfrom
DanielEScherzer:reflection-constant-extension

Conversation

@DanielEScherzer

@DanielEScherzer DanielEScherzer commented Oct 25, 2024

Copy link
Copy Markdown
Member

No description provided.

@ndossche

ndossche commented Oct 25, 2024

Copy link
Copy Markdown
Member

In favor.
I'm just not sure we need both a getExtension and getExtensionName method as we can trivially get the name from a ReflectionExtension with only a few extra keystrokes.
I'm also not too sure about the string|false return.

@DanielEScherzer

DanielEScherzer commented Oct 25, 2024

Copy link
Copy Markdown
Member Author

In favor. I'm just not sure we need both a getExtension and getExtensionName method as we can trivially get the name from a ReflectionExtension with only a few extra keystrokes. I'm also not too sure about the string|false return.

I have no problem removing the name getter, but the request at #16571 included it noting that there is both a getExtension() and getExtensionName() for ReflectionClass and ReflectionFunction, so only having one of them might be a bit odd. Perhaps the getExtensionName() methods on the others should be deprecated?

@ndossche

ndossche commented Oct 25, 2024

Copy link
Copy Markdown
Member

No this PR is fine. It's more important to be consistent with what we have; even if it's a bit odd.
I wouldn't want to deprecate those as it doesn't cause harm.

ndossche
ndossche reviewed Oct 25, 2024
View reviewed changes

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work, only 2 nits.

Comment thread ext/reflection/php_reflection.c Outdated
Comment thread ext/reflection/php_reflection.c Outdated
@DanielEScherzer DanielEScherzer changed the title GH-16571: add ReflectionConstant::getExtension() and ::getExtensionName() Add ReflectionConstant::getExtension() and ::getExtensionName() Oct 25, 2024
ndossche
ndossche approved these changes Oct 25, 2024
View reviewed changes

@ndossche ndossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems fine to me, but let's wait a bit for others to chime in

@ndossche ndossche linked an issue Oct 25, 2024 that may be closed by this pull request
Add ReflectionConstant::getExtension() #16571
Closed
@ndossche

ndossche commented Oct 28, 2024

Copy link
Copy Markdown
Member

No reaction yet; perhaps shoot a quick email to the mailing list with a link to this PR to see if anyone objects. If no one complains we can just merge it.

Girgias
Girgias approved these changes Oct 29, 2024
View reviewed changes

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, looks like an oversight

@DanielEScherzer
DanielEScherzer force-pushed the reflection-constant-extension branch from e310657 to 59949e3 Compare November 1, 2024 07:19
@DanielEScherzer

DanielEScherzer commented Nov 1, 2024

Copy link
Copy Markdown
Member Author

<rebased, conflicts with #15847>

@ndossche

ndossche commented Nov 9, 2024

Copy link
Copy Markdown
Member

No one complained, I'm merging this.

@ndossche
ndossche merged commit 10f1f92 into php:master Nov 9, 2024
@ndossche

ndossche commented Nov 9, 2024

Copy link
Copy Markdown
Member

Thanks!

@DanielEScherzer
DanielEScherzer deleted the reflection-constant-extension branch November 9, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndossche ndossche ndossche approved these changes
@Girgias Girgias Girgias approved these changes
@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner

Assignees

No one assigned

Labels

Extension: reflection

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add ReflectionConstant::getExtension()

3 participants

@DanielEScherzer @ndossche @Girgias