[DanielEScherzer]

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

[ndossche]

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]

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

[ndossche]

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]

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

[iluuu1994]

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]

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.

[DanielEScherzer]

Merging is blocked
Commits must have verified signatures.

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

[ndossche]

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]

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]

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]

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]

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]

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

[ndossche]

Minor nits, otherwise ok