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

[Step 6 Upgrade] Add PHP 8+ Attributes support #130

Merged
merged 53 commits into from Feb 4, 2024

Conversation

samsonasik
Copy link
Contributor

@lisachenko here the 6th step 👍 , to add attributes support.

@lisachenko
Copy link
Member

Hello, @samsonasik!

Do you plan to add actual ReflectionAttribute implementation later, so ReflectionMethod->getAttributes(), ReflectionClass->getAttributes() and similar from AST

@samsonasik
Copy link
Contributor Author

Hi @lisachenko , yes, I will try it in this PR

@goaop goaop deleted a comment from Nitpick-CI Feb 2, 2024
@goaop goaop deleted a comment from Nitpick-CI Feb 2, 2024
@goaop goaop deleted a comment from Nitpick-CI Feb 2, 2024
@goaop goaop deleted a comment from Nitpick-CI Feb 2, 2024
@goaop goaop deleted a comment from Nitpick-CI Feb 2, 2024
@goaop goaop deleted a comment from Nitpick-CI Feb 2, 2024
@samsonasik
Copy link
Contributor Author

@lisachenko I added failing test case with stub with throw RuntimeException inside it, do you mean unit test should not show error even it has RuntimeException inside stub 031c9db :

There was 1 error:

1) Go\ParserReflection\Locator\ComposerLocatorTest::testLocateClassWithAttributes
RuntimeException: test

If I remove the $this->__initialize() usage, it got error:

There was 1 error:

1) Go\ParserReflection\Locator\ComposerLocatorTest::testLocateClassWithAttributes
Error: Internal error: Failed to retrieve the reflection object

/Users/samsonasik/www/parser-reflection/src/Traits/AttributeResolverTrait.php:28

I tried cheking from the node from its reflector and got segmentation fault:

Runtime:       PHP 8.3.0
Configuration: /Users/samsonasik/www/parser-reflection/phpunit.xml.dist

.[1]    7644 segmentation fault  vendor/bin/phpunit tests/Locator/ComposerLocatorTest.php
    public function getAttributes(?string $name = null, int $flags = 0): array
    {
        $node = $this->getNode();
        $attributes = [];

        foreach ($node->attrGroups as $attrGroup) {
            foreach ($attrGroup->attrs as $attr) {
                if ($name === null) {
                    $attributes[] = new ReflectionAttribute($attr->name->toString(), $this);

                    continue;
                }

                if ($name !== $attr->name->toString()) {
                    continue;
                }

                $attributes[] = new ReflectionAttribute($name, $this);
            }
        }

        return $attributes;
    }

@samsonasik
Copy link
Contributor Author

@lisachenko I added handling get Attributes by AST 9a2bb99 , not sure yet on getTarget() tho, as it not documented in php manual https://www.php.net/manual/en/reflectionattribute.gettarget.php

@samsonasik
Copy link
Contributor Author

@lisachenko I updated to get Line via Node which go to end of attrGroups line and add + 1 (mostly next line of attrGroups), and throw the RuntimeException on getTarget() and newInstance() as it require the real ReflectionAttribute b567473

@samsonasik
Copy link
Contributor Author

implemented 🎉 @lisachenko Ready to merge 👍

@lisachenko
Copy link
Member

lisachenko commented Feb 4, 2024

Wow! This was fast! ))

I've tested it for the class and method - it works!! 🚀

There is only gotcha with properties (would be nice to add a test for this) - for ReflectionProperty attributes are stored not inside PropertyProperty::$attrGroups, but inside type node (instance of Property AST node, which might be accessed via getTypeNode() getter.

PHP Deprecated:  Return type of Go\ParserReflection\ReflectionProperty::getValue($object = null) should either be compatible with ReflectionProperty::getValue(?object $object = null): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in ./parser-reflection/src/ReflectionProperty.php on line 196
PHP Warning:  Undefined property: PhpParser\Node\Stmt\PropertyProperty::$attrGroups in ./parser-reflection/src/Traits/AttributeResolverTrait.php on line 29
PHP Warning:  foreach() argument must be of type array|object, null given in ./parser-reflection/src/Traits/AttributeResolverTrait.php on line 29

For ReflectionParameter, attribute also generates this deprecation notice:
PHP Deprecated: Return type of Go\ParserReflection\ReflectionParameter::getDefaultValue() should either be compatible with ReflectionParameter::getDefaultValue(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in ./parser-reflection/src/ReflectionParameter.php on line 265

@lisachenko
Copy link
Member

@lisachenko I added handling get Attributes by AST 9a2bb99 , not sure yet on getTarget() tho, as it not documented in php manual https://www.php.net/manual/en/reflectionattribute.gettarget.php

We can skip this function implementation, or just delegate to the parent call. I guess, it should load the attribute class into memory, then check the bitmask for which type of items this attribute should be applied. We don't need this functionality now.

@samsonasik
Copy link
Contributor Author

@lisachenko sure, resolved at eefee84 on getting attrGroups fromProperty 👍 , it should be ready now 👍

@samsonasik
Copy link
Contributor Author

I also updated ReflectionParameter::getDefaultValue() return type 799e69d

@lisachenko
Copy link
Member

@samsonasik could you please check X/Twitter private messages BTW? ) Need some info

@samsonasik
Copy link
Contributor Author

@lisachenko yes, replied 👍

@samsonasik
Copy link
Contributor Author

implemented 🎉 @lisachenko Ready to merge 👍

@lisachenko lisachenko merged commit bf823ac into goaop:master Feb 4, 2024
2 checks passed
@lisachenko
Copy link
Member

Merged, thanks!

@samsonasik samsonasik deleted the step6-attributes branch February 4, 2024 11:31
@scrutinizer-notifier
Copy link

The inspection completed: No new issues

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

Successfully merging this pull request may close these issues.

None yet

4 participants