Regression in Form extension since 3.2

[romainneutron]

Since Symfony 3.2 RC 1, I see a regression in my web app: I have a form extension that disable csrf-protection on the /yolo/* routes:

use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\Extension\Core\Type\FormType;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\OptionsResolver\OptionsResolver;

class YoloCsrfDisablerExtension extends AbstractTypeExtension
{
    private $requestStack;

    public function __construct(RequestStack $requestStack)
    {
        $this->requestStack = $requestStack;
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $request = $this->requestStack->getCurrentRequest();

        if (null === $request) {
            return;
        }

        if (0 !== strpos($request->getPathInfo(), '/yolo/')) {
            return;
        }

        $resolver->setDefaults([
            'csrf_protection' => false,
        ]);
    }

    public function getExtendedType()
    {
        return FormType::class;
    }
}

Prior to Symfony 3.2, when I submitted a form that failed on validation, errors returned by the form submission returned something like: The value should not be blank.

Since Symfony 3.2, even if the form extension is enabled, I now got two errors: The value should not be blank and The CSRF token is invalid. Please try to resubmit the form. that should not appear since the option is disabled with my extension

[romainneutron]

Fixed using priority on the service definition:

        <service id="yolo_csrf" class="YoloCsrfDisablerExtension">
            <argument type="service" id="request_stack"/>
            <tag name="form.type_extension" priority="-5" extended-type="Symfony\Component\Form\Extension\Core\Type\FormType" />
        </service>

[stof]

The regression seems to indicate that the sorting does not preserve the order of extensions having the same priority. We should fix that to avoid BC breaks

[HeahDude]

I do not agree on this, if it worked before it was a kind of "luck" that we should avoid. That's why having priorities is good because if extensions depend on each other, then it should be done carefully.

Either by registering a form extension to add them in the order one needs, or thanks to the new feature of the full stack.

[francoispluchino]

@HeahDude I don't agree with you on:

if it worked before it was a kind of "luck"

Before adding the priority option, the form-extensions were loaded in the order defined by::

• the order of the bundles
• The order of the service definitions in the config file
• The order of dynamic additions from the framework extensions and the compiler passes

Adding the priority option is good news, But it is preferable to keep the order according to the addition for definitions with the same priority. We will avoid BC (see FriendsOfSymfony/FOSRestBundle#1602 for example).

[HeahDude]

@francoispluchino Hi, I understand your points.

Before adding the priority option, les form-extensions était chargés dans l'ordre definis par:

• the order of the bundles
• The order of the service definitions in the config file
• The order of dynamic additions from the framework extensions and the compiler passes

Yes, an order very fragile, easy to break without noticing, and really hard to maintain IMHO.

Now again I really think they should be 2 ways of being explicit about extensions "order" for sure:

• using a form.extension which has the responsibility to register some form types, form type extensions and guessers in the form registry.
• using priorities as it is done for many other tags in the full stack (and that's consistent).

This is the best to be in full control here, but ok, this is just my opinion.

What I think is that breaking it now like this, just reveals some inconsistencies in the code, and once fixed with one of the ways I've proposed, it is a safer thing for the future.

Extensions were not meant to "extend" each other in the first place, so we have to make some compromise.

[francoispluchino]

Make this change for a new major release (ex. 3.x to 4.0), justified with your arguments, ok, I understand, even though order very fragile is not a real argument, because you have the same problem if 2 bundles define the same priority and the same form type.

Even though the extensions were not intended to extend each other, this scenario often occurs with libraries, and the order of adding Bundles was not hazardous.

The problem is that changing this behavior was introduced in a new minor version (3.1 to 3.2), and does not match with the Symfony release rules for deprecations, namely, adding depreciations between minor versions, and accepting BCs between major versions.

This change should have been done for the version 4.0 and not the 3.2.

[HeahDude]

The problem is that we cannot deprecate this kind of behavior and we also maintain upgrade files for minor releases. So it would have broken the same in 4.0, without being able to get some notice before, just by reading the upgrade file :/

not a real argument, because you have the same problem if 2 bundles define the same priority and the same form type.

This is exactly what I'm talking about, it's just about being more explicit, easy to understand and to solve, because then how do you handle two bundles when one needs some service to be registered first and the other needs this one, this is not manageable by bundle ordering. Such use cases for services in different bundles that need priorities because they share some tag should be able to define a priority.

Maybe we (mostly I in this case, so sorry about that) need to be more careful in the future to prevent such BC break, but reverting it now might break applications now using priorities :(

So I don't know what to say now.

[nicolas-grekas]

Is this php7 only? Php7 uses non stable sorting algos, on the contrary to php5.
If this is a BC break, it has to be fixed. The fact that the fix might break newest apps is not a reason to accept the break.

[francoispluchino]

@nicolas-grekas No, at least since php5.3, see:

• https://bugs.php.net/bug.php?id=53710
• https://bugs.php.net/bug.php?id=60926

SplPriorityQueue is not a FIFO implementation. With the php7, the performance has improved, but php7 still did not fix the problem.

There are several solutions to maintain performance and achieve the expected behavior:

• http://stackoverflow.com/questions/25522897/why-splpriorityqueue-class-is-a-queue-conceptual
• https://medium.com/@rtheunissen/efficient-data-structures-for-php-7-9dda7af674cd#.q65419rnw
• https://github.com/php-ds
• https://mwop.net/blog/253-Taming-SplPriorityQueue.html

Alternatively, we can use the old method:

// Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait

private function findAndSortTaggedServices($tagName, ContainerBuilder $container)
{
    $services = array();

    foreach ($container->findTaggedServiceIds($tagName) as $serviceId => $tags) {
        foreach ($tags as $attributes) {
            $priority = isset($attributes['priority']) ? $attributes['priority'] : 0;
            $services[$priority][] = new Reference($serviceId);
        }
    }

    // sort by priority and flatten
    if (count($services) > 0) {
        krsort($services);
        $services = call_user_func_array('array_merge', $services);
    }

    return $services;
}

[HeahDude]

@francoispluchino see #20993 :)

[francoispluchino]

I just saw your PR. My previous example is not more simple?

[HeahDude]

I'm not sure about that, we have to do a first triage by extended type https://github.com/symfony/symfony/pull/20993/files#diff-a5475cdc6eb4006880d92f461fba8034R63.

[francoispluchino]

I don't think it's necessary, because the order is preserved. So each extended-type will have a correct priority order.

After, regarding the performance, I did not check, but I guess that 1 foreach(), 1 krsort() et 1 array_merge() is faster than 1 foreach(), 1 usort() and 2 imbricated array_walk(), But given that it is at the level of the compiler pass, it can be said that it is secondary :).

[HeahDude]

Yeah what about fixing your issue? :)

[francoispluchino]

@nicolas-grekas @HeahDude See the PR #20995.