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

Improve class name checking in GeneratorCommand #38965

Closed
wants to merge 4 commits into from

Conversation

schrosis
Copy link

Summary

  • Support for class names with subdirectories
  • Checking for valid class names as PHP
  • Add reserved words

Support for class names with subdirectories

GeneratorCommand checks if the given name is a reserved word.
image

However, a name with a subdirectory will pass this check.
image

Changed to check for simple class names without the subdirectory part.

Checking for valid class names as PHP

This change will check for invalid class names such as 2021MyClass String+Helper

The regular expression used can be found here.
https://www.php.net/manual/en/language.oop5.basic.php

Add reserved words

Add missing reserved words and reserved words to be added in PHP 8.1.

  • int
  • float
  • bool
  • string
  • true
  • false
  • null
  • void
  • iterable
  • object
  • match
  • enum
  • readonly

https://www.php.net/manual/en/reserved.other-reserved-words.php
https://github.com/php/php-src/blob/master/Zend/zend_language_parser.y#L136
https://github.com/php/php-src/blob/master/Zend/zend_language_parser.y#L166
https://github.com/php/php-src/blob/master/Zend/zend_language_parser.y#L157

Screenshot

image


I'm new to contributing to OSS, so please let me know if I'm missing anything.

'list',
'match',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mixed is also reserved

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

As you said, mixed seems to work as a reserved word as well.
I also found a PR in the language documentation.
php/doc-en#938

Likewise, it looks like never will be added in PHP 8.1!

@taylorotwell
Copy link
Member

I'm not particularly interested in these types of checks. It is up to the user to not create invalid class names.

@schrosis
Copy link
Author

@taylorotwell
Thanks for the review!
I agree with you.

However, there is already a check for incomplete reserved words.
I think this should be maintained or removed if it is not needed.

While I think it's the user's responsibility to avoid creating invalid class names, on the other hand, as mentioned in the PR where the reserved word check was added, it's not elegant to have the process fail in the middle and leave a contaminated file.
#33037

For that reason, I think it's a good idea to be able to check beforehand!

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

Successfully merging this pull request may close these issues.

3 participants