nette / php-generator

🐘 Generates neat PHP code for you. Supports new PHP 8.3 features.
https://doc.nette.org/php-generator
Other
2.11k stars 138 forks source link

Loading from code should not use FQCN #97

Closed alanpoulain closed 2 years ago

alanpoulain commented 2 years ago

Version: 3.6.4

Bug Description

When loading from the code (PhpFile::fromCode($content)), everything is resolved to its FQCN because of the NameResolver visitor. It is then very difficult to merge new generated code with the loaded one, since the first one is not using FQCN.

Expected Behavior

Loading from code should not use FQCN for its nodes.

Possible Solution

Either do not use NameResolver when loading, or use the originalName attribute of the node instead.

dg commented 2 years ago

When exporting to PHP, FQCNs are simplified with respect to the current namespace and use-statements, see this test https://github.com/nette/php-generator/blob/master/tests/PhpGenerator/Extractor.extractAll.resolving.phpt

alanpoulain commented 2 years ago

Are you sure it works for the attributes too? I have encountered this issue for them.

dg commented 2 years ago

Can you show a example that didn't work?

alanpoulain commented 2 years ago

OK my bad, it was because setTypeResolving(false) was called on the printer. However enabling type resolving creates another issue: if the attribute added manually is not a FQCN, it will be resolved to the root namespace, even if there is a use for it. For instance:

$phpFileFromCode = PhpFile::fromCode(<<<END
    <?php

    namespace App\Entity;

    /**
     * The most generic type of item.
     *
     * @see https://schema.org/Thing
     */
    class Thing
    {
    }
    END
);
foreach ($phpFileFromCode->getNamespaces() as $namespace) {
    $namespace->addUse('Foo\Bar');
}
foreach ($phpFileFromCode->getClasses() as $class) {
    $class->addAttribute('Bar');
}
echo (new Printer())->printFile($phpFileFromCode);

will generate:

<?php

namespace App\Entity;

use Foo\Bar;

/**
 * The most generic type of item.
 *
 * @see https://schema.org/Thing
 */
#[\Bar]
class Thing
{
}

That's why setTypeResolving(false) has been added in the first place. Shouldn't the type be resolved if there is a use for it?

dg commented 2 years ago

Imagine the following example:

$phpFileFromCode = PhpFile::fromCode(<<<END
    <?php

    namespace App\Entity;

    class Thing
    {
        function test()
        {
            return new Bar;
        }
    }
    END
);
foreach ($phpFileFromCode->getNamespaces() as $namespace) {
    $namespace->addUse('Foo\Bar');
}
echo (new Printer())->printFile($phpFileFromCode);

What should be the correct output? And what should be the correct output with setTypeResolving(false)?

If you want to change use-statements, you risk changing the meaning of the code inside methods. I don't think the library should allow that.

A more robust approach is to leave type resolving enabled and use the FQN everywhere.

dg commented 2 years ago

I've added the resolveName method, so you should be able to use this:

foreach ($phpFileFromCode->getClasses() as $class) {
    $class->addAttribute($namespace->resolveName('Bar')); // converts to App\Entity\Bar
}
alanpoulain commented 2 years ago

Thank you very much, it works well :slightly_smiling_face: