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

Ignore autoload and includes #4330

Closed
afilina opened this issue Sep 29, 2020 · 15 comments
Closed

Ignore autoload and includes #4330

afilina opened this issue Sep 29, 2020 · 15 comments
Labels

Comments

@afilina
Copy link
Contributor

afilina commented Sep 29, 2020

Feature Request

I'm wondering how feasible it is to make Rector not care about autoloading or includes.

I can't use it on the majority of my legacy projects. They were built pre-composer, routinely contain 1M+ LOC, have hundreds of libs loaded ten different ways that aren't really known upfront, missing entire folders from the repo, etc. My most recent project had 4M LOC. This makes autoloading so complex that it would not be practical to try and reverse-engineer it for Rector's purposes. We're talking days of effort per project, which usually eats up more time than the tool would save.

If it wasn't for the autoloading and include_path issues, I would have used Rector a lot more often. Right now, I use it maybe once every few months, despite needing it every single day. It's painful to brute-force changes that I know for a fact can be automated with Rector. Any way to make this easier?

@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 29, 2020

Hi Anna, thanks for reaching out.

What exactly technically does "not care about autoloading or includes"? Could you share some minimal PHP snippet example what should be skipped/ignored?

@afilina
Copy link
Contributor Author

afilina commented Sep 29, 2020

As I said, those are really complex projects and it's always a different problem. I can try and come up with a few examples from the last few projects.

I can imagine that it could be a config saying "don't try to autoload classes when you encounter them". Most of the changes that I need to do are localized and don't need cross-file inspection.

@TomasVotruba
Copy link
Member

I see. We could use static reflection from PHPStan: https://phpstan.org/blog/zero-config-analysis-with-static-reflection

@afilina
Copy link
Contributor Author

afilina commented Sep 29, 2020

This honestly sounds like a total game-changer. I'll try to write a few test cases.

@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 29, 2020

👍

Here are some basic steps: #3490

@Driky
Copy link

Driky commented Oct 5, 2020

Hello @TomasVotruba, is static reflection in any workable shape with rector ? Following the link you gave we didn't really understand if it was underway or if we need to activate some flag to be able to use it.
If it is underway is there anything we could do to help ?

@TomasVotruba
Copy link
Member

Hi @Driky, there is no integration yet.

We need a contributor who would add such a feature.

@afilina
Copy link
Contributor Author

afilina commented Oct 6, 2020

Here are two scenarios. For both, I added the following to the phpstan.neon file at the root of the project:

parameters:
    featureToggles:
        disableRuntimeReflectionProvider: true

and this class in src/MyClass.php

<?php
namespace App;
class MyClass extends ParentClass
{
    public function MyClass()
    {
    }
}

If I don't have any autoloading configured in rector.yaml or composer.json, I get this error:

 [ERROR] Could not process "src/MyClass.php" file, due to:                                                              
         "Analyze error: "Class MyClass was not found while trying to analyse it - discovering symbols is probably not  
         configured properly.". Include your files in "parameters > autoload_paths".                                    
         See https://github.com/rectorphp/rector#configuration". 

Already, this is weird because how can it not find the class when it's right there in the file mentioned in the same error message.

If I do have autoloading configured, which in most projects requires several days, I get this error:

[ERROR] Could not process "src/MyClass.php" file, due to:                                                              
         "_HumbugBoxf43f7c5c5350\Roave\BetterReflection\Reflection\ReflectionClass "App\ParentClass" could not be found 
         in the located source".                                                                                        
                                                                                
 [ERROR] Could not process "src/MyClass.php" file, due to:                                                              
         "Class 'App\ParentClass' not found".           

Some classes will just not be available. Some of them are in library folders residing on the server and not par of the repo, some have to be autogenerated using a dev environment that we don't have, etc. Most of my changes are very localized and I don't want to be blocked by missing parents and such.

This is pretty much what happens in every single project.

@TomasVotruba
Copy link
Member

The feature toggle in phpstan.neon does not affect Rector inner reflection, so this is now expected.

@afilina
Copy link
Contributor Author

afilina commented Oct 7, 2020

@TomasVotruba Two questions:

  1. Does it make sense to discover symbols by giving PHPStan the processed files using PHPStan's FileIteratorSourceLocator? This will fix not finding MyClass while analyzing MyClass.php, in case the file is not within parameters.scanDirectories https://github.com/Roave/BetterReflection/blob/4.11.x/docs/usage.md

  2. How do we want to handle missing symbols? For example, we can have MyClass extends ParentClass where ParentClass symbol hasn't been found, but that shouldn't prevent us from using localized rectors like WrapVariableVariableNameInCurlyBracesRector.

@TomasVotruba
Copy link
Member

  1. I have no idea how this part works in PHPStan, so I don't know.

  2. I'd handle it the same way PHPStan does (not sure how PHPStan does it). If PHPStan can handle this case, so should Rector.

@afilina
Copy link
Contributor Author

afilina commented Oct 7, 2020

  1. PHPStan currently throws an exception that you catch. So it's really up to us how we want to deal with it in Rector.

Right now, we're just catching the generic exception and complaining about parameters > autoload_paths:

} catch (AnalysedCodeException $analysedCodeException) {

@TomasVotruba
Copy link
Member

  1. I see. What do you recommend?

@afilina
Copy link
Contributor Author

afilina commented Oct 7, 2020

@TomasVotruba I don't know enough about how Rector indexes the analyzed symbols to answer that. I will need to read a lot more code to have a mental model of that particular process.

We can perhaps catch ClassNotFoundException instead of the generic AnalysedCodeException, then create a stub on the fly and re-analyze the file, but then I don't know what effect that would have downstream. Example: class MyClass extends ParentClass, where Parent is not in any of the analyzed files. We should allow these cases in some way, because they are really common in legacy.

@TomasVotruba
Copy link
Member

#3490 more official knowledge about PHPStan static reflection

Closing as solution is duplicate of #3490

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

4 participants