ContextFactory incorrect namespace aliases

[piku235]

Following the example below:

namespace App;

use Common\Domain\DayOfWeek;

class Foo 
{
    /**
     * @var DayOfWeek
     */
    private $dayOfWeek;
}

Unfortunately, instead of the typeCommon\Domain\DayOfWeek on the dayOfWeek property, its type is App\DayOfWeek.

After a longer digging, I stumbled upon ContextFactory::extractUseStatements method. The problem here is how this method uses aliasing.
For the example above the Context::getNamespaceAliases call will end up with

array('Common\Domain\DayOfWeek' => 'Common\Domain\DayOfWeek')

instead of

array('DayOfWeek' => 'Common\Domain\DayOfWeek')

Also, I noticed the tests testReadsAliasesFromClassReflection, testReadsAliasesFromProvidedNamespaceAndContent are incorrect for ContextFactory, that's why it looks like no one has been able to notice it all this time.

[jaapio]

Hm, I need to check if this was intended by design or that this is a bug. It is hard to believe that a library that is so widely used has a major issue like this one.
At first sight it looks like a bug to me. But that depends on how you are using this library.

It would be helpful if you can provide a link to your project that currently uses this library.

[piku235]

I wanted to use it in one of my private projects and immediately stumbled upon this issue. I use it with the symfony/property-info component that has PhpDocExtractor and uses the phpDocumentator. I think you can find what is wrong by looking into the tests I fixed in my PR #77.

Actually, I think I found when the bug exactly came. When you go back to the commit f989f458daac6b90f2ebbcbc729e774caec493e4 that is before the commit 51761af97432e694cf1c40910bde4aa62c6cd603 you'll see the original behavior matches my expectations, so simply the commit Updated namespace parser to handle grouped namespaces changed the behavior and because of the broken tests, they weren't able to detect that slight change.

Referring even to php docs you can find such examples:

use My\Full\Classname as Another;

// this is the same as use My\Full\NSname as NSname
use My\Full\NSname;

That's why I believe the ContextFactory::extractUseStatements should return

array('NSname' => 'My\Full\NSname')

instead of

array('My\Full\NSname' => 'My\Full\NSname')

The only workaround for now is to use:

use My\Full\NSname as NSname;

But as you can tell it shouldn't work like that.

Regarding the tests of ContextFactory there're two things that caught my attention.

1. testReadsAliasesFromClassReflection

$expected = [
       'm' => m::class,
       'DocBlock' => DocBlock::class,
       'Tag' => Tag::class,
       'phpDocumentor' => 'phpDocumentor',
       'TestCase' => TestCase::class,
       'Assert' => Assert::class,
      'e' => e::class,
       ReflectionClass::class => ReflectionClass::class,
      'stdClass',
];
sort($expected);
sort($actual);
$this->assertSame($expected, $actual);

The sort function actually sorts by "altering" keys in the provided array, in the end only "values" are compared as identical.
2. testReadsAliasesFromProvidedNamespaceAndContent
Following the previous case, but this one is bit different.

$this->assertSame(sort($expected), sort($actual));

The result of sort function is always a boolean value (true on success, false otherwise), so this assertion simply is wrong, it always just compares boolean's.

[jaapio]

Thanks for this awesome description and search work. This helped a lot in understanding where things went wrong.

[piku235]

You're welcome, good to hear it helped. :)

[jaapio]

Released v 0.7.2 and 1.0.1 including the fix suggested in #77.