ReflectionClass: show enums differently from classes, test output #15956

DanielEScherzer · 2024-09-19T08:49:54Z

While internally enums are mostly the same as classes, their output in
ReflectionClass::__toString() should show the enum as the developer wrote it,
rather than as the engine stored it. Accordingly

Say that the enum is an enum, not a final class
Include the backing type, if any, in the declaration line
List enum cases separately from constants, and show the underlying values, if
any

Also add tests for the output - the tests are added in the first commit, so
that the changes in the second commit can be more easily confirmed.

GH-15766

nielsdos · 2024-09-19T21:07:40Z

I didn't look at the code, but in general I'm in favor of the idea. It's too late for 8.4 though, and this may have BC implications if people rely on the output. So when we start on 8.5 you may need to email the ML to pitch this.

DanielEScherzer · 2024-09-19T21:09:54Z

I didn't look at the code, but in general I'm in favor of the idea. It's too late for 8.4 though, and this may have BC implications if people rely on the output. So when we start on 8.5 you may need to email the ML to pitch this.

Yeah, there is no rush on this. Just wondering though, is the next version going to be 8.5, or 9.0? I wasn't able to find documentation on how many minor releases are done for each major release - since the github milestones show 8.4 and then 9.0, I assumed 9.0 was next - whichever it is, this can totally wait

nielsdos · 2024-09-19T21:12:39Z

The version will be bumped to 9.0 if it's necessary to do so (e.g. large engine changes).
The next version is most likely 8.5 unless decided otherwise.

DanielEScherzer · 2025-02-25T17:41:38Z

Mailing list discussion is at https://externals.io/message/126504, I'll rebase this patch once it is ready to merge

iluuu1994 · 2025-02-25T17:51:52Z

Remove the UnitEnum and BackedEnum interfaces from the list of interfaces implemented

Not sure there's a reason to do that. After all, reflection is intentionally explicit.

For comparison: https://3v4l.org/ecsMZ

