Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [PHP] add missing reserved classnames #9458

Merged
merged 6 commits into from Feb 10, 2022

Conversation

bshaffer
Copy link
Contributor

Protobuf generates class with the name Parent in PHP (see this PR). This is an invalid class name, and results in a fatal error. The same is true for the word self:

https://www.php.net/manual/en/reserved.classes.php

Following identifiers may not be used as a class name as they have special purpose.

Our current list represents PHP "keywords", which have almost the same restrictions in PHP, but are slightly more restricted:

https://www.php.net/manual/en/reserved.keywords.php

The following words cannot be used as constants, class names, function or method names. They are, however, allowed as property, constant, and method names of classes, interfaces and traits, except that class may not be used as constant name.

The difference here is keywords cannot be used in global functions or constants. As protobuf for PHP does not generate any global functions or constants, they can be treated as the same, and do not need special treatment (such as kReservedClasses).

This is not a backwards-compatibility breaking change, as any protobuf objects with the names self or parent would be throwing PHP fatal errors if generated today.

@bshaffer
Copy link
Contributor Author

bshaffer commented Feb 4, 2022

@acozzette I fixed the tests - I missed adding them to the kValidConstantNames array. This PR should be good to go now 🤞

@bshaffer
Copy link
Contributor Author

bshaffer commented Feb 8, 2022

@acozzette okay they're really fixed this time I promise!

@acozzette acozzette merged commit ce57b5b into protocolbuffers:master Feb 10, 2022
@acozzette
Copy link
Member

Thanks, @bshaffer.

@bshaffer bshaffer deleted the add-protected-class-names branch February 10, 2022 17:40
@bshaffer
Copy link
Contributor Author

@acozzette my pleasure! Thanks for the quick review.

Do you know when this fix will be available in a release?

@acozzette
Copy link
Member

I'm hoping we can do a release next week, but I need to check with the rest of the team on that. We will likely do a release by the end of next month at the latest, but hopefully sooner than that.

@andreladocruz
Copy link

andreladocruz commented Apr 3, 2022

Got this error:

RUN pecl install -o -f protobuf \
    &&  rm -rf /tmp/pear \
    &&  docker-php-ext-enable protobuf

19 12.77 /tmp/pear/temp/pear-build-defaultuserjglifJ/protobuf-3.20.0/libtool: line 1290: can't create third_party/utf8_range/naive.loT: nonexistent directory
19 12.77 mkdir third_party/utf8_range/.libs
19 12.77 mkdir: can't create directory 'third_party/utf8_range/.libs': No such file or directory
19 12.77 make: *** [Makefile:226: third_party/utf8_range/naive.lo] Error 1
19 12.77 ERROR: `make' failed

@icatalina
Copy link

👋 I just noticed Object is not listed as a reserved class name in the php documentation, but it is:

PHP Fatal error:  Cannot use 'Object' as class name as it is reserved in Object.php on line 14

Fatal error: Cannot use 'Object' as class name as it is reserved in Object.php on line 14

Errors parsing Object.php

I've already raised an issue with them: php/doc-en#1518

@damianwadley
Copy link

damianwadley commented Apr 13, 2022

Object it isn't a reserved class name. It is a reserved word and comes with broader restrictions than just as a class name.
https://www.php.net/manual/en/reserved.other-reserved-words.php

@icatalina
Copy link

Hey @damianwadley, sorry for the confusion. I suppose being a reserved word makes the Object class "reserved" because you can't name a class like that. It sure encompasses more than just class names as you pointed out.

In this particular case, protobuf is generating a class called Object which isn't valid php after 7.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants