Skip to content
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

[DX] Add convention-based config loading to allow services from extension to be loaded #6129

Closed
TomasVotruba opened this issue Apr 13, 2021 · 16 comments
Assignees
Labels

Comments

@TomasVotruba
Copy link
Member

Problem: If you create and Rector extension, to run Rector with it the rector.php must include it explicitly. Without it, the extension does not load its services.

The workaround is to add config manually e.g. in bin/rector.php: https://github.com/sabbelasichon/typo3-rector/blob/b7a6b8a67adf0c2e84974eb1be6f8469de1b920e/bin/rector.php#L48


Solution would be to include config in composer.json configuration, like PHPStan composer pluging does: https://github.com/phpstan/extension-installer

The generated class is used during DI creation: https://github.com/phpstan/phpstan-src/blob/c471c7b050e0929daf432288770de673b394a983/src/Command/CommandHelper.php#L193-L217


It would also help with magical loading we currently have:

rector/config/config.php

Lines 15 to 29 in 021ffc7

// rector root
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-symfony/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-nette/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-laravel/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-phpunit/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-cakephp/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-doctrine/config/config.php', null, 'not_found');
// rector sub-package
$containerConfigurator->import(__DIR__ . '/../../rector-symfony/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-nette/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-laravel/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-phpunit/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-cakephp/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-doctrine/config/config.php', null, 'not_found');


Based on call with cc @sabbelasichon

@sabbelasichon
Copy link
Contributor

Thanks for taking care. I would like to create something here. Maybe not so fast as you do, but it is nothing with time pressure, right?

@TomasVotruba
Copy link
Member Author

You can go for it 👍

@sabbelasichon
Copy link
Contributor

Basically i am done. Would you create a dedicated rector/rector-installer package or is it just part of rector itself?

@TomasVotruba
Copy link
Member Author

is it just part of rector itself

👍 I would do it as MVP and see how it works.

@sabbelasichon
Copy link
Contributor

Sorry, but i think it should be a standalone package because it must be of type composer-plugin to register it correctly. I don´t like to give the whole rector package the type of composer-plugin.

@sabbelasichon
Copy link
Contributor

If you like you could create a rector/rector-installer repository and i am gonna fork it and provide my solution.

@TomasVotruba
Copy link
Member Author

Yes, plugin is standalone and imlementation in Rector is part of core.

There you go: https://github.com/rectorphp/rector-installer, you should have write access so no fork is needed 👍

@sabbelasichon
Copy link
Contributor

The installer package is ready, https://github.com/rectorphp/rector-installer/
We have to register the package at packagist.org still.

@sabbelasichon
Copy link
Contributor

If you like as a follow up i could also add the configurations to rector/rector-*. Just tell me. I would be happy to help.

@TomasVotruba
Copy link
Member Author

There you go - https://packagist.org/packages/rector/rector-installer

If you like as a follow up i could also add the configurations to rector/rector-*. Just tell me. I would be happy to help.

Yea, go for it 👍

@sabbelasichon
Copy link
Contributor

I have tested it to remove the magic config integration. It works very well. But would you make the rector-installer be a requirement for rector itself? If so, everything is fine, if not as in phpstan, we have to think about an alternative to register the extension configs. What do you think? I would suggest to make the rector-installer a first class citizen of rector itself.

@TomasVotruba
Copy link
Member Author

Could you sum up why it's a requirement?

@sabbelasichon
Copy link
Contributor

Sure. If i am removing the config lines here

rector/config/config.php

Lines 15 to 29 in 021ffc7

// rector root
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-symfony/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-nette/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-laravel/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-phpunit/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-cakephp/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../vendor/rector/rector-doctrine/config/config.php', null, 'not_found');
// rector sub-package
$containerConfigurator->import(__DIR__ . '/../../rector-symfony/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-nette/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-laravel/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-phpunit/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-cakephp/config/config.php', null, 'not_found');
$containerConfigurator->import(__DIR__ . '/../../rector-doctrine/config/config.php', null, 'not_found');
the configs are not loaded anymore without the rector-installer. Or do is miss something here?

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Apr 19, 2021

That should work, yea. I'm not sure how the added feature works, so I'm only curious :)

Could you explain the technical steps by step for me and why does the rector-installer be a requirement for rector itself?

@sabbelasichon
Copy link
Contributor

Thanks for merging. This issue can be closed then, right?

@TomasVotruba
Copy link
Member Author

Yes! 👍

Just a tip to automate this. If you put "closes x" in PR description, it will close the issue automatically on PR merge. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

TomasVotruba added a commit that referenced this issue Jul 7, 2024
rectorphp/rector-src@2f86b39 [Php82] Handle has only readonly properties but not all promoted property on ReadOnlyClassRector (#6129)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants