Change enum string name for reserved words

[noahdietz]

Changes the string name associated with an enum that uses a reserved word to be the enum name without the PB prefix that the constant gets. For example the enum value CLASS would be generated as const PBCLASS and the string name returned by the name() helper for it will be 'CLASS' instead of 'PBCLASS'.

The value helper will accept either 'CLASS' or 'PBCLASS' because it will attempt to prefix the string with 'PB' before looking it up again. This "extra check" logic will only be generated for those enums that use a reserved word as a value.

This is a breaking change to generated PHP code that depended on the value returned by name() being prefixed with PB. This is a rare use case. Furthermore, the name() function exists for two primary reasons:

1. The REST API accepts the string names, not the constants
2. To make the enum values more easily human-readable in debugging

For the 1st case, the "PB" prefix would throw an error in the REST API, and for the 2nd case, there is no reliant code on the prefix behavior. Worth noting, serializeToJsonString does not use the name() function to serialize the enum into the JSON blob.

Thank you @bshaffer for the concise words describing the scenarios in question.

Fixes #9763

[noahdietz]

cc @bshaffer

[noahdietz]

I understand the breaking nature of this change, while hyper focused and could be considered fixing already broken generated code, might be controversial. I opened this to get the ball rolling after a bit of discussion in the issue.

[noahdietz]

cc @haberman

[haberman]

Please be more specific in the PR description about exactly what cases will break backward compatibility. As I understand it, the cases of breakage are rare.

[noahdietz]

Please be more specific in the PR description about exactly what cases will break backward compatibility. As I understand it, the cases of breakage are rare.

Done!

[noahdietz]

Hi @haberman what can we expect on timing here? Will this be accepted for the upcoming release or afterwards? Thanks!

[haberman]

We will merge this soon and it will go into the .21.0 release early next month.

I think the description somewhat overstates the breakage. As I understand it we expect it to be very rare that anyone will actually experience breakage, because:

1. If anyone was using this for JSON, it would have already been broken, since JSON won't accept names like PBCLASS.
2. If anyone is using the string for human-readable output, this change will not break them.

So we don't really know of any cases were we expect this to be truly breaking.