Regression in Form extension since 3.2 #20332

romainneutron · 2016-10-27T14:35:59Z

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

The text was updated successfully, but these errors were encountered:

romainneutron · 2016-10-27T14:43:45Z

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 · 2016-11-15T11:40:12Z

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 · 2016-11-15T13:56:09Z

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 · 2016-12-19T18:23:40Z

@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 · 2016-12-19T18:50:17Z

@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 · 2016-12-19T19:37:16Z

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 · 2016-12-19T19:56:43Z

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 kn 8000 ow what to say now.

nicolas-grekas · 2016-12-19T20:08:22Z

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 · 2016-12-19T22:25:36Z

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

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:

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 · 2016-12-19T22:29:32Z

@francoispluchino see #20993 :)

francoispluchino · 2016-12-19T22:32:24Z

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

HeahDude · 2016-12-19T22:33:35Z

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 · 2016-12-19T22:48:51Z

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 · 2016-12-19T22:51:23Z

Yeah what about fixing your issue? :)

francoispluchino · 2016-12-20T09:48:03Z

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

…ass trait (francoispluchino) This PR was merged into the 3.2 branch. Discussion ---------- [DependencyInjection] Fix the priority order of compiler pass trait | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20332, #20993 | License | MIT | Doc PR | ~ A regression has appeared between the version `3.1` and `3.2` on the compilation passes using a priority option (see #20332). Indeed, the order is no longer preserved for service definitions with the same priority. This is caused by the `SplPriorityQueue` class that does not have a FIFO implementation (see this [comment](#20332 (comment))). The PR #20993 fixes the problem but only for Forms, not for all compiler passes using the `PriorityTaggedServiceTrait` trait since `3.2`. Commits ------- aac9a7e Fix the priority order of compiler pass trait

…ass trait (francoispluchino) This PR was merged into the 3.2 branch. Discussion ---------- [DependencyInjection] Fix the priority order of compiler pass trait | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20332, #20993 | License | MIT | Doc PR | ~ A regression has appeared between the version `3.1` and `3.2` on the compilation passes using a priority option (see #20332). Indeed, the order is no longer preserved for service definitions with the same priority. This is caused by the `SplPriorityQueue` class that does not have a FIFO implementation (see this [comment](symfony/symfony#20332 (comment))). The PR #20993 fixes the problem but only for Forms, not for all compiler passes using the `PriorityTaggedServiceTrait` trait since `3.2`. Commits ------- aac9a7e Fix the priority order of compiler pass trait

romainneutron added Form Bug labels Oct 27, 2016

carsonbot added the Status: Needs Review label Oct 27, 2016

romainneutron closed this as completed Oct 27, 2016

daFish mentioned this issue Nov 3, 2016

Change priority of DisableCSRFExtension to make it compatible with Symfony 3.2 FriendsOfSymfony/FOSRestBundle#1602

Closed

HeahDude mentioned this issue Dec 19, 2016

[FrameworkBundle] Fixed form type extensions priority ordering #20993

Closed

francoispluchino mentioned this issue Dec 20, 2016

[DependencyInjection] Fix the priority order of compiler pass trait #20995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Regression in Form extension since 3.2 #20332

Regression in Form extension since 3.2 #20332

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Regression in Form extension since 3.2 #20332

Regression in Form extension since 3.2 #20332

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!