Skip to content

Commit

Permalink
[Form] Move CSRF options from types to the CSRF extension
Browse files Browse the repository at this point in the history
  • Loading branch information
vicb committed May 18, 2011
1 parent fdbdcbb commit ba31b5a
Show file tree
Hide file tree
Showing 11 changed files with 140 additions and 5 deletions.
Expand Up @@ -121,7 +121,6 @@ public function getDefaultOptions(array $options)
'choice_list' => null,
'choices' => array(),
'preferred_choices' => array(),
'csrf_protection' => false,
'empty_data' => $multiple || $expanded ? array() : '',
'error_bubbling' => false,
);
Expand Down
Expand Up @@ -123,7 +123,6 @@ public function getDefaultOptions(array $options)
'format' => \IntlDateFormatter::MEDIUM,
'data_timezone' => null,
'user_timezone' => null,
'csrf_protection' => false,
// Don't modify \DateTime classes by reference, we treat
// them like immutable value objects
'by_reference' => false,
Expand Down
Expand Up @@ -58,7 +58,6 @@ public function getDefaultOptions(array $options)
{
return array(
'type' => 'string',
'csrf_protection' => false,
);
}

Expand Down
Expand Up @@ -36,7 +36,6 @@ public function getDefaultOptions(array $options)
'options' => array(),
'first_name' => 'first',
'second_name' => 'second',
'csrf_protection' => false,
'error_bubbling' => false,
);
}
Expand Down
Expand Up @@ -97,7 +97,6 @@ public function getDefaultOptions(array $options)
'pattern' => null,
'data_timezone' => null,
'user_timezone' => null,
'csrf_protection' => false,
// Don't modify \DateTime classes by reference, we treat
// them like immutable value objects
'by_reference' => false,
Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/Form/Extension/Csrf/CsrfExtension.php
Expand Up @@ -34,7 +34,12 @@ protected function loadTypes()
protected function loadTypeExtensions()
{
return array(
new Type\ChoiceTypeCsrfExtension(),
new Type\DateTypeCsrfExtension(),
new Type\FileTypeCsrfExtension(),
new Type\FormTypeCsrfExtension(),
new Type\RepeatedTypeCsrfExtension(),
new Type\TimeTypeCsrfExtension(),
);
}
}
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Csrf\Type;

use Symfony\Component\Form\AbstractTypeExtension;

class ChoiceTypeCsrfExtension extends AbstractTypeExtension
{
public function getDefaultOptions(array $options)
{
return array('csrf_protection' => false);
}

public function getExtendedType()
{
return 'choice';
}
}
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Csrf\Type;

use Symfony\Component\Form\AbstractTypeExtension;

class DateTypeCsrfExtension extends AbstractTypeExtension
{
public function getDefaultOptions(array $options)
{
return array('csrf_protection' => false);
}

public function getExtendedType()
{
return 'date';
}
}
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Csrf\Type;

use Symfony\Component\Form\AbstractTypeExtension;

class FileTypeCsrfExtension extends AbstractTypeExtension
{
public function getDefaultOptions(array $options)
{
return array('csrf_protection' => false);
}

public function getExtendedType()
{
return 'file';
}
}
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Csrf\Type;

use Symfony\Component\Form\AbstractTypeExtension;

class RepeatedTypeCsrfExtension extends AbstractTypeExtension
{
public function getDefaultOptions(array $options)
{
return array('csrf_protection' => false);
}

public function getExtendedType()
{
return 'repeated';
}
}
@@ -0,0 +1,27 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Form\Extension\Csrf\Type;

use Symfony\Component\Form\AbstractTypeExtension;

class TimeTypeCsrfExtension extends AbstractTypeExtension
{
public function getDefaultOptions(array $options)
{
return array('csrf_protection' => false);
}

public function getExtendedType()
{
return 'time';
}
}

9 comments on commit ba31b5a

@kriswallsmith
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FormTypeExtension removes the CSRF field from any non-root view -- doesn't that make all these extensions moot?

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cant see the benefit of having all theese classes instead of the default option on the types them selves, seems a bit overkill. And i think what Kris wrote is correct.

@vicb
Copy link
Contributor Author

@vicb vicb commented on ba31b5a May 19, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kriswallsmith the condition is if ((!$form->hasParent() || $form->getParent()->isRoot()) so it can be true for children - I think I have tested without these extensions and it failed but I will double check tomorrow.

@henrikbjorn that is the way Form is architectured and should be used. (And the default types can be used without the csrf extension in which case the option could be misleading)

@vicb
Copy link
Contributor Author

@vicb vicb commented on ba31b5a May 19, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kris, forget about the condition thing, the second part is still right...

@vicb
Copy link
Contributor Author

@vicb vicb commented on ba31b5a May 19, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I start remembering now, I first dropped the csrf_protection (should be the config you are talking about) and:

  • The file type does not render the csrf field,
  • The choice type would interpret the csrf field as an option

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extensions just add options that could be on the types them selves. Basically you are just moving a single option to its own class i cant see the benefit in this.

@vicb
Copy link
Contributor Author

@vicb vicb commented on ba31b5a May 20, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base types do not support CSRF, CSRF is supported by loading the CSRF extension.

This means that this option has no effect if the CSRF extension is not loaded. That's why this option has been added to the type extensions provided by the CSRF extension. So that when you load the CSRF extension, you get the CSRF protection and the types get extended with the CSRF related option.

It would work by leaving the option in the base types but it is definitely not the right way - if we keep the option in the base types then may be we should also make CSRF the default and no more an extension.

@henrikbjorn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about then having a fieldtypeextension that have it being false and a formtypeextension where it is true instead of replicating a bunch of classes that does the same thing.

@vicb
Copy link
Contributor Author

@vicb vicb commented on ba31b5a May 20, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you disable CSRF on a form then ?

There are probably better solutions than what I have proposed, you should submit a PR when you have a working one.

Please sign in to comment.