Rector: Allow other namespaces for @Inject in JmsInjectAnnotationRector

[holtkamp]

I was checking whether I could use rector to change my PHP-DI @Inject annotation-based dependency injections to constructor based injections.

To my big surprise I found out about an already existing rector:

https://github.com/rectorphp/rector/blob/master/docs/AllRectorsOverview.md#jmsinjectannotationrector

The only difference seems to be that the PHP-DI @Inject annotation does not have/use a namespace. That made me wonder, instead of making a new rector, would it be a (good) idea to change https://github.com/rectorphp/rector/blob/fac1503915aea843a3c123a5c622ecf0708bda62/packages/Jms/src/Rector/Property/JmsInjectAnnotationRector.php in such a way that the namespace of the annotation can be configured / left empty, probably defaulting to https://github.com/rectorphp/rector/blob/fac1503915aea843a3c123a5c622ecf0708bda62/packages/Jms/src/Rector/Property/JmsInjectAnnotationRector.php#L28

[TomasVotruba]

Hi,

so it's basically the very same change just with different annotation?

What will happen when you change it in JmsInjectAnnotationRector manually?

If that's what helps, we can make the rule configurable.

[holtkamp]

What will happen when you change it in JmsInjectAnnotationRector manually?

Will give it a shot and report back here, this week is quite busy, so might take some time.

[holtkamp]

I finally found some time to try this out.

Using rector 0.5.5 and the following rector.yaml file:

# rector.yaml
services:
  Rector\Rector\Property\InjectAnnotationClassRector:
    $annotationClass: 'Inject'

This results in an error:

Make sure "kernel_class" parameters is set in rector.yaml in "parameters:" section

Which seems to indicate this functionality is only available for Symfony applications?

When the property type indicates that it is a FQCN (using class_exists()?), this type could probably be used directly instead of having the AnalyzedApplicationContainer determine the type. This way it should also work for "non-Symfony" applications.

Not sure this is possible, just thinking out loud 😄

class Example {

  /**
   * @Inject
   * @var \Fully\Qualified\Class\Name\To\Dependency
   */
  private $someDependency;

}

can then become:

class Example {

  /**
   * @var \Fully\Qualified\Class\Name\Of\Dependency
   */
  private $someDependency;

  public function __construct(\Fully\Qualified\Class\Name\Of\Dependency $someDependency)
  {
    $this->someDependency = $someDependency
  }
}

[TomasVotruba]

Thanks for testing!

What exact bin command do you run?

[holtkamp]

I used:

./vendor/bin/rector process path/to/some/files/with/simple/inject/annotations  

[holtkamp]

@TomasVotruba nice, works great!

A "nice to have" I encountered:

• re-use an imported class alias if applicable

Currently the FQCN of the dependency is used, but it might be an alias:

use Fully\Qualified\Class\Name\To\Dependency as MyDependency;

class Example {

  /**
   * @Inject
   * @var MyDependency
   */
  private $someDependency;

}

Maybe I can come up with a PR for this 😄

UPDATE
Tried to use an import by changing resolveType() to :

        $varTypeInfo = $this->docBlockManipulator->getVarTypeInfo($node);
        if ($varTypeInfo !== null) {
            if($varTypeInfo->getType() !== null){
                return ltrim($varTypeInfo->getType(), '\\');
            }
            if($varTypeInfo->getFqnType() !== null){
                return ltrim($varTypeInfo->getFqnType(), '\\');
            }
        }

but apparently the type gets always prefixed with a backslash. So let's leave it for now / have another rector apply this optimization.

[TomasVotruba]

That could be done :)

Tried to use an import by changing resolveType() to :

Good start!

When you send PR with test case for this, I'll navigate you to the finish.

[holtkamp]

When you send PR with test case for this, I'll navigate you to the finish.

To be honest: by using "Code" => "Code Cleanup" in PHPStorm the aliases of the imported classes are used properly, so this kind of "reduces" this "nice to have" request 😉 .

Protected properties change to private?
Something more important: it seems that the visibility of protected properties is changed to private due to this line:

https://github.com/rectorphp/rector/blob/125dac40669e16ca26224311f07fad4024bf4464/src/Rector/Property/InjectAnnotationClassRector.php#L199

Not sure what the rationale / idea of this is. But in case specialized classes extend the class which is "rectored", than these properties become unavailable to them..

[TomasVotruba]

this "nice to have" request 😉 .

I see :). There is also Rector rule that does the same. Unless it breaks the code, let's keep it this way.

Not sure what the rationale / idea of this is.

Well, since costructor is used now, the value doesn't have to be available publicly for container magic injection.

But in case specialized classes extend the class which is "rectored", than these properties become unavailable to them..

I see. What would you suggest?

[holtkamp]

the value doesn't have to be available publicly for container magic injection.

I don't think this is always the case. When using PHP-DI, one can also use @Inject on private properties. So (in this case) the container magic also works on private stuff...

What would you suggest?

Not making this rector act "too smart" and not change visibility. KISS, maybe this is functionality for a separate rector?

Possible bug alert
And I think I found a bug as well which is worth investigating...

It seems given a class:

<?php

use Some\Random\Other\ClassName;

class Example {

  /**
   * @Inject
   * @var \Fully\Qualified\Class\Name\To\Dependency\ClassName
   */
  private $someDependency;

}

The rector currently changes this to

<?php

use Some\Random\Other\ClassName;

class Example {

  /**
   * @var \Fully\Qualified\Class\Name\Of\Dependency\ClassName
   */
  private $someDependency;

  public function __construct(ClassName $someDependency)
  {
    $this->someDependency = $someDependency
  }
}

The "imported" class name (or alias?) seems to be re-used, which can be correct, but sometimes is incorrect... To contribute, I can try to come up with a failing test case coming weekend 👍

[holtkamp]

Regarding my "possible bug alert" in https://github.com/rectorphp/rector/issues/1400#issuecomment-508675676

I think i managed to reproduce it, see

https://github.com/rectorphp/rector/issues/1400#issuecomment-508675676

and

https://github.com/holtkamp/rector/commit/0c93729d995d71ff8ac4e8fd73dcfbda81e4f7d1

For a failing test case

[TomasVotruba]

This is not a bug. Use coding standards to handle this

[holtkamp]

This is not a bug.

Well, see https://github.com/rectorphp/rector/pull/1731, this "not bug" causes the rector to break valid code, since a property will get a different type assigned.

Let's discuss it further in https://github.com/rectorphp/rector/issues/1729

[TomasVotruba]

Let's discuss it further in #1729

:+1: mixing issues is a mess

[holtkamp]

A bit more on-topic: I worked on a rector to remove @Inject annotations from ZendFramework 1 FrontControllers.

The FrontController of the framework itself has a constructor, making it "impossible" / nasty to use constructor based dependency injection. This limitation is also described here: http://www.johnhall.io/2017/06/25/zf1-controller-dependency-injection.html

Instead of using an ActionHelper as described in the article above: the envisioned rector collects all properties that need to be injected in an associative array "propertyName" => "classname" and sets that as class constant. Specializing classes "append" that class constant array with "own" dependencies.

While working on the rector, I noticed that support to work with "constants" is a bit limited, so my work might inspire to improve such support later on 🤓 . The rector works, hopefully I can find some time to add some tests and open a PR soon!

[TomasVotruba]

New issue