Is there a possibility to skip a rule based on class attribute?

[tad56]

Hello!
I'm trying to skip a ClassPropertyAssignToConstructorPromotionRector in each class that has #[ORM\Entity] attribute.
Skipping via paths is not an option as I have multiple different places for Entities. Is there a way to do it?

Have a good day!
Tad

[samsonasik]

Why you skip it? is there error when using the rule?

[tad56]

As I checked, withSkip() can take Rule::class. Rule:class => [paths], path
I use this for other situations, but here I'm not able to differentiate all target files by path(s).
It would require our developers to always add new entity class to the rector skip config.

Any options? :)

[TomasVotruba]

@tad56 Thanks for feedback. I'm thinking if this is ORM design in general and if should be skipped by default.

Could you share minimal failing snippet via https://getrector.com/demo ?

[tad56]

I don't think that this rule causes a "problem".
It's rather a matter of our personal feel that "entities" looks more clearer with attributes over class properties instead of __constructor arguments and we would like to keep entities without PHP's property promotion.

here is a snippet what the rule does with entity - https://getrector.com/demo/d3abe37d-b2fc-4359-b4e8-18a5d805800c

What do you mean by minimal failing snippet? (asking because I can't figure out if there is a way to ignore rule by class attribute)

I was thinking about something like

 $rectorConfig->skip(
        [
            ClassPropertyAssignToConstructorPromotionRector::class => [
                new ByAttributeSkipper('Doctrine\ORM\Mapping\Entity'),
               __DIR__ . '/src/otherFolder/or/file
            ]
       ]
  );

[TomasVotruba]

I see. In that case it's better to use the skip feature suggested by @samsonasik

I do this for some other rules, that I personally don't like in entities:

$rector->withSkip([
    ClassPropertyAssignToConstructorPromotionRector::class => '*/Entity/*',
]);

This is the best and easiest way I can think of 👍

[tad56]

ok, still not a solution for me - as I tried to mention earlier, entities in our project don't have similarities in paths. Usually they lay in src/{function}/domain/ - but in the same folder are other classes. Also no similar prefixes/suffixes.

But thank you anyway, I will just not use this rule then.

If you think that there is a place in this project for such solution like 'custom skipper' for a rule I could try to work on such functionality. could be fun, :)
Although I would need some help.

👋

[TomasVotruba]

I know, this requires standardized paths. Before I use this, I move all entities to */Entity/* namespace. It works perfectly with other tools' ignores like PHPStand or ECS, so it's worth the IDE move :)

[uncaught]

I've a similar use case: I would like to skip RestoreDefaultNullToNullableTypePropertyRector for classes extending a specific base class.

The background is, that those classes are value objects mapping json payloads, which are partial. So the only way I can still know if a property was present in the json is by checking if the property was set. With rector setting nullable properties to = null, they are never unset.

[uncaught]

Not sure if the $element here is actually my class or what other shape this could be, but if it was, I could use your SkipVoterInterface and do something like:

 ->withSkip([
   RestoreDefaultNullToNullableTypePropertyRector::class => new class implements SkipVoterInterface {
     public function match($element): bool {
       return is_object($element) || class_exists($element);
     }

     public function shouldSkip($element, string $filePath): bool {
       return (new \ReflectionClass($element))->isSubclassOf(MyBaseClassToSkip::class);
     }
   },
 ])

and @tad56 could then use something like:

 ->withSkip([
   ClassPropertyAssignToConstructorPromotionRector::class => new class implements SkipVoterInterface {
     public function match($element): bool {
       return is_object($element) || class_exists($element);
     }

     public function shouldSkip($element, string $filePath): bool {
       return (new \ReflectionClass($element))->getAttributes(\Doctrine\ORM\Mapping\Entity::class) !== [];
     }
   },
 ])

[uncaught]

Might be nice though, if there was only one method in the interface and if the ReflectionClass was provided directly. That way you wouldn't leak out objects, either.

So maybe a new interface might be cleaner:

interface CustomSkipperInterface {
  public function skip(\ReflectionClass $class): bool;
}

$filePath should be obsolete, too, with ReflectionClass::getFileName() existing.

[TomasVotruba]

Looks good to me :)

Could kick of PR with your proposal?

[uncaught]

I'll try in a few days 👍

[uncaught]

Pulled of a draft: rectorphp/rector-src#6508

Was more work than I had anticipated! I'm probably not going to work on this during the week. Next weekend at the erliest again. I'd appreciate some feedback :)