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

Make use of RectorConfig and ECSConfig #2840

Merged
merged 8 commits into from
Apr 20, 2022
Merged

Make use of RectorConfig and ECSConfig #2840

merged 8 commits into from
Apr 20, 2022

Conversation

TomasVotruba
Copy link
Contributor

@TomasVotruba TomasVotruba commented Apr 20, 2022

No description provided.

@TomasVotruba TomasVotruba changed the title tv upgrade Make use of RectorConfig and ECSConfig Apr 20, 2022
@TomasVotruba TomasVotruba force-pushed the tv-upgrade branch 2 times, most recently from 19b649d to a6a9d39 Compare April 20, 2022 11:41
@TomasVotruba TomasVotruba force-pushed the tv-upgrade branch 3 times, most recently from febe8cc to 3830cda Compare April 20, 2022 12:22
@TomasVotruba
Copy link
Contributor Author

Phew :) the huge configs makes my PhpStorm lag, but Rector helped itself to upgrade :)

I hope I got everything right. This is good to go 👍

Could you check?

@sabbelasichon sabbelasichon merged commit 7b11cdd into main Apr 20, 2022
@sabbelasichon
Copy link
Owner

Thank you very much. That looks pretty good.

@sabbelasichon sabbelasichon deleted the tv-upgrade branch April 20, 2022 15:02
@TomasVotruba
Copy link
Contributor Author

💪

@simonschaufi
Copy link
Collaborator

simonschaufi commented May 27, 2022

@TomasVotruba How is the file ssch/typo3-rector/config/composer/typo3-104-composer-packages-extensions.php generated? It has an error:

[ERROR] Call to undefined method "Rector\Config\RectorConfig::ruleWithConfigure()" in                                  
         /xxx/vendor/rector/rector/vendor/ssch/typo3-rector/src/Set/../../config/composer/typo3-10
         4-composer-packages-extensions.php (which is being imported from "/xxx/rector.php").  

It should actually be ruleWithConfiguration

@TomasVotruba
Copy link
Contributor Author

@simonschaufi Thanks for reporting 👍

I don't know, but when you look for "ruleWithConfigure" string, it should be clear.
Could you send PR to fix it?

@simonschaufi
Copy link
Collaborator

@TomasVotruba I found it only within that file but that file looks like it's generated as it contains plenty of versions of TYPO3 extensions (which is not up-to-date anyway) and should be generated again.

@TomasVotruba
Copy link
Contributor Author

I'd simply replace it, as it didn't change last month.

@simonschaufi
Copy link
Collaborator

simonschaufi commented May 27, 2022

@TomasVotruba Sorry that this turns into a support chat but I found the command that generates the file but when calling it, I get the error from rector-src: " Node "PhpParser\Node\Expr\Closure" with parent of "PhpParser\Node\Stmt\Return_" is missing scope required for scope refresh."

The command is: vendor/bin/rector add-composer-typo3-extensions-to-config

I'm totally lost what "missing scope" means here. I hope you as the founder of rector knows what that means? This is the file where the error comes from: https://github.com/sabbelasichon/typo3-rector/blob/main/config/composer/move_extension_from_ter_to_packagist.php

@TomasVotruba
Copy link
Contributor Author

This will be tricky... is the new Closure node somehow traversed?

@simonschaufi
Copy link
Collaborator

I have no idea about that

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented May 28, 2022

It looks like miss-use of AddPackageVersionRector for simple node traverser

@TomasVotruba
Copy link
Contributor Author

I'm working on refactoring here #2998

It's almost done, just super slow. I'll seen during weekend, what can be improved :) thanks for reporting 👍

@TomasVotruba
Copy link
Contributor Author

@simonschaufi So after 5 hours I've come to few findings :) the command is super slow because:

  • it fetches all typo3 packages from Packagist, one by one... in total 2777 API calls
  • it parses and loads files in /config/composer, that have thousands of lines of nested method calls in PHP, so it takes Rector long time to load and process
  • it generates probably hundreds such configuration items, based on dependencies of 2777 packagist

I see this code first time, as I only improve changes from Rector core here, so bear with me.

The idea to improve this I had was:

  • do not work with PHP, but only dump json package renames
  • load those package renames via foreach() in sets

I'm not sure this is helpful, as the command takes ~30 mins to run (maybe more) and any break is hell to debug.

Another idea is:

  • drop this component of this package, because it seems too heavy to maintain and work with. I guess 90 % of changes takes the most churn

Saying that, I don't want to invest more energy anymore, as to make it useful would take probably 3-5 days, as everything is new to me here.

But you can continue with the exploration. Here is some work you can continue with https://github.com/sabbelasichon/typo3-rector/pull/2998/files

@sabbelasichon
Copy link
Owner

sabbelasichon commented May 28, 2022

Thanks Tomas. It worked quite well. Don't know why it is not anymore. Lost track of it. I think we should drop this as it turned out not to be used that much. I will have a deeper look next week.

@simonschaufi
Copy link
Collaborator

simonschaufi commented May 28, 2022

@TomasVotruba thank you so much for your investigation!
@sabbelasichon I noticed that there has been a GitHub action to update the composer package list but this is also gone, at least I found some commits like: "[automated] Update composer packages (#2427)" which I couldn't find in any workflow file

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.

3 participants