Conditionally stop extending from the SensioFrameworkExtraBundle's Template annotation class

[mbabker]

Ref: #2354

With SensioFrameworkExtraBundle being deprecated and not compatible with Symfony 7, one of the things that has to be fixed to achieve Symfony 7 compatibility is breaking the reliance on that bundle for this bundle's @View annotation/attribute. That is accomplished with this PR by breaking the class inheritance and rewiring the event subscriber processing the annotation to work without the features from the SensioFrameworkExtraBundle.

This includes a hard B/C break by removing the class inheritance, the @View annotation class would no longer pass a class_exists(Sensio\Bundle\FrameworkExtraBundle\Configuration\Template::class); check as a result of this change. That's honestly a very low-risk change for downstream users, but has to be pointed out anyway. Maybe a bit higher risk is the fact that not all of the methods from the parent class are provided (as most of them are (rightfully) oriented toward rendering a template, which the event subscriber doesn't support).

As suggested later in this thread, the B/C break is mitigated by using conditional inheritance, so the @View annotation will continue extending Sensio\Bundle\FrameworkExtraBundle\Configuration\Template when the class is present and the hard break can be delayed to 4.0.

[W0rma]

Thank you for working on this!

I just applied the changes to my own project which doesn't use any View annotation/attribute - everything does still work like expected.

[W0rma]

This includes a hard B/C break by removing the class inheritance, the @view annotation class would no longer pass a class_exists(Sensio\Bundle\FrameworkExtraBundle\Configuration\Template::class); check as a result of this change. That's honestly a very low-risk change for downstream users, but has to be pointed out anyway. Maybe a bit higher risk is the fact that not all of the methods from the parent class are provided (as most of them are (rightfully) oriented toward rendering a template, which the event subscriber doesn't support).

@goetas What's your opinion? Do you consider this a "soft" BC break or must this PR be part of the next major release?

[andersonpem]

This includes a hard B/C break by removing the class inheritance, the @view annotation class would no longer pass a class_exists(Sensio\Bundle\FrameworkExtraBundle\Configuration\Template::class); check as a result of this change. That's honestly a very low-risk change for downstream users, but has to be pointed out anyway. Maybe a bit higher risk is the fact that not all of the methods from the parent class are provided (as most of them are (rightfully) oriented toward rendering a template, which the event subscriber doesn't support).

@goetas What's your opinion? Do you consider this a "soft" BC break or must this PR be part of the next major release?

Hi guys. I'm not developing this, but I'm on the lookout. And looking forward to using this bundle in Symfony v7 :)
In my humble opinion, as a senior, considering it's a potentially API breaking change, it would be wise to bump it to a major release. 😄

[xabbuh]

You could declare the class conditionally based on whether or not the Template class from SensioFrameworkBundle is present.

[mbabker]

You could declare the class conditionally based on whether or not the Template class from SensioFrameworkBundle is present.

Yeah, that looks to work. PR updated.

