Skip to content

Rector as Services to remove dump proxy "rectors" to "services" mechanism #335

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

Merged
merged 14 commits into from
Mar 4, 2018

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Mar 3, 2018

Why?

  • using rectors key in config instead of services was just to save 1 extra line of config:

    Before

     # rector.yml
     rectors:
         SomeRector:
             key: value

    After

     # rector.yml
     services:
         SomeRector:
             $config:
                 key: value
    
         # which is in full syntax
         SomeRector:
             arguments:
                 $config:
                     key: value

Which of these 2 version is clear to first time user, that meets Rector the first time?


To make this work 5 extra classes were needed, that duplicated Symfony DI:

I don't like reinventing the wheel, moreover in Symfony\DI case

I also recall, how frustrated I was when writing ECS and exploring PHP_CodeSniffer and PHP CS Fixer when I found out - all sniffs/fixers are just statically added services; it would save me dozens of hours if services like here would be used

Also writing a post about Rector and Collector helped me to realize this WTF

image

For all these reasons we should get rid of this extra layer that only came from historical background now and adds more comlexity then help.

Benefits

  • 5 classes replicating Symfony\DI behavior was removed
  • everyone can see from the config that Rector is service, right away from the config
  • better understanding how Rector get dependencies and how they can be configured
  • also allows to use any Symfony\DI features right away from the config; lot of hacking would be required to do it the old way
  • removing extra config validation like this: https://github.com/rectorphp/rector/pull/335/files#diff-0e65def841bad669b99fc89ef2257117L173 and let Symfony\DI handle it instead

Todo

@TomasVotruba TomasVotruba changed the title [POC] Rector as Services to remove dump "rectors" to service mechanism Rector as Services to remove dumb "rectors" to "services" mechanism Mar 3, 2018
@carusogabriel
Copy link
Contributor

@TomasVotruba Gonna review this afternoon. Sounds like an amazing improvement.

TomasVotruba added a commit to TomasVotruba/tomasvotruba.com that referenced this pull request Mar 3, 2018
TomasVotruba added a commit to TomasVotruba/tomasvotruba.com that referenced this pull request Mar 3, 2018
@TomasVotruba TomasVotruba changed the title Rector as Services to remove dumb "rectors" to "services" mechanism Rector as Services to remove dump proxy "rectors" to "services" mechanism Mar 3, 2018
Copy link
Contributor

@carusogabriel carusogabriel left a comment

Choose a reason for hiding this comment

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

I believe this will improve better understand of Rector as well 👏

@TomasVotruba TomasVotruba merged commit 0cc9d52 into master Mar 4, 2018
@TomasVotruba
Copy link
Member Author

Thank you, it definitely will 👍

@TomasVotruba TomasVotruba deleted the rector-as-services branch March 4, 2018 13:42
# old namespace prefix
- 'PHPUnit_'
# exclude classes
- '!PHPUnit_Framework_MockObject_MockObject'
Copy link
Member Author

Choose a reason for hiding this comment

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

Now it could be another param with explicit name, like $skipClasses
To show this feature to the user without commenting everything

@TomasVotruba
Copy link
Member Author

Makes use of this awesome feature: symfony/symfony#21313

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Mar 11, 2018

How to: custom service formats in services.yml

Just a note, there is quite simple and clear way to write custom service formats in services.yml

All is here: https://github.com/Symplify/Symplify/blob/adbf304c01a41c5d42593840436fa2450d364d2a/packages/EasyCodingStandard/src/Yaml/CheckerTolerantYamlFileLoader.php#L27-L81

<?php declare(strict_types=1);

namespace Symplify\EasyCodingStandard\Yaml;

use Nette\Utils\Strings;
use Symfony\Component\Config\FileLocatorInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Loader\YamlFileLoader;

/**
 * The need: https://github.com/symfony/symfony/pull/21313#issuecomment-372037445
 */
final class CheckerTolerantYamlFileLoader extends YamlFileLoader
{
    /**
     * @var CheckerConfigurationGuardian
     */
    private $checkerConfigurationGuardian;

    public function __construct(ContainerBuilder $containerBuilder, FileLocatorInterface $fileLocator)
    {
        $this->checkerConfigurationGuardian = new CheckerConfigurationGuardian();

        parent::__construct($containerBuilder, $fileLocator);
    }

    /**
     * @param string $file
     * @return array|mixed|mixed[]
     */
    protected function loadFile($file)
    {
        $decodedYaml = parent::loadFile($file);

        if (isset($decodedYaml['services'])) {
            return $this->moveArgumentsToPropertiesOrMethodCalls($decodedYaml);
        }

        return $decodedYaml;
    }

    /**
     * @param mixed[] $yaml
     * @return mixed[]
     */
    private function moveArgumentsToPropertiesOrMethodCalls(array $yaml): array
    {
        foreach ($yaml['services'] as $checker => $serviceDefinition) {
            if (empty($serviceDefinition)) {
                continue;
            }

            // is checker service?
            if (! Strings::endsWith($checker, 'Fixer') && ! Strings::endsWith($checker, 'Sniff')) {
                continue;
            }

            if (Strings::endsWith($checker, 'Fixer')) {
                $this->checkerConfigurationGuardian->ensureFixerIsConfigurable($checker, $serviceDefinition);
                // move parameters to "configure()" call
                $yaml['services'][$checker]['calls'] = [
                    ['configure', [$serviceDefinition]],
                ];
            }

            if (Strings::endsWith($checker, 'Sniff')) {
                // move parameters to property setters
                foreach ($serviceDefinition as $key => $value) {
                    $this->checkerConfigurationGuardian->ensurePropertyExists($checker, $key);
                    $yaml['services'][$checker]['properties'][$key] = $this->escapeValue($value);
                }
            }

            // cleanup parameters
            foreach ($serviceDefinition as $key => $value) {
                unset($yaml['services'][$checker][$key]);
            }
        }

        return $yaml;
    }

    /**
     * @param mixed $value
     * @return mixed
     */
    private function escapeValue($value)
    {
        if (is_numeric($value)) {
            return $value;
        }

        return Strings::replace($value, '#@#', '@@');
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants