[Php80] Handle multiple ->ruleWithConfiguration() call in same rule on AnnotationToAttributeRector#4831
[Php80] Handle multiple ->ruleWithConfiguration() call in same rule on AnnotationToAttributeRector#4831samsonasik wants to merge 3 commits intomainfrom
Conversation
|
I am debugging ➜ rector-src git:(failing-fixture) vendor/bin/phpunit rules-tests/Php80/Rector/Class_/AnnotationToAttributeRector/MultipleCallAnnotationToAttributeRectorTest.php
PHPUnit 10.3.2 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.5
Configuration: /Users/samsonasik/www/rector-src/phpunit.xml
array (1)
0 => Rector\Php80\ValueObject\AnnotationToAttribute #1984
| tag: 'Doctrine\ORM\Mapping\Entity'
| attributeClass: null
array (1)
0 => Rector\Php80\ValueObject\AnnotationToAttribute #1981
| tag: 'Doctrine\ORM\Mapping\ManyToMany'
| attributeClass: null
F 1 / 1 (100%)
Time: 00:00.250, Memory: 58.00 MB
There was 1 failure: |
|
@TomasVotruba the possible solution is to use public function configure(array $configuration): void
{
Assert::allIsAOf($configuration, AnnotationToAttribute::class);
- $this->annotationsToAttributes = $configuration;
+ $this->annotationsToAttributes = array_merge($this->annotationsToAttributes, $configuration);any idea to make it general on other rules as well? |
|
@lucasmirloup I fixed with add new @TomasVotruba the existing rules-tests may need update based on this change. |
|
@TomasVotruba with this change, individual test is somehow working ok on rector-symfony, but not work on run all: ➜ rector-symfony git:(main) ✗ vendor/bin/phpunit tests/Set/Symfony44/Symfony44Test.php
PHPUnit 10.3.2 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.5
Configuration: /Users/samsonasik/www/rector-symfony/phpunit.xml
. 1 / 1 (100%)
Time: 00:00.567, Memory: 60.00 MB
OK (1 test, 2 assertions)
➜ rector-symfony git:(main) ✗ vendor/bin/phpunit
PHPUnit 10.3.2 by Sebastian Bergmann and contributors.
Runtime: PHP 8.2.5
Configuration: /Users/samsonasik/www/rector-symfony/phpunit.xml
..............F................................................ 63 / 403 ( 15%)
............................................................... 126 / 403 ( 31%)
............................................................... 189 / 403 ( 46%)
............................................................... 252 / 403 ( 62%)
............................................................... 315 / 403 ( 78%)
............................................................... 378 / 403 ( 93%)
......................... 403 / 403 (100%)
Time: 00:10.361, Memory: 224.50 MB
There was 1 failure:
1) Rector\Symfony\Tests\Set\Symfony44\Symfony44Test::test with data set #0
Failed on fixture file "event_fixture.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
class SomeEventListener
{
- public function someMethod(\Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent $exceptionEvent)
+ public function someMethod(\Symfony\Component\HttpKernel\Event\ExceptionEvent $exceptionEvent)so it somehow reset on some area. |
|
@TomasVotruba Ok, the change is expected on all run on rector-symfony, as merged. |
|
Oh, it's not, So the |
|
@TomasVotruba let's tweak with reset value in |
|
Finally, it works @TomasVotruba @lucasmirloup 🎉 🎉 🎉 All checks have passed 🎉 🎉 🎉 @TomasVotruba I think it is ready. |
|
This should be handled internally by the I'll look into it |
…n same rule on AnnotationToAttributeRector
8ba0d70 to
2bf591b
Compare
|
Rebased, @TomasVotruba how about merge as is for now and improve in separate PR? This already tweak on |
|
No need to rush this one, as it better be fixed correctly than re-patched. The rules should be registered just once, maybe that will help: $isBound = $this->bound($rectorClass);
// avoid double registration
if ($isBound === false) {
$this->singleton($rectorClass);
$this->tagRectorService($rectorClass);
}You can try these checks in I'm looking into this, but experience some infinite loops while running this test. How do tests work for you? :) |
Seems this fixed it, there was infinite loop without the illuminate/container patch: |
|
|
|
@TomasVotruba to reproduce, you can run the following test: |
|
@TomasVotruba it has nothing todo with service registered once or many times, the configurable rules need to call |
|
The |
| $configurableRector->configure($configuration); | ||
|
|
||
| SimpleParameterProvider::addParameter(Option::RECTOR_CONFIGURATION, [ | ||
| $rectorClass => $configuration, |
There was a problem hiding this comment.
The $rectorClass as configurable index is essential here, to be re-used to merge with existing service config on next ->ruleWithConfiguration() called.
@TomasVotruba @lucasmirloup this is failing fixture for multiple calls
->ruleWithConfiguration()on same rector rule which should merge instead of replace:Fixes rectorphp/rector#8150
Ref https://getrector.com/demo/d196b918-b061-4c2d-a439-7058e88e6e57