Why is there a check to resolve types in the targets namespace prior to the Use clauses? #87

Closed
drdamour opened this Issue Jul 19, 2013 · 8 comments

Comments

Projects
None yet
2 participants
@drdamour
Contributor

drdamour commented Jul 19, 2013

given this:

namespace yyy;

use Some\Namespace\SomeDependency;

class X{

    /**
     * @Inject
     * @var SomeDependency
     */
    protected $serviceFactory = null;

}

I've found that the classDefintion is attempting to resolve a class "yyy\SomeDependency" prior to attempting to resolve "Some\Namespace\SomeDependency" .

The code (PhpDocParser::getPropertyType) seems to attempt to resolve the type from the namespace of the injection target prior to the fully qualified name coming form a use statement.

Is this the correct behaviour? I would think that the use statement forces the SomeDependency alias to resolve to Some\Namespace\SomeDependency for the entire class defintion (or better code scoped under that use statement) such that it could never point to a "yyy\SomeDependency" type. Although i may be wrong with PHP's naming resolution here...

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 20, 2013

Member

If I understand correctly your question, this shouldn't be a problem because you can't have SomeDependency both in yyy and imported with use Some\Namespace\SomeDependency;.

It actually make sense, you can't have a class twice with the same name. See http://3v4l.org/WVZIu

So I guess the order of check isn't really important for it to work. The only thing that the order could affect is performance maybe.

Member

mnapoli commented Jul 20, 2013

If I understand correctly your question, this shouldn't be a problem because you can't have SomeDependency both in yyy and imported with use Some\Namespace\SomeDependency;.

It actually make sense, you can't have a class twice with the same name. See http://3v4l.org/WVZIu

So I guess the order of check isn't really important for it to work. The only thing that the order could affect is performance maybe.

@drdamour

This comment has been minimized.

Show comment
Hide comment
@drdamour

drdamour Jul 20, 2013

Contributor

yup it's just another performance optimization. The Zend loader attempts to include the yyy\SomeDependency (which in general it probably would be not..i mean what are the odds?) and will log a warning about it not being found. (If the loader checked before it loaded it, the warning wouldn't be there. it's funny how this poor loader is exposing some stuff with PHP-DI)

I'd suggest flipping the order to match up with PHP's name resolution. I'll put together a pull request for it if you're in agreement.

Contributor

drdamour commented Jul 20, 2013

yup it's just another performance optimization. The Zend loader attempts to include the yyy\SomeDependency (which in general it probably would be not..i mean what are the odds?) and will log a warning about it not being found. (If the loader checked before it loaded it, the warning wouldn't be there. it's funny how this poor loader is exposing some stuff with PHP-DI)

I'd suggest flipping the order to match up with PHP's name resolution. I'll put together a pull request for it if you're in agreement.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 20, 2013

Member

Well in terms of optimizing, I was thinking this is mostly unsignificant since this will be called only once ever because afterwards it is cached.

And to optimize, I see that the PhpDocParser first parses the class file, then does the trivial check with the namespace, then uses the result of the parsing for resolving use statements. See here.

This could be optimized by parsing the file after the trivial tests:

if ($this->classExists($class->getNamespaceName() . '\\' . $type)) {
...
} elseif ($this->classExists($type)) {

(because I imagine parsing the file is not trivial)

I believe what you suggest wouldn't be optimizing performance-wise, it would be mostly to avoid Zend's loader warning. I really think this is unfortunate that Zend's autoloader works that way, a simple file_exists before trying to require it would be enough (that's what Composer do I think) (sigh... zend 😕 ).

I really don't know what to do about Zend's warning. Do you use $autoloader->suppressNotFoundWarnings(true);? Do you still have the warning in your logs?

Member

mnapoli commented Jul 20, 2013

Well in terms of optimizing, I was thinking this is mostly unsignificant since this will be called only once ever because afterwards it is cached.

And to optimize, I see that the PhpDocParser first parses the class file, then does the trivial check with the namespace, then uses the result of the parsing for resolving use statements. See here.

This could be optimized by parsing the file after the trivial tests:

if ($this->classExists($class->getNamespaceName() . '\\' . $type)) {
...
} elseif ($this->classExists($type)) {

(because I imagine parsing the file is not trivial)

I believe what you suggest wouldn't be optimizing performance-wise, it would be mostly to avoid Zend's loader warning. I really think this is unfortunate that Zend's autoloader works that way, a simple file_exists before trying to require it would be enough (that's what Composer do I think) (sigh... zend 😕 ).

I really don't know what to do about Zend's warning. Do you use $autoloader->suppressNotFoundWarnings(true);? Do you still have the warning in your logs?

@drdamour

This comment has been minimized.

Show comment
Hide comment
@drdamour

drdamour Jul 20, 2013

Contributor

actually file_exists won't look in all include paths like include_once will, you want to use http://php.net/stream_resolve_include_path . Problem with suppression is that it's that nasty @ that really scares me. And yes, parsing the use statements certainly has some level of cost.

But after thinking about it a bit more..i'm thinking this could be a BUG.. In the previous example:

namespace yyy;

use Some\Namespace\SomeDependency;

class X{

    /**
     * @Inject
     * @var SomeDependency
     */
    protected $serviceFactory = null;

}

Assuming no defs exist...the DI should inject an instance of Some\Namespace\SomeDependency...but suppose a yyy\SomeDependency class exists. I believe the DI layer would actually inject the yyy\SomeDependency type.

