Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

[CodingStandard] ClassStringToClassCosntantFixer init #262

Merged
merged 13 commits into from
Jul 15, 2017

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Jul 14, 2017

Closes #260

Converts: SomeClass => \SomeClass::class

@TomasVotruba
Copy link
Member Author

@dg I've implemented the ::class checker. Could you briefly check it?

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Jul 14, 2017

@SpacePossum @keradus Could I ask you guys for short review?

{
foreach (array_reverse($tokens->toArray(), true) as $index => $token) {
/** @var Token $token */
if (! $this->isStringToken($token)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is possible directly look for code T_CONSTANT_ENCAPSED_STRING ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, that's the token I was looking for! Thank you

}

$potentialClassOrInterfaceName = trim($token->getContent(), "'");
if (class_exists($potentialClassOrInterfaceName) || interface_exists($potentialClassOrInterfaceName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Class may not exist, because ECS is used for projects that uses own classes, so better is regexp checking. I am using this:

			if (preg_match('#^[A-Z]\w*[a-z]\w*(\\\\[A-Z]\w*[a-z]\w*)*\z#', $potentialClassOrInterfaceName)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I will try it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is, it replaces non existing classes as well: 9a14c3b#diff-1ca8f9425dbe99f88cb78b89459d3a6e

So I'd probably stay with *_exists() function or do you know how to solve it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sry, the regexp should be '#^[A-Z]\w*[a-z]\w*(\\\\[A-Z]\w*[a-z]\w*)+\z#', ie. + instead of *.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, it can change false-classes, but in practice it rarely happens. Regexp will not detect non-namespace classes, which are usually native PHP classes, so perhaps ideal solution is class_exists || interface_exists || regexp.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for fixed regex, that makes sense.

Work much better now 🎂

Last thing I have to resolve is importing use statements.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Jul 14, 2017

It looks like importing of namespaces is done by SlevomatCodingStandard\Sniffs\Namespaces\ReferenceUsedNamesOnlySniff.

Which is run after this. Too bad

/**
* @var string
*/
private const CLASS_OR_INTERFACE_PATTERN = '#^[A-Z]\w*[a-z]\w*(\\\\[A-Z]\w*[a-z]\w*)+\z#';
Copy link

Choose a reason for hiding this comment

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

a lot of falsy positives here

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggestion?

Copy link

Choose a reason for hiding this comment

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

drop anything that may creates wrong changes, it's better to convert too few than too much

Copy link
Member Author

Choose a reason for hiding this comment

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

For example?

@keradus
Copy link

keradus commented Jul 14, 2017

@keradus Could I ask you guys for short review?

  1. this rule is violating rule of context-unaware. for that reason, the result of fixer will be different on different envs.
  2. working fixer executed on dozens of repos: keradus/PHP-CS-Fixer@33be691

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Jul 14, 2017

@keradus Thanks for fast reply!

  1. "violating rule of context-unaware" What does that mean? How to improve that?
  2. Why it is not part of php-cs-fixer? That would save me a lot of work. I've used a bit from your version, so thanks for sharing it.

keradus referenced this pull request in keradus/PHP-CS-Fixer Jul 15, 2017
@keradus
Copy link

keradus commented Jul 15, 2017

@TomasVotruba
Copy link
Member Author

I merged to test this in practise.

Still open to feedback.

@TomasVotruba TomasVotruba merged commit ddf3505 into master Jul 15, 2017
@TomasVotruba TomasVotruba deleted the coding-standard-php55-class branch July 15, 2017 14:34
@TomasVotruba TomasVotruba mentioned this pull request May 27, 2018
5 tasks
@deprecated-packages deprecated-packages locked as resolved and limited conversation to collaborators Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[CodingStandard] add ::class fixer
3 participants