Annotations refactoring & PHP 8 Attributes

[Vincz]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Documented?	partly
Fixed tickets	#707

[murtukov]

I thought we are gonna discuss it in January first

[Vincz]

I thought we are gonna discuss it in January first

@murtukov We will. I had a bit of free time and I just wanted to implement the PHP 8 attributes and try to clean the annotations code base (also based on #707). But everything is still open to discussion.

[Vincz]

@mcg-web Hey man!
I have a couple of issues with php stan as you can see here : https://github.com/overblog/GraphQLBundle/pull/801/checks?check_run_id=1637669155

I have a bunch of missing type hints but I'm not sure how to handle them when a parameter can accept multiple types (string or array for example).
Also, there is a bunch of issue related to PHP 8 (PHP stan does not know some classes or methods specifics to PHP8). Not sure how to handle this as well.
Could you help me with this?

[Vincz]

@mcg-web Another problem is with the test coverage. As the PHP 8 attribute parser tests only run with PHP8, the coverage drops when used with PHP 7.

[mcg-web]

Hey @Vincz !

I have a bunch of missing type hints but I'm not sure how to handle them when a parameter can accept multiple types (string or array for example).

you should use phpdoc right now since union is available only for php >= 8.0

/**
* @param string|array $myArg
*/
function ($myArg) {}

Another problem is with the test coverage. As the PHP 8 attribute parser tests only run with PHP8, the coverage drops when used with PHP 7.

we can try using php 8.0 for coverage tests

[Vincz]

Ok guys, this PR is almost ready.
I still need to update the documentation.
Trying to make a chinese panda happy, all features from #707 have been implemented (except the 'get' removal from queries).

• The nested annotations have been deprecated in favor of a flat config.
• We now have a type guesser based on doc block @var and @return
• PHP8 attributes are supported.
• We have shortcuts on annotations (ex: @Field('myfield')) - I'm not satisfied with the implementation but I'm waiting for this PR Introduced annotation NamedArgumentConstructor doctrine/annotations#391 to be merged, so I can submit another one to handle default property on annotation, so we can use more explicit configuration property instead of value.

We would need to be able to run tests in a PHP8 environment to support attributes (at the moment tests related to attributes are skipped if not in PHP8).
Some config keys have been renamed for builders so it's a BC break (is that ok if we targeting v1.0 with this new features or should I provider Backward compatibility).

Let me know what you think!

[Vincz]

Hey @murtukov! I'm trying to run the test on Github with PHP 8 (see my latest commit).
It failed because of the PHP Generator library. Do you know if it is compatible with PHP 8 and if so, would you mind to update the composer to allow it?
Thanks man!

[murtukov]

@Vincz just released a new version of PHPCodeGenerator. Change the composer requirement to 0.1.5

[Vincz]

@Vincz just released a new version of PHPCodeGenerator. Change the composer requirement to 0.1.5

Thank you!

[Vincz]

@murtukov It break the PHP 7 tests, because:

➜  ~ $ echo '<?php new flu(); ?>' | php
PHP Fatal error:  Uncaught Error: Class 'flu' not found in Standard input code:1

but...

➜  ~ $ echo '<?php new flu(); ?>' | php8
PHP Fatal error:  Uncaught Error: Class "flu" not found in Standard input code:1

[murtukov]

@Vincz well if both approaches use almost the 100% same codebase, then let it be a single bundle 🙂

One thing I don't understand is why removing shorthands? As I know the Symfony's Router uses both annotations and attributes with a single class and it's still possible to use shorthands. Why can't we do the same?

Here is the implementation: https://github.com/symfony/routing/blob/5.x/Annotation/Route.php

[Vincz]

@murtukov Yes, at the end it was exactly the same: Metadata stored on a PHP classes, but the exploitation of this metadata is the same. So there is just a very few differences than we can handle in the parser itself.

I removed the shorthands for now, cause I didn't like the implementation. The current route annotation use the "old" way to setup the annotation. It takes the parameters from an array in the first argument of the constructor. So there is no argument validation by the annotation system. So, they introduce the NamedArgumentConstructor to ease the migration to attribute. But the Symfony team is also planning to use the "new" way (see doctrine/annotations#391).
I'll put them back as soon as we have the ability to set a default property other than value in annotations. It should not be a problem to add it later as there will be no BC break. I also wanted to rename a bunch of properties for more explicit names, like expression instead of value in @Access or @Public for example.
To handle it, I was adding a value property to the constructor to handle the default and use it's value as the value of my default property. But I had to raise an exception if both where used and I had to mark required properties as nullable and I didn't liked it.
But the shorthand works with attributes as long as the arguments order is correct. It's just a constructor call with named arguments.

[Vincz]

@murtukov About the tests, the coverage drops because the attributes tests are skipped.
What do you think we should do about it? Should we update the coverage environment to use PHP 8?

[Vincz]

Well... The tests with coverage in PHP8 is ok, but the coverage upload to scrutinizer fails.

[murtukov]

@Vincz seems like switching to PHP 8 environment is the only solution

[Vincz]

@mcg-web Hey man! Any idea what the coverage upload would fail? I don't want to mess with the ci tasks so I didn't try to fix it myself...

[murtukov]

@Vincz I don’t know, I never configured GitHub workflows. Try setting the PHP version to “8.0” instead of just “8”

[Vincz]

Ok, so the problem is ocular (the binary to upload the coverage) downloaded from the scrutinizer website is an old version (see scrutinizer-ci/ocular#53).
The solution seems to install it as a composer dependency.

- name: "Install Ocular as depencies"
   run: composer req "scrutinizer/ocular" --dev --no-update

I tried, it installs fine, but I'm unable execute it, no matter the syntax I use (the binary is stored in vendor/bin/ocular)
I tried the following:

run: php vendor/bin/ocular code-coverage:upload --format=php-clover build/logs/clover.xml

run: vendor/bin/ocular code-coverage:upload --format=php-clover build/logs/clover.xml

run: ./vendor/bin/ocular code-coverage:upload --format=php-clover build/logs/clover.xml

But none of them worked and the binary is not found. Seems like a context problem.
ping @mcg-web

[murtukov]

@Vincz the path is fixed and seems to work now. Just need to add/ignore some test coverages.

P.S. Could you register in Telegram for quick contacts? Slack is too heavy and imperfect for this.

[Vincz]

@murtukov You're the best! I'll check the missing tests.
I just registered to Telegram and sent you an email with my account details.