Here's the execution code:

            $found = false;

            if ($this->classExists($class->getNamespaceName() . '\\' . $type)) {
                $type = $class->getNamespaceName() . '\\' . $type;
                $found = true;
            } elseif (isset($uses[$loweredAlias])) {
                // Imported classes
                if (false !== $pos) {
                    $type = $uses[$loweredAlias] . substr($type, $pos);
                } else {
                    $type = $uses[$loweredAlias];
                }
                $found = true;

If yyy\SomeDependency exists, then the first if check would be true, and the $type would be incorrectly set to yyy\SomeDependency. This would be contrary to the use statement in the injection target that defines the local alias type of SomeDependency to Some\Namespace\SomeDependency, right?

I bet this isn't encountered too often (or ever) because of the unlikeliness of the conflict. I can probably put together a unit test to prove the bug, but not tonight.

Contributor

drdamour commented Jul 20, 2013

actually file_exists won't look in all include paths like include_once will, you want to use http://php.net/stream_resolve_include_path . Problem with suppression is that it's that nasty @ that really scares me. And yes, parsing the use statements certainly has some level of cost.

But after thinking about it a bit more..i'm thinking this could be a BUG.. In the previous example:

namespace yyy;

use Some\Namespace\SomeDependency;

class X{

    /**
     * @Inject
     * @var SomeDependency
     */
    protected $serviceFactory = null;

}

Assuming no defs exist...the DI should inject an instance of Some\Namespace\SomeDependency...but suppose a yyy\SomeDependency class exists. I believe the DI layer would actually inject the yyy\SomeDependency type.

Here's the execution code:

            $found = false;

            if ($this->classExists($class->getNamespaceName() . '\\' . $type)) {
                $type = $class->getNamespaceName() . '\\' . $type;
                $found = true;
            } elseif (isset($uses[$loweredAlias])) {
                // Imported classes
                if (false !== $pos) {
                    $type = $uses[$loweredAlias] . substr($type, $pos);
                } else {
                    $type = $uses[$loweredAlias];
                }
                $found = true;

If yyy\SomeDependency exists, then the first if check would be true, and the $type would be incorrectly set to yyy\SomeDependency. This would be contrary to the use statement in the injection target that defines the local alias type of SomeDependency to Some\Namespace\SomeDependency, right?

I bet this isn't encountered too often (or ever) because of the unlikeliness of the conflict. I can probably put together a unit test to prove the bug, but not tonight.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 20, 2013

Member

You are right!

That's quite unlikely of course, and that's a very edgy situation because imagine the same example without DI:

<?php
namespace yyy;

use Some\Namespace\SomeDependency;

class X{
    public function __construct() {
        $o = new SomeDependency();
    }
}

Assuming:

  • Some\Namespace\SomeDependency exists
  • yyy\SomeDependency exists

This code would:

  • fail (PHP error) if yyy\SomeDependency was included before in the code
  • else create an object of type Some\Namespace\SomeDependency

So that's very edgy :)

But you are right, to be consistent with how PHP works use statements need to be checked before trying to load the class using the current namespace.

So I'm OK if you want to open a pull request :) (replicating that with a tests seems overkill to me, but if you want to feel free).

Member

mnapoli commented Jul 20, 2013

You are right!

That's quite unlikely of course, and that's a very edgy situation because imagine the same example without DI:

<?php
namespace yyy;

use Some\Namespace\SomeDependency;

class X{
    public function __construct() {
        $o = new SomeDependency();
    }
}

Assuming:

  • Some\Namespace\SomeDependency exists
  • yyy\SomeDependency exists

This code would:

  • fail (PHP error) if yyy\SomeDependency was included before in the code
  • else create an object of type Some\Namespace\SomeDependency

So that's very edgy :)

But you are right, to be consistent with how PHP works use statements need to be checked before trying to load the class using the current namespace.

So I'm OK if you want to open a pull request :) (replicating that with a tests seems overkill to me, but if you want to feel free).

@drdamour

This comment has been minimized.

Show comment
Hide comment
@drdamour

drdamour Jul 20, 2013

Contributor

I believe the code would not error if yyy\SomeDependency were included before the code via a require/include statement: http://www.php.net/manual/en/language.namespaces.faq.php#language.namespaces.faq.conflict

only if it was declared in the same file would it cause a conflict. use statements are apparently scoped to the file they are present in and can override global scope.

that's weird, cause if you were to pre-optimize your code base by merging files you'd get different results. Probably a penalty of adding namespaces a decade & a half after the language exists...

Contributor

drdamour commented Jul 20, 2013

I believe the code would not error if yyy\SomeDependency were included before the code via a require/include statement: http://www.php.net/manual/en/language.namespaces.faq.php#language.namespaces.faq.conflict

only if it was declared in the same file would it cause a conflict. use statements are apparently scoped to the file they are present in and can override global scope.

that's weird, cause if you were to pre-optimize your code base by merging files you'd get different results. Probably a penalty of adding namespaces a decade & a half after the language exists...

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 20, 2013

Member

Right! Well that's one more reason to say this is a "bug" then :)

Member

mnapoli commented Jul 20, 2013

Right! Well that's one more reason to say this is a "bug" then :)

@mnapoli mnapoli referenced this issue Jul 28, 2013

Merged

3.3 #90

7 of 7 tasks complete

mnapoli added a commit that referenced this issue Jul 30, 2013

Merge pull request #93 from drdamour/3.3
For #87 changed the annotation type parser to resolve with use statement...
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Jul 30, 2013

Member

Fixed in 3.3

Member

mnapoli commented Jul 30, 2013

Fixed in 3.3

@mnapoli mnapoli closed this Jul 30, 2013

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