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.

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

NewObjectToFactoryCreateRector #982

Merged
merged 1 commit into from Jan 24, 2019

Conversation

2 participants
@JanMikes
Copy link
Collaborator

JanMikes commented Jan 22, 2019

Closes #981

Before

<?php

public function xyz()
{
    $a = new MyClass();
}

After

/** @var MyClassFactory */
private $myClassFactory;

public function xyz()
{
    $a = $this->myClassFactory->createMyClass();
}
@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 22, 2019

@TomasVotruba please could you check my code?

I am getting weird error with this config:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException : The configuration key "Rector\Tests\Rector\Architecture\Factory\NewObjectToFactoryCreateRector\Source\MyClass" is unsupported for definition "Rector\Rector\Architecture\Factory\NewObjectToFactoryCreateRector" in "/Users/janmikes/Sites/rector/tests/Rector/Architecture/Factory/NewObjectToFactoryCreateRector/config.yml". Allowed configuration keys are "alias", "parent", "class", "shared", "synthetic", "lazy", "public", "abstract", "deprecated", "factory", "file", "arguments", "properties", "configurator", "calls", "tags", "decorates", "decoration_inner_name", "decoration_priority", "autowire", "autoconfigure", "bind".
 /Users/janmikes/Sites/rector/vendor/symfony/dependency-injection/Loader/YamlFileLoader.php:813
 /Users/janmikes/Sites/rector/vendor/symfony/dependency-injection/Loader/YamlFileLoader.php:341
 /Users/janmikes/Sites/rector/vendor/symfony/dependency-injection/Loader/YamlFileLoader.php:229
 /Users/janmikes/Sites/rector/vendor/symplify/package-builder/src/Reflection/PrivatesCaller.php:23
 /Users/janmikes/Sites/rector/vendor/symplify/package-builder/src/Yaml/FileLoader/AbstractParameterMergingYamlFileLoader.php:106
 /Users/janmikes/Sites/rector/vendor/symfony/config/Loader/DelegatingLoader.php:40
 /Users/janmikes/Sites/rector/src/DependencyInjection/RectorKernel.php:50
 /Users/janmikes/Sites/rector/vendor/symfony/http-kernel/Kernel.php:653
 /Users/janmikes/Sites/rector/vendor/symfony/http-kernel/Kernel.php:543
 /Users/janmikes/Sites/rector/vendor/symfony/http-kernel/Kernel.php:133
 /Users/janmikes/Sites/rector/src/DependencyInjection/ContainerFactory.php:26
 /Users/janmikes/Sites/rector/src/Testing/PHPUnit/AbstractRectorTestCase.php:142
 /Users/janmikes/Sites/rector/src/Testing/PHPUnit/AbstractRectorTestCase.php:64
@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 22, 2019

Nevermind, i fixed that. Not using provideConfig() but getRectorConfiguration() did the trick 👍

@JanMikes JanMikes changed the title [WIP] Failing tests [WIP] NewObjectToFactoryCreateRector Jan 22, 2019

@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 22, 2019

@TomasVotruba call for help again, i digged into it a bit and found out that it does not matter whether i use provideConfig() or getRectorConfiguration() -> it does the same thing with only different syntax.

The problem is when using array in config:

services:
  Rector\Rector\Architecture\Factory\NewObjectToFactoryCreateRector:
    Rector\Tests\Rector\Architecture\Factory\NewObjectToFactoryCreateRector\Source\MyClass: []

It stops working. Could you please have a quick look at that?

@TomasVotruba
Copy link
Member

TomasVotruba left a comment

The issue is, there is no constructor to autowire parameters to.

services:
    Rector\Rector\Architecture\Factory\NewObjectToFactoryCreateRector:
        anything...

is shortcut for

services:
    Rector\Rector\Architecture\Factory\NewObjectToFactoryCreateRector:
        arguments:
            anything...

Your Rector needs constructor to pass configuration into.

Inspire here:

/**
* @param string[] $oldToNewClasses
*/
public function __construct(array $oldToNewClasses)
{
$this->oldToNewClasses = $oldToNewClasses;
}

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 23, 2019

<?php

final class NewObjectToFactoryCreateRector extends AbstractRector
{
    /**
     * @var string[][]
     */
    private $factoryWithMethodByObject;

