Symfony 7 support continuation

[antiftw]

Continuation of this PR.

• I've just used the composer diff from the other PR. However, I am not sure if that's actually correct, since I doubt this repo still supports versions that way back, but I could be wrong (?)
• Deprecated the Annotations (with attributes ;)).
• Created Attributes to replace them.
• Mostly went fine, however there is some code where you cannot really replace the annotations with attributes. Because I was not sure what to do with these, I just left them there, since they seemed to be effecting the 'used' status of the use statement for that particular attribute.
• Added some property typing.
• Some other minor refactors (deprecation warnings, other warnings from PHPStorm).
• Modified some Models to use constructor property promotion.
• Modified Translate attribute so that you dont have to provide a default domain.

[bocharsky-bw]

Thank you for starting working on it!

I allowed the CI to run here so that you could see some errors, but I suppose you should see those errors locally too?

[bocharsky-bw]

But I also think we should drop support for Symfony 3.4 and 4.4 too. Those are officially unmaintained anyway, so it's time to drop it. And I think we can bump 5 & 6 to 5.4 & 6.4 - I hope it will help us to make tests green easier

So, the ideal Symfony support version constraints should be: ^5.4 || ^6.4 || ^7.0 here and in other places.

Wdyt?

[antiftw]

Yes, I also thought something (see remark in PR message) similar, but I thought I'd await your opinion about this since you have more in depth knowledge of the repository.

I'll update the PR.

UPDATE: Done.

[antiftw]

Ok, I managed to fix some of the tests, however the other ones I'm not really sure what to do with. For the the remaining errors and failures it's related to my remark in the PR message. I'll provide an example:

In some of the code, the used annotations are in the middle of variables

        $builder
            ->add('foobar', TextType::class, array(
                /** @Desc("Foobar:") */
                'label' => 'test_label',
            ));

But if you try to put an attribute there it will give syntax errors. Furthermore, attributes only seem to be allowed in front of class/function definitions and class properties.

I'm not aware of any workarounds for this, so I'm not really sure how one would want to support this kind of functionality using attributes. Any of you have an idea?

EDIT:

I noticed the PHP STAN and CSFixer tests were failing because of an older PHP version, so I added ^7.3 ("back") to composer.

[bocharsky-bw]

I see, that's a bummer :/

I'm not sure. Probably we can use class properties where we will use the attribute and then reuse the property in that code? I'm not sure if this will stop working because of this though.

Or I wonder if we can continue using PHP annotations in this specific spot maybe?

[antiftw]

I agree, that felt like an obvious thing to try, but sadly that didn't work. About using 2 separate readers, I'm guessing that's probably possible.

Also did not think about the fact that adding 7.3 back does not allow typing of class properties, so I'm not sure what you would prefer? Want me to remove the typing or bump it to 8.*?

[bocharsky-bw]

Thank you! ❤️

[antiftw]

I think I have some bad news. I think I found what you were talking about.

I was reverting the changes, and when I came across this file tests/Functional/Visitor/Php/Symfony/ValidationAnnotationTest.php I was unable to change it back to AnnotationLoader without PHPstorm complaining.

Some furthing digging dug up:

Reading annotations in PHP files is deprecated since Symfony 6.4. Also, the AnnotationLoader was deprecated in Symfony 6.4.

soooo.... what now? :)

EDIT:

Ok, so I've just put back the AttributeLoader again and for those cases I am able to use the attributes (It's not actually this packages custom attributes, but rather a translation inside a validator attribute):

<?php

namespace Translation\Extractor\Tests\Resources\Php\Symfony;

use Symfony\Component\Validator\Constraints as Assert;
use Symfony\Component\Validator\Constraints\NotBlank;

class ValidatorAnnotation
{

    #[Assert\NotNull(message: "start.null")]
    private string $start;

    #[NotBlank(message: "end.blank")]
    private string $end;
}

The final error it's failing on now is \Translation\Extractor\Tests\Functional\Visitor\Php\Symfony\ValidationAnnotationTest::testExtractAnnotationError

Which seems weird, because you would expect this to error, but for some reason it does not:

<?php

namespace Translation\Extractor\Tests\Resources\Php\Symfony;

class ValidatorAnnotationError
{
    #[Foobar('this should be an error')]
    private string $foo;
}

(I just replaced the annotation with its attribute counterpart)

Maybe rewrite the test that it does not check for errors but just to check if it did not find any valid attributes? When I dump the collection variable inside the test it just has two empty arrays:

object(Translation\Extractor\Model\SourceCollection)#43047 (2) {
   ["sourceLocations":"Translation\Extractor\Model\SourceCollection":private]=>
      array(0) {
   }
   ["errors":"Translation\Extractor\Model\SourceCollection":private]=>
      array(0) {
   }
}

[bocharsky-bw]

Ah, heck... that's exactly the problem I faced, now I remember it :/

So, I'm not sure about the best solution here, I'm open to ideas

[antiftw]

:)

Perhaps a way to test this is to change the test from

 public function testExtractAnnotationError()
    {
        $factory = new LazyLoadingMetadataFactory(new AttributeLoader());
        $extractor = new ValidationAnnotation($factory);
        $collection = $this->getSourceLocations($extractor, Resources\Php\Symfony\ValidatorAnnotationError::class);

        $errors = $collection->getErrors();
        $this->assertCount(1, $errors);
    }

to

 public function testExtractAnnotationError()
    {
        $factory = new LazyLoadingMetadataFactory(new AttributeLoader());
        $extractor = new ValidationAnnotation($factory);
        $collection = $this->getSourceLocations($extractor, Resources\Php\Symfony\ValidatorAnnotationError::class);

        $this->assertCount(0, $collection);
    }

Since apparently the AttributeReader does not give in errors when putting in non-existing attributes to extract. It rather just results in no attributes.

I'd like to hear what you think about this change. I've committed and pushed it so you can look over the changes without me copy pasting just snippets. To be clear, this commit passes the local vendor/bin/simple-phpunit testsuite.

[antiftw]

Ok so I've had some pressing other issues to take care of but this evening I had some spare time to look into this PR.

I figured out how to run the GitHub actions locally so I was able to fix most of the warnings and errors that the statistical tests threw. However I've got one left that I don't really understand, and I was hoping it makes more sense to you:

[Static analysis/PHPStan     ]   ❗  ::error file=src/Visitor/Php/Symfony/AbstractFormType.php,line=,col=0::Ignored error pattern #^Access to an undefined property PhpParser\\Node\:\:\$args\.$# in path /home/antiftw/repos/forks/php-translation-extractor/src/Visitor/Php/Symfony/AbstractFormType.php was not matched in reported errors.

I was able to fix a similar error by adding a null check in the code. I tried the same here, but no luck. However a notable difference is that the other error did indicate a line number where it occurred, and this one does not. Also this one includes a part ("was not matched in reported errors") that the other did not.

EDIT:

Minor change and it seems to be passing the last test too now.

EDIT²: The package I'm using it in required a bump of a dependency of this package (nikic/php-parser) so that it could also use 5.0.0. This caused some issues but they are fixed too. All tests pass locally, so fingers crossed and it should work now.

[antiftw]

Thank you for starting working on it!

I allowed the CI to run here so that you could see some errors, but I suppose you should see those errors locally too?

You're welcome, and glad to see such a quick response :)

Thanks, that was one of the reasons why I submitted the PR, since I actually had some trouble getting the test working yesterday evening. But I got them to work now. First going to cook some food and then I'm going to dive right in.

[bocharsky-bw]

I see tests are green - this is awesome! Thank you for your work on it

I have only 1 question to clarify so far

[bocharsky-bw]

This looks like we just supress the test, i.e. the errors never tested?

[antiftw]

Well thats actually the point we discussed here.

The Symfony\Component\Validator\Mapping\Loader\AttributeLoader; apparently does not give any errors when it encounters an an attribute it does not recognize, while apparently the Symfony\Component\Validator\Mapping\Loader\AnnotationLoader did when it encountered an unknown annotation.

[bocharsky-bw]

Ah I remember, thanks!

yeah, probably we don't need that test at all then, IMO it has no value anymore

[antiftw]

I agree indeed, does not really add much. I can remove it, but I am not able to do so at this moment, so that would have to wait until later this day.

[antiftw]

Pushed a commit where the test and case are removed.

[bocharsky-bw]

Thank you for your active work on this!

It looks ready to me and I have no more comments. If others agree too - we can merge this and deliver this long-waiting Sf7 support finally

[antiftw]

Nice! And happy to help :)

[bocharsky-bw]

OK, tests pass, so I'm releasing it! Thanks again for this big work 👍