Bump to Rector 0.9.20 #1961
Bump to Rector 0.9.20 #1961
Conversation
| }, | ||
| "require-dev": { | ||
| "captainhook/captainhook": "^5.4", | ||
| "captainhook/plugin-composer": "^5.2", | ||
| "phpunit/phpunit": "^8.0|^9.0", |
There was a problem hiding this comment.
PHPUnit 9 is supporting PHP 7.3+, so PHPUnit 8 can be dropped here
| trait ConnectionCallTrait | ||
| final class ConnectionCallFactory | ||
| { | ||
| /** | ||
| * @var NodeFactory | ||
| */ | ||
| protected $nodeFactory; | ||
| private $nodeFactory; | ||
|
|
||
| /** | ||
| * @required | ||
| */ | ||
| public function autowireNodeFactoryTrait(NodeFactory $nodeFactory): void | ||
| public function __construct(NodeFactory $nodeFactory) | ||
| { | ||
| $this->nodeFactory = $nodeFactory; | ||
| } |
There was a problem hiding this comment.
Traits depend on magical presence of other properties, e.g. here is "hope" for $nodeFactory.
Instead, it's better to use service, that has clear contract and there is no need for hope :)
I know this was used in Rector a lot, so it's only natural to use it in the code that build on Rector. But we're moving away from this in Rector, so code is cleaner and easier to maintain.
There was a problem hiding this comment.
@TomasVotruba Yes, i am not a big fan of traits either. So cool, Rector is moving away from it.
| if (! $this->isMethodStaticCallOrClassMethodObjectType($node, SysNoteRepository::class)) { | ||
| if (! $this->nodeTypeResolver->isMethodStaticCallOrClassMethodObjectType($node, SysNoteRepository::class)) { | ||
| return null; | ||
| } | ||
| if (! $this->isName($node->name, 'findByPidsAndAuthor')) { |
There was a problem hiding this comment.
This is a typical example of where AbstractRector is heading. There will be less and less global methods accessible everywhere, and more focus on specific services that handles the call.
There method will be probably kept even in the future, as they're very practical for every rule:
getName()isName()getStaticType()- etc
| $parentNode = $node->getAttribute('parent'); | ||
| $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE); |
There was a problem hiding this comment.
Strings attribute can typoed, eg. parnet (been there :)) and are hard to detect.
With constants, there is a future proof that even if the string names changes, the constant will still work.
There was a problem hiding this comment.
@TomasVotruba Most of the time i did it right. But sometimes the evenings were so long. You know ;-)
There was a problem hiding this comment.
I know, same here :)
That's why I've delegated this task to PHPStan:
services:
# require constant in argument position
-
class: Symplify\PHPStanRules\Rules\RequireMethodCallArgumentConstantRule
tags: [phpstan.rules.rule]
arguments:
constantArgByMethodByType:
PhpParser\Node:
getAttribute: [0]
hasAttribute: [0]
setAttribute: [0]He will tell me if something is wrong 👍
| if (TYPO3_MODE === 'BE') { | ||
| // must be defined | ||
| $_EXTKEY = '___'; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Test files are autoloaded, so any missing variable will be reported as fatal error (PHP 8 here :))
This little line will make sure the code is valid.
Another practise we use in Rector core is to wrap code to a class and method run by default.
Code works, is autoloaded but is not run.
|
@TomasVotruba If you like i can takeover and try to fix the things by myself and ask questions if you like. Saves you some valuable time. What do you think? |
|
Thank you! That would be great :) I've just finished what I guessed. There is one test case excluded, because of bug in Rector. It's in next 0.9.21 release, but I'd like to finish this first, as there is more changes for `AbstractCommunityTestCase. |
|
It's yours 👍 |
|
Let me know if you need to explain anything in more detail. Break for lunch now 🥘 |
|
Looks ready to merge 👍 |
|
@TomasVotruba Thanks a lot men. Looks good to me. I think it is ok to have somehow more boilerplate but keep the internals more clean. Thanks for keeping us always up to date. |
There has been many changes in
AbstractRector, like removal of ~15 traits. This was a bad design magic crap choice I made years ago and now was time to face it :)Another big change is that
AbstractRectorTestCasecustom methods likesetParameter()andgetPhpVersion()are moved to config-based approach. This will remove extra magical layer in test and make it easier.I have it still in my head, so I'll upgrade Rector here to put headaches on you.
Good job with tests, it give me very fast feedback in what is wrong 👍
Pre-step to:
rectorphp/rector#5478 (comment)