-
-
Notifications
You must be signed in to change notification settings - Fork 687
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
Comments
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? |
You can go for it 👍 |
Basically i am done. Would you create a dedicated rector/rector-installer package or is it just part of rector itself? |
👍 I would do it as MVP and see how it works. |
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. |
If you like you could create a rector/rector-installer repository and i am gonna fork it and provide my solution. |
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 👍 |
The installer package is ready, https://github.com/rectorphp/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. |
There you go - https://packagist.org/packages/rector/rector-installer
Yea, go for it 👍 |
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. |
Could you sum up why it's a requirement? |
Sure. If i am removing the config lines here Lines 15 to 29 in 021ffc7
|
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 |
Thanks for merging. This issue can be closed then, right? |
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 |
rectorphp/rector-src@2f86b39 [Php82] Handle has only readonly properties but not all promoted property on ReadOnlyClassRector (#6129)
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#L48Solution would be to include config in
composer.json
configuration, like PHPStan composer pluging does: https://github.com/phpstan/extension-installerThe 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
Based on call with cc @sabbelasichon
The text was updated successfully, but these errors were encountered: