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

Can I still add my own, project-specific rules easily? #6404

Closed
mpdude opened this issue May 11, 2021 · 11 comments
Closed

Can I still add my own, project-specific rules easily? #6404

mpdude opened this issue May 11, 2021 · 11 comments

Comments

@mpdude
Copy link
Contributor

mpdude commented May 11, 2021

Now that Rector comes prefixed by default, I wonder if I can still add new rules easily as described in https://github.com/rectorphp/rector/blob/main/docs/create_own_rule.md.

I mean – when my project just declares rector/rector as a dev dependency, all the classes that are imported in the example code given are prefixed/namespaced, right?

How can this even work, am I missing something?

@TomasVotruba
Copy link
Member

TomasVotruba commented May 11, 2021

There are few namespaces that are not prefixed - typically Rector, PHPParser and PHPStan:
https://github.com/rectorphp/rector-src/blob/afd75171fbf325d07d404c4ad3ecded81757f73b/utils/compiler/src/PhpScoper/StaticEasyPrefixer.php#L24-L41

If you need some extra classes, they have to be required in your custom package composer.json.


The goal of prefixing is to avoid conflict with Symfony, Nette, Doctrine etc. conflicts of Rector and your project.

@sabbelasichon
Copy link
Contributor

sabbelasichon commented May 11, 2021

@mpdude I would say, this should be possible most of the time, not only theoretically ;-)
But of course there are some exceptions. So, if you need i.e. something from the symfony/filesystem dependency you should explicitly require this dependency in your project itself. This stands also for other classes you could have used implicitly in your rules for the sake of convenience.

@TomasVotruba
Copy link
Member

The rule of thumb is the same with your project. If you need e.g. YAML component in your code, you should not hope some of existing dependencies already requires it :)

You should require it in your composer.json explicitly. It might happens (an happens) that the dependency you use to get YAML will drop it and suddenly the YAML is missing and your project crashes.

@mpdude
Copy link
Contributor Author

mpdude commented May 11, 2021

Take a look at the namespace imports in the example/documentation:

use Nette\Utils\Strings;
use PhpParser\Node;
use PhpParser\Node\Identifier;
use PhpParser\Node\Expr\MethodCall;
use Rector\Core\Rector\AbstractRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

Strictly speaking, when I write a rule like in the example, I'd need to declare all packages as (dev) dependencies that are needed to get the mentioned classes.

So... should I require all the relevant packages? For example, nette/utils is a Rector dependency, but will be prefixed. I will end up with "my" version together with the prefixed version brought along by Rector.

Conversely, nikic/php-parser is brought along by Rector, but is neither prefixed nor declared as replaces or something else in composer.json, with the possibility of mixing autoloaders and/or incompatible versions of the code.

Oh, and regarding which classes Rector brings along and/or prefixes: Is that part of the API of Rector, i. e. can I rely on that list not to change on minor version updates?

@TomasVotruba
Copy link
Member

For nette\utils yes, you have to require your own version.
The rest is included in Rector package, as linked in my first comment: #6404 (comment)

@TomasVotruba
Copy link
Member

For nikic/php-parser, your project will require your own version, but when running Rector, it will load it's own version of nikic/php-parser. That way each project uses their own prefered dependency version :)

This one is handled by preload.php that Rector includes: https://github.com/rectorphp/rector/blob/main/preload.php
PHPStan is using the same approach to avoid same conflict.

@sabbelasichon
Copy link
Contributor

Maybe, i don´t get it right now, but why do we need a changing prefix? Why not have a fixed prefixed so a 3rd party consumer can rely on prefixed classes?

@sabbelasichon
Copy link
Contributor

sabbelasichon commented May 11, 2021

For me it is quite hard to benefit from the prefixed version right now. I have to require a lot of packages which are already part of rector itself and some things don´t went smoothly. I.e. if i am requiring the rector/rector-generator package the function service from the symfony/dependency-injection container cannot be found / autoloaded anymore.

@TomasVotruba
Copy link
Member

Maybe, i don´t get it right now, but why do we need a changing prefix? Why not have a fixed prefixed so a 3rd party consumer can rely on prefixed classes?

To be honest, it's mostly a convention. Another reason might be composer internals, that autoload might not get triggered propperly... but that's just my rought assumption.

Saying that, we can give it a try :)

@TomasVotruba
Copy link
Member

the function service from the symfony/dependency-injection container cannot be found / autoloaded anymore.

This should be working. That's a bug

@TomasVotruba
Copy link
Member

Closing as answered.

tl;dr; If you write your own rules with packages that used to be in Rector but now are scoped, there packages have to be required in your project explicitly. Same way you need to require packages in your app to make sure the class is there.

TomasVotruba added a commit that referenced this issue Oct 29, 2024
TomasVotruba added a commit that referenced this issue Oct 30, 2024
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

No branches or pull requests

3 participants