Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enum as a Attribute parameter #930

Open
piotrooo opened this issue Jun 27, 2023 · 4 comments · May be fixed by #940
Open

Enum as a Attribute parameter #930

piotrooo opened this issue Jun 27, 2023 · 4 comments · May be fixed by #940

Comments

@piotrooo
Copy link

piotrooo commented Jun 27, 2023

Hello!

I think I found bug. Lets consider following snippet:

<?php

use PhpParser\BuilderFactory;
use PhpParser\Node\Name;
use PhpParser\PrettyPrinter\Standard;

enum HttpMethod: string
{
    case GET = 'GET';
}

#[Attribute(Attribute::TARGET_METHOD)]
readonly class HTTP
{
    public HttpMethod $httpMethod;

    public function __construct(
        HttpMethod|string $httpMethod,
        public string $path
    )
    {
        if (is_string($httpMethod)) {
            $httpMethod = HttpMethod::from($httpMethod);
        }
        $this->httpMethod = $httpMethod;
    }
}

interface Api
{
    #[HTTP(httpMethod: HttpMethod::GET, path: '/users')]
    public function getUsers();
}

$reflectionClass = new ReflectionClass(Api::class);

$builderFactory = new BuilderFactory();

$class = $builderFactory->class('ApiImpl')
    ->implement('\\' . Api::class);

$reflectionMethod = $reflectionClass->getMethod('getUsers');

$method = $builderFactory
    ->method($reflectionMethod->getName())
    ->makePublic();

$reflectionAttributes = $reflectionMethod->getAttributes();
foreach ($reflectionAttributes as $reflectionAttribute) {
    $name = new Name($reflectionAttribute->getName());
    $attribute = $builderFactory->attribute($name, $reflectionAttribute->getArguments());

    $method->addAttribute($attribute);
}

$class->addStmt($method->getNode());

$proxyNamespaceBuilder = $builderFactory
    ->namespace('Namespace')
    ->addStmt($class);

$standard = new Standard();
echo $standard->prettyPrint([$proxyNamespaceBuilder->getNode()]);

When I try to create a proxy class with method which takes enum as a parameters I receive an exception:

Fatal error: Uncaught LogicException: Invalid value in /opt/project/vendor/nikic/php-parser/lib/PhpParser/BuilderHelpers.php on line 280

Call Stack:
    0.0004     401096   1. {main}() /opt/scratches/scratch_37.php:0
    0.0052    1342792   2. PhpParser\BuilderFactory->attribute($name = class PhpParser\Node\Name { protected $attributes = []; public $parts = [0 => 'HTTP'] }, $args = ['httpMethod' => enum HttpMethod::GET('GET'), 'path' => '/users']) /opt/scratches/scratch_37.php:54
    0.0052    1346616   3. PhpParser\BuilderFactory->args($args = ['httpMethod' => enum HttpMethod::GET('GET'), 'path' => '/users']) /opt/project/vendor/nikic/php-parser/lib/PhpParser/BuilderFactory.php:26
    0.0053    1351336   4. PhpParser\BuilderHelpers::normalizeValue($value = enum HttpMethod::GET('GET')) /opt/project/vendor/nikic/php-parser/lib/PhpParser/BuilderFactory.php:252

When I added a following handling in BuilderHelpers::normalizeValue() code works OK.

public static function normalizeValue($value) : Expr {
    if ($value instanceof Node\Expr) {
        return $value;
    }

    if (is_null($value)) {
        return new Expr\ConstFetch(
            new Name('null')
        );
    }

    if (is_bool($value)) {
        return new Expr\ConstFetch(
            new Name($value ? 'true' : 'false')
        );
    }

    if (is_int($value)) {
        return new Scalar\LNumber($value);
    }

    if (is_float($value)) {
        return new Scalar\DNumber($value);
    }

    if (is_string($value)) {
        return new Scalar\String_($value);
    }

    if (is_array($value)) {
        $items = [];
        $lastKey = -1;
        foreach ($value as $itemKey => $itemValue) {
            // for consecutive, numeric keys don't generate keys
            if (null !== $lastKey && ++$lastKey === $itemKey) {
                $items[] = new Expr\ArrayItem(
                    self::normalizeValue($itemValue)
                );
            } else {
                $lastKey = null;
                $items[] = new Expr\ArrayItem(
                    self::normalizeValue($itemValue),
                    self::normalizeValue($itemKey)
                );
            }
        }

        return new Expr\Array_($items);
    }

    // !!! HERE !!!
    if (is_object($value)) {
        return new Expr\ClassConstFetch(new Name($value::class), $value->name);
    }

    throw new \LogicException('Invalid value');
}

What do you think about this fix? It's ok? Or it should be handled in the another way?

@malyMiso
Copy link

malyMiso commented Aug 14, 2023

Hi @piotrooo ,
I am not sure if it is related or not (in case of "not related", please let me know and I'll report a new issue). When I use enum as default value in overridden method I end up with the same error.

enum PriorityEnum: int
{
    case HIGH = 5;
    case NORMAL = 1;
}

class BaseProduct
{
    public function create(ProductData $productData): Product
    {}
}

class Product extend BaseProduct: int
{
    public function create(
        ProductData $productData,
        PriorityEnum = PriorityEnum::HIGH
    ): Product
    {}
}

Your fix works for me

@ruudk ruudk linked a pull request Aug 15, 2023 that will close this issue
ruudk added a commit to ruudk/PHP-Parser that referenced this issue Aug 15, 2023
@ruudk
Copy link
Contributor

ruudk commented Aug 15, 2023

@piotrooo Great that you found a fix. But why didn't you create the PR with it? Then it could have been polished and merged.

Now it's still not solved.

Here is the PR: #940

ruudk added a commit to ruudk/PHP-Parser that referenced this issue Aug 15, 2023
ruudk added a commit to ruudk/PHP-Parser that referenced this issue Aug 15, 2023
ruudk added a commit to ruudk/PHP-Parser that referenced this issue Aug 15, 2023
ruudk added a commit to ruudk/PHP-Parser that referenced this issue Aug 15, 2023
@piotrooo
Copy link
Author

@piotrooo Great that you found a fix. But why didn't you create the PR with it? Then it could have been polished and merged.

Now it's still not solved.

Here is the PR: #940

That was only a idea how to solve it. I wasn't 100% sure that solution is correct. Sorry about that.

Your solution is more elegant.

@piotrooo
Copy link
Author

Hi @piotrooo , I am not sure if it is related or not (in case of "not related", please let me know and I'll report a new issue). When I use enum as default value in overridden method I end up with the same error.

I'm not quite sure what you want to do so I cannot confirm your error is related. But if you receiving a similar exception - that could be the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants