Skip to content

Use require string argument in constructor rule #5623

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Use require string argument in constructor rule #5623

wants to merge 3 commits into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Feb 19, 2021

Waiting on new tagged release for Symplify PHPStan Rules now that deprecated-packages/symplify#2968 is merged.

This PR is a reminder for that.

It should prevent #5596 from happening again.

@TomasVotruba
Copy link
Member

It's better to test Symplify rule first, before tagging. We discovered many bugs before letting them out :)

Using dev-master for Symplify packages, temporarily

@ruudk
Copy link
Contributor Author

ruudk commented Feb 19, 2021

@TomasVotruba Done, do you agree that these should be fixed?


 ------------------------------------------------------------------------
  rules/nette-to-symfony/src/NodeFactory/ActionWithFormProcessClassMethodFactory.php:40
 ------------------------------------------------------------------------
  - '#Use quoted string in constructor "new PhpParser\\Node\\Name\\FullyQualified\(\)" argument on position 0 instead of "\:\:class\. It prevent scoping of the class in building prefixed package#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  rules/nette-to-symfony/src/Rector/Class_/NetteControlToSymfonyControllerRector.php:151
 ------------------------------------------------------------------------
  - '#Use quoted string in constructor "new PhpParser\\Node\\Name\\FullyQualified\(\)" argument on position 0 instead of "\:\:class\. It prevent scoping of the class in building prefixed package#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  src/DependencyInjection/Loader/ConfigurableCallValuesCollectingPhpFileLoader.php:34
 ------------------------------------------------------------------------
  - '#Method parameters must be compatible with its parent#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  src/DependencyInjection/Loader/ConfigurableCallValuesCollectingPhpFileLoader.php:44
 ------------------------------------------------------------------------
  - '#Method parameters must be compatible with its parent#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  utils/doctrine-annotation-parser-syncer/src/Rector/Assign/AssignNewDocParserRector.php:41
 ------------------------------------------------------------------------
  - '#Use quoted string in constructor "new PhpParser\\Node\\Name\\FullyQualified\(\)" argument on position 0 instead of "\:\:class\. It prevent scoping of the class in building prefixed package#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  utils/doctrine-annotation-parser-syncer/src/Rector/ClassMethod/ChangeOriginalTypeToCustomRector.php:42
 ------------------------------------------------------------------------
  - '#Use quoted string in constructor "new PhpParser\\Node\\Name\\FullyQualified\(\)" argument on position 0 instead of "\:\:class\. It prevent scoping of the class in building prefixed package#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  utils/doctrine-annotation-parser-syncer/src/Rector/ClassMethod/LogIdentifierAndResolverValueInConstantClassMethodRector.php:134
 ------------------------------------------------------------------------
  - '#Use quoted string in constructor "new PhpParser\\Node\\Name\\FullyQualified\(\)" argument on position 0 instead of "\:\:class\. It prevent scoping of the class in building prefixed package#'
 ------------------------------------------------------------------------

@TomasVotruba
Copy link
Member

@TomasVotruba Done, do you agree that these should be fixed?

That was point of the rule, right? 👍

@ruudk
Copy link
Contributor Author

ruudk commented Feb 19, 2021

@TomasVotruba Yes, but I mean, are there no false positives? I know I'll have to manually change them and that's fine.

@TomasVotruba
Copy link
Member

TomasVotruba commented Feb 19, 2021 •
edited
Loading

Not sure. I trust your jugment. Which cases would you exclude and why?

@ruudk ruudk force-pushed the use-RequireStringArgumentInConstructorRule branch from 7ce3fda to 44535c4 Compare February 20, 2021 10:21
@ruudk
Copy link
Contributor Author

ruudk commented Feb 20, 2021

Updated the PR!

@TomasVotruba
Copy link
Member

Good 👍 I'm gonna release new patch Simplify today

CI is failing on PHP 7.3

@ruudk
Copy link
Contributor Author

ruudk commented Feb 20, 2021

How to fix 7.3? As it doesn't have the type hints. Ignore the rule for that file?

@TomasVotruba
Copy link
Member

Not sure. How did it happen?

It's PHPUnit, not PHPStan

image

@ruudk ruudk force-pushed the use-RequireStringArgumentInConstructorRule branch from 44535c4 to b0ef475 Compare February 20, 2021 11:35
@ruudk ruudk force-pushed the use-RequireStringArgumentInConstructorRule branch from b0ef475 to a415e1f Compare February 22, 2021 13:12
@ruudk
Copy link
Contributor Author

ruudk commented Feb 22, 2021

@TomasVotruba 2 more errors left:

 ------------------------------------------------------------------------
  packages/better-php-doc-parser/src/PhpDocParser/BetterPhpDocParser.php:240
 ------------------------------------------------------------------------
  - '#Regex must use string named capture groups instead of numeric#'
 ------------------------------------------------------------------------

 ------------------------------------------------------------------------
  rules/php70/src/EregToPcreTransformer.php:277
 ------------------------------------------------------------------------
  - '#Regex must use string named capture groups instead of numeric#'
 ------------------------------------------------------------------------

ignore them?

Or please merge this PR so that at least some work is fixed already... master is now broken.

@ruudk ruudk mentioned this pull request Feb 22, 2021
Closed
@TomasVotruba
Copy link
Member

ignore them?

Better fix them, if it's not a problem. Green CI is easy for me to merge ✔️
Otherwise I have to check what is causing the problem and it creates extra unnecesary delays.

@ruudk
Copy link
Contributor Author

ruudk commented Feb 22, 2021

I understand, but the 2 errors that are reported I don't know how to fix. It's happening because of changes in symplify/phpstan-rules. Unrelated to my original PR.

Maybe @samsonasik can have a look at it?

@TomasVotruba
Copy link
Member

TomasVotruba commented Feb 22, 2021 •
edited
Loading

Understood, this will happen in the future as we test symplify/phpstan-rules here. Most of them were based on needs for Rector code, so we might also use Symplify dev version just to confirm they work correctly.

I usually fix it that I look for "Regex must use string named capture groups instead of numeric" string with PHPStorm. It shows RequireStringRegexMatchKeyRule. Then there is link to RequireStringRegexMatchKeyRuleTest tests. There are tests fixture that shows fixed/broken code, to demonstrate how it should be fixed.

@TomasVotruba
Copy link
Member

TomasVotruba commented Feb 22, 2021 •
edited
Loading

I've upgraded to Symplify and fixes static reports in #5655

After it's automerged, I'll process this PR too, so we got it finished 🤗

@TomasVotruba
Copy link
Member

Cherry picked in #5656

@TomasVotruba TomasVotruba mentioned this pull request Feb 22, 2021
Merged
@TomasVotruba TomasVotruba mentioned this pull request Feb 22, 2021
Closed
TomasVotruba added a commit that referenced this pull request Feb 16, 2024
@TomasVotruba
Updated Rector to commit 7cd56d864dc52f88416c47845239ad897467a003
f1e8075 
rectorphp/rector-src@7cd56d8 RectorConfigBuilder: skip and rules can be called multiple times (#5623)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruudk @TomasVotruba