Option to ignore phpdoc errors #184

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants
@kdubois
Contributor

kdubois commented Aug 13, 2014

We ran into an issue where autowiring was turned off but still happening because annotations were turned on. This in turn caused errors on our end with legacy code that had bad annotations in the constructor, ie:

/**
* @param type $myclass
*/
public function __construct($myclass)
{
    //(...)
}

threw error:

An Unhandled PhpDocReader\AnnotationException Has Occurred: The @param annotation for parameter "myclass" of MyClass::__construct contains a non existent class "type". Did you maybe forget to add a "use" statement for this annotation?

The fix I'm proposing resolved this issue and is tested both with autowiring turned on and turned off.

kdubois added some commits Aug 12, 2014

Update AnnotationDefinitionSource.php
If Autowiring is turned off but annotations are turned on, autowiring still happens.  This fix will prevent this from happening (I'm updating ContainerBuilder.php as well)
Update ContainerBuilder.php
When useAnnotations are turned on, but useAutowiring is turned off, Autowiring is still happening.  Passing useAutowiring to the AnnotationDefinitionSource allows that class to handle autowiring appropriately
@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 13, 2014

Contributor

Sorry for the messy commits, this was my first github contribution. I'm sure you can squash them into a cleaner commit on your end :)

Contributor

kdubois commented Aug 13, 2014

Sorry for the messy commits, this was my first github contribution. I'm sure you can squash them into a cleaner commit on your end :)

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 15, 2014

Member

Hi !

Thanks for the contribution, however I'm not sure I understand what is the problem. You can use either "autowiring" (which completely ignores annotations) or "annotations" (which uses @Inject + @param + autowiring).

If you have a bad @param annotation when you use "annotations", then it makes sense PHP-DI fails (because the annotation is broken). In PHP-DI's language, @param and @Inject are both considered annotations.

The problem with your edit is that:

  1. it breaks backward compatibility
  2. if you don't have @Inject on the constructor it will not be scanned (if $this->useAutoWiring is false) -> that will lead to failures because then PHP-DI will do new SomeClass() with no parameters while the constructor needs some parameters
Member

mnapoli commented Aug 15, 2014

Hi !

Thanks for the contribution, however I'm not sure I understand what is the problem. You can use either "autowiring" (which completely ignores annotations) or "annotations" (which uses @Inject + @param + autowiring).

If you have a bad @param annotation when you use "annotations", then it makes sense PHP-DI fails (because the annotation is broken). In PHP-DI's language, @param and @Inject are both considered annotations.

The problem with your edit is that:

  1. it breaks backward compatibility
  2. if you don't have @Inject on the constructor it will not be scanned (if $this->useAutoWiring is false) -> that will lead to failures because then PHP-DI will do new SomeClass() with no parameters while the constructor needs some parameters
@@ -191,7 +205,7 @@ private function getMethodInjection(ReflectionMethod $method)
// Look for @Inject annotation
/** @var $annotation Inject|null */
try {
- $annotation = $this->getAnnotationReader()->getMethodAnnotation($method, 'DI\Annotation\Inject');
+ $inject_annotation = $this->getAnnotationReader()->getMethodAnnotation($method, 'DI\Annotation\Inject');

This comment has been minimized.

@mnapoli

mnapoli Aug 15, 2014

Member

The codebase follows the PSR-2 coding standard ($injectAnnotation would be better for example)

@mnapoli

mnapoli Aug 15, 2014

Member

The codebase follows the PSR-2 coding standard ($injectAnnotation would be better for example)

This comment has been minimized.

@kdubois

kdubois Aug 15, 2014

Contributor

updated!

@kdubois

kdubois Aug 15, 2014

Contributor

updated!

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 15, 2014

Member

Maybe you can tell me what you expected PHP-DI to do with you example:

/**
* @param type $myclass
*/
public function __construct($myclass)
{
    //(...)
}

What do you want it inject in the constructor when annotations are turned on?

Member

mnapoli commented Aug 15, 2014

Maybe you can tell me what you expected PHP-DI to do with you example:

/**
* @param type $myclass
*/
public function __construct($myclass)
{
    //(...)
}

What do you want it inject in the constructor when annotations are turned on?

@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 15, 2014

Contributor

Salut Mathieu! Maybe our understanding is wrong, but what we're looking for is a way to be able to auto inject only when the @Inject annotation is present. Our assumption was that if Autowiring is turned off, any constructors/methods/properties without the @Inject annotation would be left alone, but seemingly the only way to leave constructors alone is to turn off both annotations and autowiring - which is not waht we want.

The real problem is that we have a large codebase with a lot of legacy code that has docblocks with bad annotations, and we would like to start using PHP-DI without trying to find these issues scattered throughout our codebase, by only adding @Inject to classes where we want PHP-DI to work. Without the fix I proposed, I don't think it's safe to start using PHP-DI in our production environment.

Is the backwards incompatibility you're concerned about with the possibility of applications having autowiring turned off & annotations turned on; and still counting on autowiring to work?

Contributor

kdubois commented Aug 15, 2014

Salut Mathieu! Maybe our understanding is wrong, but what we're looking for is a way to be able to auto inject only when the @Inject annotation is present. Our assumption was that if Autowiring is turned off, any constructors/methods/properties without the @Inject annotation would be left alone, but seemingly the only way to leave constructors alone is to turn off both annotations and autowiring - which is not waht we want.

The real problem is that we have a large codebase with a lot of legacy code that has docblocks with bad annotations, and we would like to start using PHP-DI without trying to find these issues scattered throughout our codebase, by only adding @Inject to classes where we want PHP-DI to work. Without the fix I proposed, I don't think it's safe to start using PHP-DI in our production environment.

Is the backwards incompatibility you're concerned about with the possibility of applications having autowiring turned off & annotations turned on; and still counting on autowiring to work?

@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 15, 2014

Contributor

In the case of the example, if autowiring is off, I would like this bad param annotation to not throw an error. ie I want PHP-DI to completely ignore this constructor.

Contributor

kdubois commented Aug 15, 2014

In the case of the example, if autowiring is off, I would like this bad param annotation to not throw an error. ie I want PHP-DI to completely ignore this constructor.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 15, 2014

Member

OK I see but it doesn't make sense to ignore constructors. You can't new a class and not call its constructor (well actually you can with reflection but that's really not a good thing to do here at all).

Member

mnapoli commented Aug 15, 2014

OK I see but it doesn't make sense to ignore constructors. You can't new a class and not call its constructor (well actually you can with reflection but that's really not a good thing to do here at all).

@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 15, 2014

Contributor

Sorry - to be more clear, I don't want the code to ignore the constructor, I just don't want PHP-DI to try to auto load the dependency or even try to work with the $param annotations if @Inject is not present. ie. Currently the code calling this class passes in the dependency:

$class2 = new class2($myclass);

but based on the bad annotation in the previous code example, I get an error back, even though I'm passing in the right dependency.

Contributor

kdubois commented Aug 15, 2014

Sorry - to be more clear, I don't want the code to ignore the constructor, I just don't want PHP-DI to try to auto load the dependency or even try to work with the $param annotations if @Inject is not present. ie. Currently the code calling this class passes in the dependency:

$class2 = new class2($myclass);

but based on the bad annotation in the previous code example, I get an error back, even though I'm passing in the right dependency.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 15, 2014

Member

OK sorry but this is still not clear.

Given:

class MyClass {
    /**
    * @param type $myclass
    */
    public function __construct($myclass)
    {
        //(...)
    }
}

$obj = $container->get('MyClass');

What should happen? (i.e. what value should the container pass to the constructor?)

Member

mnapoli commented Aug 15, 2014

OK sorry but this is still not clear.

Given:

class MyClass {
    /**
    * @param type $myclass
    */
    public function __construct($myclass)
    {
        //(...)
    }
}

$obj = $container->get('MyClass');

What should happen? (i.e. what value should the container pass to the constructor?)

@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 15, 2014

Contributor

So where I'm running into the issue is in a 'master' class where I added:

class MasterClass {
    public function __construct()
    {
        // Add Dependency Injection Container
        $builder = new \DI\ContainerBuilder();
        $builder->useAutowiring(false);
        $container = $builder->build();
        $container->injectOn($this);
    }
}

A child class of this master class has the offending @param type myclass:

class MyClass extends MasterClass {
    /**
    * @param type $myotherclass
    */
    public function __construct($myotherclass)
    {
        parent::__construct();
        $this->myotherclass = $myotherclass;
        //(...)
    }
}

$obj = new MyClass(new myotherclass());

And that results in the following error:

An Unhandled PhpDocReader\AnnotationException Has Occurred: The @param annotation for parameter "myotherclass" of MyClass::__construct contains a non existent class "type". Did you maybe forget to add a "use" statement for this annotation?

I understand that this is not an optimal setup and that I can (and should) just change @param type $myotherclass to @param myotherclass $myotherclass but that's not a workable solution for our codebase right now..

Contributor

kdubois commented Aug 15, 2014

So where I'm running into the issue is in a 'master' class where I added:

class MasterClass {
    public function __construct()
    {
        // Add Dependency Injection Container
        $builder = new \DI\ContainerBuilder();
        $builder->useAutowiring(false);
        $container = $builder->build();
        $container->injectOn($this);
    }
}

A child class of this master class has the offending @param type myclass:

class MyClass extends MasterClass {
    /**
    * @param type $myotherclass
    */
    public function __construct($myotherclass)
    {
        parent::__construct();
        $this->myotherclass = $myotherclass;
        //(...)
    }
}

$obj = new MyClass(new myotherclass());

And that results in the following error:

An Unhandled PhpDocReader\AnnotationException Has Occurred: The @param annotation for parameter "myotherclass" of MyClass::__construct contains a non existent class "type". Did you maybe forget to add a "use" statement for this annotation?

I understand that this is not an optimal setup and that I can (and should) just change @param type $myotherclass to @param myotherclass $myotherclass but that's not a workable solution for our codebase right now..

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 15, 2014

Member

OK thank you I see now!

What do you think about a global option that would be like "ignore invalid classes in @param annotations" instead?

Because autowiring means using the type-hints. So @param is not autowiring, it's really "annotations" (it doesn't make sense to disable it if "autowiring" is disabled). Having an option for ignoring bad @param phpdoc would be better IMO.

That would require modifying mnapoli/PhpDocReader to have such an option (it would be an option to prevent this exception from being thrown). And then PHP-DI should also have the same option, that is forwarded to PhpDocReader.

For example, at a high level, it could be used like this:

$builder = new ContainerBuilder();
$builder->ignorePhpDocErrors(true);

$container = $builder->build();
Member

mnapoli commented Aug 15, 2014

OK thank you I see now!

What do you think about a global option that would be like "ignore invalid classes in @param annotations" instead?

Because autowiring means using the type-hints. So @param is not autowiring, it's really "annotations" (it doesn't make sense to disable it if "autowiring" is disabled). Having an option for ignoring bad @param phpdoc would be better IMO.

That would require modifying mnapoli/PhpDocReader to have such an option (it would be an option to prevent this exception from being thrown). And then PHP-DI should also have the same option, that is forwarded to PhpDocReader.

For example, at a high level, it could be used like this:

$builder = new ContainerBuilder();
$builder->ignorePhpDocErrors(true);

$container = $builder->build();
@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 15, 2014

Contributor

That would work great! I'll work on that later today, and I'll revert the other changes.

Contributor

kdubois commented Aug 15, 2014

That would work great! I'll work on that later today, and I'll revert the other changes.

@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 21, 2014

Contributor

I added the ignorePhpDocErrors flag, and updated the phpdocreader dependency in composer to 1.3 (the version that has the ignorePhpDocErrors support

Contributor

kdubois commented Aug 21, 2014

I added the ignorePhpDocErrors flag, and updated the phpdocreader dependency in composer to 1.3 (the version that has the ignorePhpDocErrors support

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 24, 2014

Member

Thanks. Just a heads up, I'm in holiday right now for a couple weeks more, so I have random internet access and not much time. If this PR doesn't move a lot it's not because I've forgotten about it ;)

Member

mnapoli commented Aug 24, 2014

Thanks. Just a heads up, I'm in holiday right now for a couple weeks more, so I have random internet access and not much time. If this PR doesn't move a lot it's not because I've forgotten about it ;)

src/DI/ContainerBuilder.php
+ {
+ $this->ignorePhpDocErrors = $bool;
+ return $this;
+ }

This comment has been minimized.

@mnapoli

mnapoli Aug 24, 2014

Member

Watch out you indented with tabs instead of spaces (should be PSR-2 compliant)

@mnapoli

mnapoli Aug 24, 2014

Member

Watch out you indented with tabs instead of spaces (should be PSR-2 compliant)

src/DI/Definition/DefinitionManager.php
+ /**
+ *
+ * @param \DI\Definition\Source\DefinitionSource $source
+ */

This comment has been minimized.

@mnapoli

mnapoli Aug 24, 2014

Member

this docblock is useless because it duplicates the type-hinting

@mnapoli

mnapoli Aug 24, 2014

Member

this docblock is useless because it duplicates the type-hinting

+ public function __construct($ignorePhpDocErrors = false)
+ {
+ $this->ignorePhpDocErrors = $ignorePhpDocErrors;
+ }

This comment has been minimized.

@mnapoli

mnapoli Aug 24, 2014

Member

tabs again it seems

@mnapoli

mnapoli Aug 24, 2014

Member

tabs again it seems

@@ -202,7 +215,7 @@ private function getMethodInjection(ReflectionMethod $method)
}
$annotationParameters = $annotation ? $annotation->getParameters() : array();
- // @Inject on constructor is implicit
+ // if this is not a constructor; and annotations are turned off, then we don't want to continue.

This comment has been minimized.

@mnapoli

mnapoli Aug 24, 2014

Member

Could you restore the original comment please? (I think that's a leftover from the previous implementation)

@mnapoli

mnapoli Aug 24, 2014

Member

Could you restore the original comment please? (I think that's a leftover from the previous implementation)

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 24, 2014

Member

The code looks good (except minor points I added inline), just FYI your last commits are not linked to your GitHub account (for some reason, probably the email address, it happened to me, anyway that's not a problem it's just for you).

Member

mnapoli commented Aug 24, 2014

The code looks good (except minor points I added inline), just FYI your last commits are not linked to your GitHub account (for some reason, probably the email address, it happened to me, anyway that's not a problem it's just for you).

@kdubois

This comment has been minimized.

Show comment
Hide comment
@kdubois

kdubois Aug 25, 2014

Contributor

Thanks Mathieu, I fixed the indentation, removed the docblock and reverted the inject on constructor comment. Thanks for the heads up on the github email assocation by the way :)

Contributor

kdubois commented Aug 25, 2014

Thanks Mathieu, I fixed the indentation, removed the docblock and reverted the inject on constructor comment. Thanks for the heads up on the github email assocation by the way :)

@mnapoli mnapoli changed the title from Disable implicit autowiring when in annotation mode and autowiring is turned off. to Option to ignore phpdoc errors Sep 1, 2014

@mnapoli mnapoli added the enhancement label Sep 1, 2014

@mnapoli mnapoli added this to the 4.4 milestone Sep 1, 2014

mnapoli added a commit that referenced this pull request Sep 1, 2014

@mnapoli mnapoli referenced this pull request Sep 1, 2014

Merged

4.4 #187

7 of 7 tasks complete
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 1, 2014

Member

Good I have merged it manually into the 4.4 branch (pull request #187), it will show as "merged" here when 4.4 is merged into master.

Thank you for the contribution.

Member

mnapoli commented Sep 1, 2014

Good I have merged it manually into the 4.4 branch (pull request #187), it will show as "merged" here when 4.4 is merged into master.

Thank you for the contribution.

@mnapoli mnapoli closed this Sep 1, 2014

mnapoli added a commit that referenced this pull request Sep 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment