Skip to content

GH-15723: add ReflectionConstant::getFileName() - #15847

Closed
DanielEScherzer wants to merge 7 commits into
php:masterfrom
DanielEScherzer:constant-file
Closed

DanielEScherzer wants to merge 7 commits into
php:masterfrom
DanielEScherzer:constant-file

Conversation

@DanielEScherzer

@DanielEScherzer DanielEScherzer commented Sep 12, 2024 •

Copy link
Copy Markdown
Member

Allow determining the name of the file that defined a constant, when the constant was defined in userland code via define() or const. For constants defined by PHP core or extensions, false is returned, matching the existing getFileName() methods on other reflection classes.

@dstogov dstogov 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.

This adds new behavior in cost of increasing each zend_constant size.
New behavior requires the RFC process and can't go into the feature frozen PHP-8.4.
Personally, I don't see significant value. On the other hand, the cost is not significant.

@janedbal

Copy link
Copy Markdown

Personally, I don't see significant value.

One of the real-world usages I'd use it for is unused composer package analysis, see current issue

@iluuu1994

Copy link
Copy Markdown
Member

@DanielEScherzer Thank you for your PR! It's a bit late in the release process but you might still have time to consult the internals mailing list, and ask if there are any objections to adding this in 8.4.

New behavior requires the RFC process and can't go into the feature frozen PHP-8.4.

This is no longer accurate. Small feature additions have historically been allowed without requiring RFCs. It's generally understood that they are ok if announced on the ML with no objections. This is indeed vague, and I think we would benefit from specifying this process. Additionally, features are now allowed all the way up to release candidates since https://wiki.php.net/rfc/release_cycle_update.

@bwoebi

bwoebi commented Sep 12, 2024

Copy link
Copy Markdown
Member

I wonder whether it's reasonable to store the line number at the same time?

Comment thread ext/reflection/php_reflection.stub.php Outdated
@DanielEScherzer

DanielEScherzer commented Sep 16, 2024 •

Copy link
Copy Markdown
Member Author

I've sent an email to internals (my first one!) - https://news-web.php.net/php.internals/125571 // https://externals.io/message/125571
If this gets approved I'll deal with the merge conflict

@iluuu1994

Copy link
Copy Markdown
Member

There have been no objections on the ML, but unfortunately it has only been posted 5 days ago. 8.4 RC1 is tagged on Tuesday, so I would suggest delaying this for 8.5.

@DanielEScherzer

Copy link
Copy Markdown
Member Author

There have been no objections on the ML, but unfortunately it has only been posted 5 days ago. 8.4 RC1 is tagged on Tuesday, so I would suggest delaying this for 8.5.

Thats understandable, but annoying that this won't be included. Can we add the Waiting on RM label and see if they might be willing to include it? @php/release-managers-84

@janedbal

janedbal commented Oct 2, 2024

Copy link
Copy Markdown

How is this evolving? Any chance this gets into PHP 8.4 release? Thanks

@iluuu1994

Copy link
Copy Markdown
Member

@janedbal No, RC1 has landed. This will be delayed to 8.5.

@DanielEScherzer

Copy link
Copy Markdown
Member Author

Well, this missed 8.4, but since that has been branched I hope it can be merged for 8.5 - I've rebased to resolve the merge conflict and added an entry to UPGRADING

@iluuu1994 iluuu1994 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.

It seems a bit inconsistent not to offer getStartLine (getEndLine might be overkill).

Another note: We don't have this field or getStartLine on ReflectionClassConstant. The former we can get through getDeclaringClass though.

Comment thread Zend/zend_builtin_functions.c Outdated
Comment thread ext/reflection/tests/ReflectionConstant_getFileName.phpt Outdated
Comment thread ext/reflection/tests/ReflectionConstant_getFileName.phpt Outdated
@DanielEScherzer

Copy link
Copy Markdown
Member Author

It seems a bit inconsistent not to offer getStartLine (getEndLine might be overkill).

Another note: We don't have this field or getStartLine on ReflectionClassConstant. The former we can get through getDeclaringClass though.

I'd be happy to add getStartLine, but it means that we need to track the line where every constant is declared - I figured that without someone asking for it / having a use case, it would just add extra memory to every constant PHP tracks

Allow determining the name of the file that defined a constant, when the
constant was defined in userland code via `define()`. For constants defined by
PHP core or extensions, `false` is returned, matching the existing
`getFileName()` methods on other reflection classes.
@iluuu1994

iluuu1994 commented Oct 31, 2024 •

Copy link
Copy Markdown
Member

Looks good, I merged this now as there were no complaints on the list. Thanks!

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.

6 participants