Class [ class Foo implements Stringable ] {

DanielEScherzer · 2025-03-10T18:30:57Z

Remove the UnitEnum and BackedEnum interfaces from the list of interfaces implemented

Not sure there's a reason to do that. After all, reflection is intentionally explicit.

Reverted that part

iluuu1994

The code looks correct. I don't have any other objections.

ext/reflection/php_reflection.c

In preparation for improving it, phpGH-15766

While internally enums are mostly the same as classes, their output in `ReflectionClass::__toString()` should show the enum as the developer wrote it, rather than as the engine stored it. Accordingly - Say that the enum is an enum, not a final class - Include the backing type, if any, in the declaration line - List enum cases separately from constants, and show the underlying values, if any phpGH-15766

DanielEScherzer · 2025-03-20T20:14:32Z

Merging is blocked
Commits must have verified signatures.

Is this a new thing? Do I need to set up verified signatures now?

nielsdos · 2025-03-20T20:16:22Z

Merging is blocked
Commits must have verified signatures.

Is this a new thing? Do I need to set up verified signatures now?

Since we are the ones who're gonna merge it: no, we have to have signatures.
We already started requiring signatures after the xz incident last year, but the rule wasn't enforced on GH yet.

iluuu1994 · 2025-03-20T20:18:02Z

Indeed. Nonetheless, since merges made by the GH UI are signed using GH's own key, I don't understand why they would block this...

DanielEScherzer · 2025-03-20T20:20:49Z

Indeed. Nonetheless, since merges made by the GH UI are signed using GH's own key, I don't understand why they would block this...

Merges can be done in the UI? https://wiki.php.net/vcs/gitfaq says

You cannot use the Merge button on GitHub for that. The PHP Organization on GitHub will not accept contributors. It's only for mirroring!

Does that need update?

iluuu1994 · 2025-03-20T20:35:45Z

Does that need update?

Indeed, I fixed that just now.

After a quick chat with @TimWolla, it's probably best to disable this setting again. While the people with repo access should all sign their commits, we don't want to impose that on first contributors. The message makes it sound like it's an action the users must take, when that's not accurate. Unless we do a fast-forward merge, which doesn't happen too often, the signature is discarded anyway.

Unfortunate, but oh well.

iluuu1994 · 2025-03-20T20:37:20Z

Oh and @DanielEScherzer, it's still very much advisable for you to sign your commits, as you're a regular contributor. 🙂

iluuu1994

LGTM

So that we don't need to loop through all of the constants multiple times

Looping multiple times might even end up being faster due to avoiding allocations. We should pick whichever solution is the simplest.

That said, this solution is also fine.

DanielEScherzer · 2025-03-25T19:27:18Z

LGTM

So that we don't need to loop through all of the constants multiple times

Looping multiple times might even end up being faster due to avoiding allocations. We should pick whichever solution is the simplest.

That said, this solution is also fine.

I think the current version is simplest, so I plan to merge this in a few days if there is no other feedback

nielsdos

Minor nits, otherwise ok

nielsdos · 2025-03-25T20:00:41Z

ext/reflection/php_reflection.c

-	count = zend_hash_num_elements(&ce->constants_table);
-	smart_str_append_printf(str, "%s  - Constants [%d] {\n", indent, count);
-	if (count > 0) {
+	int total_count = zend_hash_num_elements(&ce->constants_table);


Suggested change

int total_count = zend_hash_num_elements(&ce->constants_table);

uint32_t total_count = zend_hash_num_elements(&ce->constants_table);

While we don't do warnings for unsigned->signed conversion and vice versa; it's still good practice to use the right type.
Also please then change the other count types to unsigned.

nielsdos · 2025-03-25T20:01:46Z

ext/reflection/php_reflection.c

+	smart_str_append_printf(str, "%sCase %s", indent, ZSTR_VAL(name));
+	if (c->ce->enum_backing_type == IS_UNDEF) {
+		// No value
+		smart_str_appends(str, "\n");


Suggested change

smart_str_appends(str, "\n");

smart_str_appendc(str, '\n');

github-actions bot added the Extension: reflection label Sep 19, 2024

DanielEScherzer force-pushed the reflection-enum branch from bc5d739 to 414b8e5 Compare October 17, 2024 22:41

DanielEScherzer force-pushed the reflection-enum branch from 414b8e5 to ea87e84 Compare March 10, 2025 18:30

iluuu1994 reviewed Mar 13, 2025

View reviewed changes

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

DanielEScherzer added 2 commits March 20, 2025 10:29

ReflectionClass: test enum output

0286dfa

In preparation for improving it, phpGH-15766

DanielEScherzer force-pushed the reflection-enum branch from ea87e84 to 8e8bb9a Compare March 20, 2025 17:41

iluuu1994 approved these changes Mar 25, 2025

View reviewed changes

nielsdos approved these changes Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReflectionClass: show enums differently from classes, test output #15956

ReflectionClass: show enums differently from classes, test output #15956

DanielEScherzer commented Sep 19, 2024 •

edited

Loading

nielsdos commented Sep 19, 2024

DanielEScherzer commented Sep 19, 2024

nielsdos commented Sep 19, 2024

DanielEScherzer commented Feb 25, 2025

iluuu1994 commented Feb 25, 2025 •

edited

Loading

DanielEScherzer commented Mar 10, 2025

iluuu1994 left a comment

DanielEScherzer commented Mar 20, 2025

nielsdos commented Mar 20, 2025 •

edited

Loading

iluuu1994 commented Mar 20, 2025

DanielEScherzer commented Mar 20, 2025

iluuu1994 commented Mar 20, 2025

iluuu1994 commented Mar 20, 2025

iluuu1994 left a comment

DanielEScherzer commented Mar 25, 2025

nielsdos left a comment

nielsdos Mar 25, 2025

nielsdos Mar 25, 2025

	int total_count = zend_hash_num_elements(&ce->constants_table);
	uint32_t total_count = zend_hash_num_elements(&ce->constants_table);

ReflectionClass: show enums differently from classes, test output #15956

Are you sure you want to change the base?

ReflectionClass: show enums differently from classes, test output #15956

Conversation

DanielEScherzer commented Sep 19, 2024 • edited Loading

nielsdos commented Sep 19, 2024

DanielEScherzer commented Sep 19, 2024

nielsdos commented Sep 19, 2024

DanielEScherzer commented Feb 25, 2025

iluuu1994 commented Feb 25, 2025 • edited Loading

DanielEScherzer commented Mar 10, 2025

iluuu1994 left a comment

Choose a reason for hiding this comment

DanielEScherzer commented Mar 20, 2025

nielsdos commented Mar 20, 2025 • edited Loading

iluuu1994 commented Mar 20, 2025

DanielEScherzer commented Mar 20, 2025

iluuu1994 commented Mar 20, 2025

iluuu1994 commented Mar 20, 2025

iluuu1994 left a comment

Choose a reason for hiding this comment

DanielEScherzer commented Mar 25, 2025

nielsdos left a comment

Choose a reason for hiding this comment

nielsdos Mar 25, 2025

Choose a reason for hiding this comment

nielsdos Mar 25, 2025

Choose a reason for hiding this comment

DanielEScherzer commented Sep 19, 2024 •

edited

Loading

iluuu1994 commented Feb 25, 2025 •

edited

Loading

nielsdos commented Mar 20, 2025 •

edited

Loading