    /**
     * @param string[][] $factoryWithMethodByObject
     */
    public function __construct(array $factoryWithMethodByObject)
    {
        $this->factoryWithMethodByObject = $factoryWithMethodByObject;
    }
    
    /**
     * @return string[]
     */
    public function getNodeTypes(): array
    {
        return [New_::class];
    }

    /**
     * @param New_ $node
     */
    public function refactor(Node $node): ?Node
    {
        foreach ($this->factoryWithMethodByObject as $type => $factoryWithMethod) {
            if (! $this->isType($node, $type)) {
                continue;
            }

            // refactor to MethodCall
            // add property
        }

        return $node;
    }
}

This will be then configured in config:

# rector.yml
services:
    NewObjectToFactoryCreateRector: # ← rector class
        # configuration array that will be passed as $factoryWithMethodByObject
        FirstObject: ['FirstFactory', 'method']
        SecondObject: ['SecondFactory', 'method']

↓ full syntax without magic. Normally people would have to write all this manually and be careful about typos in "factoryWithMethodByObject", which can be really annoying, because there is no error on typo of argument name.

services:
    NewObjectToFactoryCreateRector: # ← rector class
        arguments:
            $factoryWithMethodByObject:
                FirstObject: ['FirstFactory', 'method']
                SecondObject: ['SecondFactory', 'method']
@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 23, 2019

Just to let you know, I have tried this and this does not work, that is why i asked and felt so weird:

# rector.yml
services:
    NewObjectToFactoryCreateRector: # ← rector class
        # configuration array that will be passed as $factoryWithMethodByObject
        FirstObject: ['FirstFactory', 'method']
        SecondObject: ['SecondFactory', 'method']
@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 23, 2019

Could you push it? Now there is no array in constructor to put it to

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 23, 2019

@JanMikes JanMikes force-pushed the new-object-to-factory-method-rector branch 4 times, most recently from 5a738bd to da068fd Jan 24, 2019

@JanMikes JanMikes changed the title [WIP] NewObjectToFactoryCreateRector NewObjectToFactoryCreateRector Jan 24, 2019

@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 24, 2019

@TomasVotruba working, rebased on master (solved conflicts) and ready to merge 😉

As well i squashed commits so all new feature is just in one commit: da068fd

@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 24, 2019

Btw i did not test/cover edge cases - property already exists, factory already exists in different property in class etc.

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

@JanMikes I see some commits already in master. Could you rebase on it?

image

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

Feature works, nice 👍

Testing edge cases would be nice to have, but not neccesary. It's up to you.
I think we'll find them when using on Entrydo.

Also there is 1 fail on static analysis, please fix it:

https://travis-ci.org/rectorphp/rector/jobs/483997841#L661

@JanMikes JanMikes force-pushed the new-object-to-factory-method-rector branch from da068fd to 86826e1 Jan 24, 2019

@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 24, 2019

Thank you, i fixed that and i will as well cover the edge cases, while i am already in it 😉

I think that history must be mistaken, i have just checked it and there are no commits in master yet -> i rebased this branch on master. Last my commit i can see in master is 31a5e9bcd2c44a1553fd7f17f4d4c563c138af3b

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

You pushed 10 minutes ago, that fixed the commits 👍

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

I will as well cover the edge cases, while i am already in it

Great 👍

@JanMikes JanMikes force-pushed the new-object-to-factory-method-rector branch from 86826e1 to 43de770 Jan 24, 2019

@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 24, 2019

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

I'm on it :)

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

Btw, no need to rebase after each commit, just once in the end.
Now I don't know what's new since the last review and have to read whole code.

@TomasVotruba
Copy link
Member

TomasVotruba left a comment

I've added what I could find. Very good job in effective code 👍

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

Great! Ready to merge for me 👍

@JanMikes JanMikes force-pushed the new-object-to-factory-method-rector branch from 5e46a7a to a78d88a Jan 24, 2019

@JanMikes JanMikes merged commit 753a478 into master Jan 24, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@JanMikes JanMikes deleted the new-object-to-factory-method-rector branch Jan 24, 2019

@TomasVotruba

This comment has been minimized.

Copy link
Member

TomasVotruba commented Jan 24, 2019

Kaboom 🚀

@JanMikes

This comment has been minimized.

Copy link
Collaborator Author

JanMikes commented Jan 26, 2019

Looking forward to get this tagged 😉 for now i will use dev-master dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment