sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Twig Deprecated: Using an "if" condition on "for" tag #5575

Closed Hanmac closed 5 years ago

Hanmac commented 5 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.49.1             = 3.49.1             The missing Symfony Admin Generator
sonata-project/block-bundle              3.15.0             = 3.15.0             Symfony SonataBlockBundle
sonata-project/cache                     2.0.1              = 2.0.1              Cache library
sonata-project/core-bundle               3.17.0             = 3.17.0             Symfony SonataCoreBundle
sonata-project/datagrid-bundle           2.5.0              ~ 3.0.0              Symfony SonataDatagridBundle
sonata-project/doctrine-extensions       1.3.0              = 1.3.0              Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.9.0              = 3.9.0              Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/easy-extends-bundle       2.5.0              = 2.5.0              Symfony SonataEasyExtendsBundle
sonata-project/exporter                  2.0.1              = 2.0.1              Lightweight Exporter library
sonata-project/user-bundle               dev-master af1b208 = dev-master af1b208 Symfony SonataUserBundle

Symfony packages

$ composer show --latest 'symfony/*'
symfony/acl-bundle         dev-master 5b32179 = dev-master 5b32179 Symfony AclBundle
symfony/assetic-bundle     v2.8.2             = v2.8.2             Integrates Assetic into Symfony2
symfony/monolog-bundle     v3.3.1             = v3.3.1             Symfony MonologBundle
symfony/phpunit-bridge     v2.8.50            ~ v4.3.0             Symfony PHPUnit Bridge
symfony/polyfill-apcu      v1.11.0            = v1.11.0            Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-ctype     v1.11.0            = v1.11.0            Symfony polyfill for ctype functions
symfony/polyfill-intl-icu  v1.11.0            = v1.11.0            Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring  v1.11.0            = v1.11.0            Symfony polyfill for the Mbstring extension
symfony/polyfill-php56     v1.11.0            = v1.11.0            Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70     v1.11.0            = v1.11.0            Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-util      v1.11.0            = v1.11.0            Symfony utilities for portability of PHP codes
symfony/security-acl       v3.0.2             = v3.0.2             Symfony Security Component - ACL (Access Control List)
symfony/swiftmailer-bundle v2.6.7             ~ v3.2.7             Symfony SwiftmailerBundle
symfony/symfony            v3.4.28            ~ v4.3.0             The Symfony PHP framework

PHP version

$ php -v
PHP 7.3.5 (cli) (built: May  1 2019 13:17:17) ( ZTS MSVC15 (Visual C++ 2017) x64 )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.5, Copyright (c) 1998-2018 Zend Technologies
    with Xdebug v2.7.2, Copyright (c) 2002-2019, by Derick Rethans

Subject

Twig 2.10 does deprecate "if" condition on "for" tag https://twig.symfony.com/doc/2.x/tags/for.html#adding-a-condition

Steps to reproduce

Open Sonata admin area and check the deprecated warnings

Expected results

No Warnings

Actual results

User Deprecated: Using an "if" condition on "for" tag in "@SonataAdmin/Block/block_admin_list.html.twig" at line 17 is deprecated since Twig 2.10.0, use a "filter" filter or an "if" condition inside the "for" body instead (if your condition depends on a variable updated inside the loop).

For example this one:

{% set groups = [] %}

    {% for group in sonata_admin.adminPool.dashboardgroups %}
        {% set display_group = false %}

        {% for admin in group.items if display_group == false %}
            {% if admin.hasRoute('create') and admin.hasAccess('create') %}
                {% set display_group = true %}
                {% set groups = [group]|merge(groups) %}
            {% endif %}
        {% endfor %}
{% endfor %}

what would be the correct thing if the "if" condition on "for" tag can't be used anymore?

phansys commented 5 years ago

I confirm this deprecation, it is the cause for #5573.

Hanmac commented 5 years ago

@phansys but that wouldn't fix the problem because with newer twig the deprecation would still be there?

phansys commented 5 years ago

That's right. Seems that I've overlooked your point, sorry. The correct way to achieve the same result is using the filter filter where possible, or moving the if condition inside the loop. See https://twig.symfony.com/doc/2.x/deprecated.html#tags:

Adding an if condition on a for tag is deprecated in Twig 2.10. Use a filter filter or an "if" condition inside the "for" body instead (if your condition depends on a variable updated inside the loop)

I'll update the RFC in order to cover the required bump for 3.x version to ^1.41. Do you plan to create a PR fixing this?

Hanmac commented 5 years ago

i don't know how it would need to be done yet even when using filter, i don't know if we need 1. or if we should already jump to the 2. one for the other MR

my pseudo code is this:

for group in sonata_admin.adminPool.dashboardgroups check if ANY admin in group, hasRoute and hasAccess to create if yes merge group into groups

i think it may be done with filters or with multiple filters, but in general you just need some "any?" method/function that evaluates until it has found an item where the expression returns trueish

phansys commented 5 years ago

Given your first snippet, filter filter can not be used since the condition is based on a variable (display_group) updated inside the loop.

Hanmac commented 5 years ago

yeah thats what i thought there, but display group is only used as flag so the check isn't called more than once because it isn't needed. (twig has no break)

thats why it would need an any filter twig doesn't has yet

my example if any would exist:

{% set groups = [] %}
{% for group in sonata_admin.adminPool.dashboardgroups %}
    {% if group.items | any( admin => admin.hasRoute('create') and admin.hasAccess('create')) %}
    {% set groups = [group]|merge(groups) %}
    {% endif %}
{% endfor %}

it might even work as one-liner

{% set groups = sonata_admin.adminPool.dashboardgroups | filter(group =>group.items | any(admin => admin.hasRoute('create') and admin.hasAccess('create')))  %}
phansys commented 5 years ago

It will require a custom filter. Since this deprecation, Twig doesn't provide an alternative solution to avoid the iteration over a condition which is already reached inside the loop. Maybe you could create an RFC for such filter.

Hanmac commented 5 years ago

@phansys already did make an request for a Filter/Test function for any

https://github.com/twigphp/Twig-extensions/issues/245

Hanmac commented 5 years ago

@OskarStark Closed by #5576 ?

phansys commented 5 years ago

Yes, I think @Hanmac. Thank you!