(The failing CI can be dealt with when we've got a plan of action because the failures are all related to the PHPUnit bridge and the deprecation reporting, including Symfony 6.4 added deprecations)

[alexander-schranz]

We could disable the Deprecation helper via:

env:
    SYMFONY_DEPRECATIONS_HELPER: weak

or

env:
    SYMFONY_DEPRECATIONS_HELPER: disabled

[mbabker]

We could disable the Deprecation helper via:

I think I'm just gonna leave it be for the moment. I added the max[self]=104 config in the phpunit.xml.dist but that didn't really help anything here, and forcing a new build on #2400 shows on the PHP 8.2 + Symfony 6.4 a much lower deprecation count (without the failing build), so I think just letting this merge with a failing CI and fixing it over on the other PR immediately after is probably a better idea as we've got a much better baseline to work from (given that branch actually works on full Symfony 6.4 + 7.0 compat and addressing the appropriate deprecations, while this one's just focused on the feature compat issue).

[alexander-schranz]

I think for PHP 8.2 it would be required to be max[self]=104&max[indirect]=243. This could also be defined via a test matrix env:

    strategy:
      matrix:
        include:
          - php-version: 8.2
            composer-flags: ""
            can-fail: true # we don't want to fail the build if we are incompatible with the next (unstable) Symfony version
            env:
              SYMFONY_DEPRECATIONS_HELPER: 'max[self]=104&max[indirect]=243'

      - name: "Run PHPUnit"
        if: "${{ matrix.coverage != '' }}"
        run: |
          XDEBUG_MODE=coverage ./phpunit --coverage-clover=coverage.clover
          wget https://scrutinizer-ci.com/ocular.phar
          php ocular.phar code-coverage:upload --format=php-clover coverage.clover
        env: ${{ matrix.env }}

      - name: "Run PHPUnit"
        if: "${{ matrix.coverage == '' }}"
        run: "./phpunit"
        env: ${{ matrix.env }}

[mbabker]

I wasn't even thinking about the indirect 🤦

phpunit.xml.dist updated, we can clean that up better later as well.

[alexander-schranz]

That looks great. Maybe @goetas can give it a review now :)

[shakaran]

Thanks for the great work done here. This is the only dependency that I have pending in several projects to update finally to 7.0. All seems in place, any ETA to merge this soon? I would love a new release or at least a official dev branch to pick it. I am sure that when this gets released a lot projects will bump to SF 7.0

[goetas]

@mbabker thanks for the great work as usual!

(and @alexander-schranz thanks for pinging me on this)

[flohw]

Hi @mbabker,

I just updated to the latest release and get some issues as "there are no registered paths for namespace FOSRest" (from twig) which is fixable via twig.path.%kernel.project_dir%/templates: FOSRest but requires creating template for each route. (suggested in #1844)

The searched for other issue I had: it seems that the serializer was not invoked or something involving twig as the error was something like an array is expected in argument #1 of render, FQCN given. I tried to remove sensio/framework-extra-bundle which was still installed and everything is back to normal. (including not setting twig.path/%kernel.project_dir%/templates configuration)
I didn't investigate further as I was relying on sensio extra bundle only for the view annotation and migrate all the other parts.

Not sure if it was the intended solution though. 🤔
Hopefully this could be a solution for everyone. 🙂

Thank you again for the work.

[mbabker]

Admittedly I've never used either this bundle's #[View] attribute, the Sensio bundle's #[Template] attribute, or the listener that's supposed to handle converting views returned from controllers into responses, so I honestly have no idea how Twig is getting invoked here or why it's looking for templates in the Twig @FOSRest namespace (especially with the bundle having none to begin with). So if you or someone has a more complete reproduction scenario that we can look at, that'd probably be the best way to find a way to permanently sort things out.

(The one thing I can think of that might affect it is that I did change the event listener priority order with this PR, so maybe the comment that used to be there about needing to run before the Sensio bundle's template listener still applies and the fix is to bump back up the priority? And if that's the case, it'd be good to have a functional test covering that so it doesn't break again as long as there's an explicit compat layer with that bundle in place.)

[benconda]

Hello, I just do the update and got the same issue :
TypeError: Twig\Environment::render(): Argument #2 ($context) must be of type array, My\FQCN\Response given

Looks like the _template is set on the request attributes (with a weird .json.twig file) where it should not (the route return an Object and should not rely on template)?

[benconda]

I think it's because of this change : https://github.com/FriendsOfSymfony/FOSRestBundle/pull/2401/files#diff-2359bb4681ba7bf807bf836fd67da56e5983f70efdc2bcca29768026d4c2352cR173
The listener is now called after SensioFrameworkExtraBundle one : The higher the number, the earlier the method is called

[flohw]

Yes, that's exactly what I was talking about but with more details. 😊

I will try to dig a bit more next week.