PHP: string form of enum contains "PB" prefix used to differentiate the constant from reserved words

[noahdietz]

In #3612 the step was taken to protect generated PHP code for Enums from use of PHP reserved words that would cause interpretation exceptions. e.g. an Enum value named CLASS = 1 would be generated as PBCLASS in PHP. This did the job.

However, as part of #5342, the string value that the PHP constant maps to also includes the PB prefix (which was prepended to the constant identifier to disambiguate it). Use of the name helper to map from Enum value to the string form retrieves a name that doesn't represent the proto. The same example as above would result in "PBCLASS" as the name, while the name for the value in the proto is CLASS. Here is a specific example in Google Ads.

Is this ideal? In my opinion it seems like a misrepresentation of the enum value names.

Could the PB prefix be excluded from the generated string form? I could see this being perceived as a breaking change if users were depending on the emergent behavior of names including the prefix.

I believe this is only really a concern for users of the generated name helper in enum classes. The JSON serialization code uses the proper string form I believe.

cc: @fiboknacky @bshaffer @dwsupplee

[bshaffer]

So in the case of CLASS, the enum class would generate the following?

private static $valueToName = [
        self::PBCLASS => 'PBCLASS',
];

Where it should be generating:

private static $valueToName = [
        self::PBCLASS => 'CLASS',
];

In this case, public static function value would still fail, as the user would be providing CLASS and so unless the function was changed to something like (emphasis on something like):

    public static function value($name)
    {
        $const = __CLASS__ . '::' . strtoupper($name);
        if (!defined($const)) {
            $pbconst = __CLASS__ . '::PB' . strtoupper($name);
            if (!defined($pbconst)) {
                throw new UnexpectedValueException(sprintf(
                    'Enum %s has no value defined for name %s', __CLASS__, $name));
            }
            return constant($pbconst);
        }
        return constant($const);
    }

I definitely see this as a bug, and fixing it in the above way at least would be backwards compatible in any case where users were specifically providing PBCLASS. In the breaking case where the string returned from public static function name is changed... I don't see how that would have been used successfully up to this point (since the API would reject it). So that BC break could be considered acceptable as well.

[noahdietz]

I don't see how that would have been used successfully up to this point (since the API would reject it).

I think you are right. If the user encodes their payload/params themselves (e.g. instead of using serializeToJsonString for a JSON payload), uses name() to get 'PBCLASS', I believe a backend could see this as an issue - unless it was expecting such a name and decodes it into CLASS properly (this is what I mean by users depending on the emergent behavior). Or the backend might be using value which would successfully decode it (because the backend also has a PBCLASS constant).

Personally, I don't think a breaking change WRT the value helper (even to fix an actual bug) is warranted here. But it still remains that the string 'PBCLASS', for example, might still be in use in user code so if name starts returning 'CLASS' their logic may break...I'm torn.

[bshaffer]

I consider a change like this an acceptable breakage, as anyone writing code reliant on a bug would hopefully consider the bug might get fixed. Also it's quite an edge case. But we can leave that decision up to the maintainers.

[fiboknacky]

Agree with all points, especially the acceptable breakage. It's pretty clear from the link that Noah provided, but please let me know if you need any more input.

[noahdietz]

Ok I will take a shot at the code changes starting with the backwards incompatible approach.

[noahdietz]

@fiboknacky the fix referencing this issue will be in the x.21.0 release early next month per #9780 (comment)

[fiboknacky]

Cool! Thanks for the update.

[fiboknacky]

@noahdietz Could you tell approximately when this will be released? Thanks.

[noahdietz]

It will be in the x.21.0 release early next month per #9780 (comment)