Skip to content

Conversation

SerkanYildiz
Copy link
Contributor

@SerkanYildiz SerkanYildiz commented Oct 25, 2018

Validator should also check if the given class name is a reserved keyword or not.
However we have to come up with a better exception message, since this one does not reflect the error. Any suggestion?

Thanks for your help @weaverryan during #SymfonyConHackDay2018

Fixes #305

@SerkanYildiz SerkanYildiz force-pushed the checking-on-reserved-keywords-for-class-validitiy branch 2 times, most recently from ca02938 to d712b11 Compare October 25, 2018 13:48
src/Str.php Outdated
'use', 'var', 'while', 'xor', '__CLASS__', '__DIR__', '__FILE__',
'__FUNCTION__', '__LINE__', '__METHOD__', '__NAMESPACE__', '__TRAIT__',
'int', 'float', 'bool', 'string', 'true', 'false', 'null', 'void',
'iterable', 'object',
Copy link
Contributor

Choose a reason for hiding this comment

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

'int', 'float', 'bool', 'string', 'true', 'false', 'null', 'void',
'iterable', 'object',

This words are allowed to use in Namespace, anyway it will be very strange to use it :-)

@@ -29,7 +29,7 @@ public static function validateClassName(string $className, string $errorMessage
$pieces = explode('\\', ltrim($className, '\\'));

foreach ($pieces as $piece) {
if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $piece)) {
if (!Str::isValidPhpClassName($piece)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe will be better not to change this check, but add a new check with new error message, to show the user what is wrong with classname?

@SerkanYildiz SerkanYildiz force-pushed the checking-on-reserved-keywords-for-class-validitiy branch from 40aa3ca to 00cf423 Compare October 26, 2018 11:27
@SerkanYildiz
Copy link
Contributor Author

@sadikoff Thanks for your review! Wdyt about my current fix?

@sadikoff
Copy link
Contributor

@SerkanYildiz It's great that you have done it so fast, but I was talking about something else. I suggested not to split namespace and class validator, but leave old validation in place and add new one which will validate reserved words
ex:

foreach ($pieces as $piece) {
    if (!preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $piece)) {
        $errorMessage = $errorMessage ?: sprintf('"%s" is not valid as a PHP class name (it must start with a letter or underscore, followed by any number of letters, numbers, or underscores)', $className);
        throw new RuntimeCommandException($errorMessage);
    }
    if (Str::isPhpReservedWord($piece)) {
        throw new RuntimeCommandException(sprintf('"%s" is a reserved keyword and thus cannot be used as class name in PHP.', $className));
    }
}

@weaverryan
Copy link
Member

Yea... I think I agree. So remove Str::isValidPhpClassName() and just move that logic into Validator:: validateClassName() as @sadikoff.

But otherwise, this is awesome! I like tightening things up :)

@weaverryan
Copy link
Member

Ping @SerkanYildiz! I think we need just one more small change. Does my description above make sense?

Cheers!

@SerkanYildiz
Copy link
Contributor Author

Pong @weaverryan! Will fix this today, thanks for helping!

@SerkanYildiz SerkanYildiz force-pushed the checking-on-reserved-keywords-for-class-validitiy branch from 00cf423 to 6cebac0 Compare November 5, 2018 16:06
@SerkanYildiz
Copy link
Contributor Author

@weaverryan Pushed the requested changes. Does it look good now?

@SerkanYildiz SerkanYildiz force-pushed the checking-on-reserved-keywords-for-class-validitiy branch 2 times, most recently from 3d40de6 to 535d287 Compare November 6, 2018 09:51
@SerkanYildiz
Copy link
Contributor Author

@weaverryan Tests are failing due another error. Should I do something?

@SerkanYildiz SerkanYildiz force-pushed the checking-on-reserved-keywords-for-class-validitiy branch from 535d287 to 0b90e3c Compare December 8, 2018 14:19
@SerkanYildiz SerkanYildiz force-pushed the checking-on-reserved-keywords-for-class-validitiy branch from 0b90e3c to f308470 Compare December 8, 2018 14:50
@weaverryan
Copy link
Member

Thank you @SerkanYildiz!

@weaverryan weaverryan merged commit f308470 into symfony:master Dec 13, 2018
weaverryan added a commit that referenced this pull request Dec 13, 2018
…SerkanYildiz)

This PR was squashed before being merged into the 1.0-dev branch (closes #306).

Discussion
----------

Checking on reserved keywords for class name validity.

Validator should also check if the given class name is a reserved keyword or not.
However we have to come up with a better exception message, since this one does not reflect the error. Any suggestion?

Thanks for your help @weaverryan during #SymfonyConHackDay2018

Fixes #305

Commits
-------

f308470 Move validation logic into Validator.
a419b8f Checking on reserved keywords for class name validity.
@SerkanYildiz SerkanYildiz deleted the checking-on-reserved-keywords-for-class-validitiy branch December 14, 2018 16:32
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