Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

[Form] Allow to extend multiple types in a type extension #9584

Open
wants to merge 3 commits into from

7 participants

@egeloen
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #9507
License MIT
Doc PR -

The use case of this PR is to allow to extend all form types with a single type extension. Since the button type does not extend the form one, there is no root type which can be extended.

As pointed in the referenced ticket, I'm not sure what can be done/happened if a type extension extends two types where the first is a child of the second. The problem can become much more complex if we extend more than 2 types...

For the test part, I don't see where I can put them. Someone can give me the good direction?

@egeloen

@webmozart I would like to finish this PR. Can you give me your feeling about this feature and some advice?

@Burgov

since extended types is now an array, the method name should be $extension->getExtendedTypes(). getExtendedType can then be implemented for BC

@egeloen

@Burgov Thanks for your feedback! Anyway, I'm not sure how to implement it in a BC compatibility perspective. Should I add the new method to the FormTypeExtensionInterface and deprecate the old one. In this case, should I implement the two methods in the AbstractTypeExtension to maintain the BC. The old one will throw an exception (explaining it is deprecated) and the new one which converts the result of the old one into an array. If end users just overrides the old one, it is automatically converted into an array and if they override the new one, the old one will not be called. The issue is if they just use the interface, they must still implement the deprecated method and it will be a BC break as there is a new method on the interface...

What do you think?

@egeloen

@stof @fabpot What is the philosophy in order to rename a method in an interface while keeping the BC?

@liutec

I think this should also include an update to FormPass in order to allow specifying multiple aliases for the service tag.

$typeExtensions = array();

foreach ($container->findTaggedServiceIds('form.type_extension') as $serviceId => $tag) {
    $alias = isset($tag[0]['alias'])
        ? $tag[0]['alias']
        : $serviceId;

    $aliasArray = explode(',', $alias);
    foreach ($aliasArray as $typeName) {
        $typeExtensions[$typeName][] = $serviceId;
    }
}

$definition->replaceArgument(2, $typeExtensions);
@egeloen

Yes, I have forgotten to update it... But before, can someone point me the right direction in order to rename a method in an interface while keeping the BC?

@liutec

It's not a good idea to make changes to interfaces once they have been implemented (it kind of defeats the purpose of interfaces).

To maintain backward compatibility the best thing would be to create a new interface MultiFormTypeExtensionInterface with just the new method getExtendedTypes and check if the extension implements the new interface before calling the appropriate method:

if ($extension instanceof MultiFormTypeExtensionInterface) {
    $types = $extension->getExtendedTypes();
} else {
    $types = array($extension->getExtendedType());
}

Combine the fact that PHP is not strong typed with the possibility to add default values for parameters in the interface declaration and you get a third option:
function getExtendedType($asArray = false) { ... }

$types = $extension->getExtendedType(true);
// for BC only:
if (!is_array($types)) {
    $types = array($types);
}

This would not break the BC because all implementations satisfy the changed interface and all calls remain the same and default to the string return type but personally I don't like such methods.

@liutec

I don't know how to merge the changes into your branch so I've created a new one:
liutec@ba518bd

@egeloen

@liutec I will cherry-pick it

liutec and others added some commits
@liutec liutec [Form] Allow extending multiple types with a single type extension
Conflicts:
	src/Symfony/Component/Form/AbstractExtension.php
	src/Symfony/Component/Form/FormFactoryBuilder.php
	src/Symfony/Component/Form/Tests/Fixtures/TestExtension.php
83a29ad
@egeloen egeloen Minor CS fixes
c067cfa
@egeloen

What I don't like with this implementation is if we only implement the interfaces (FormTypeExtensionInterface and FormTypeExtensionMultiInterface) and don't use the AbstractTypeExtension, we still need to implement getExtendedType whereas we only need it for the multi one. Anyway, as PHP < 5.3.9 doesn't support to implement two interfaces which share the same methods, I don't see an other solution...

@liutec
@webmozart webmozart closed this
@webmozart webmozart reopened this
@webmozart
Collaborator

Sorry, wrong button.

Instead of a separate interface, I'd prefer to see the method added to FormTypeExtensionInterface and AbstractTypeExtension and have getExtendedType() deprecated. This breaks BC, but is technically possible, since FormTypeExtensionInterface is not marked as API.

However, I'm not sure whether this break is worth it before 3.0. You can achieve the same functionality by creating one abstract type extension and creating multiple implementations for different types.

What do you think?

@czenker

@webmozart I personally prefer being able just having one FormTypeExtension and applying it to multiple FormTypes. Having dozens of trivial FormTypeExtensions seems overly verbose to me.

Allowing a form extension to be applied to multiple form types in SF Framework (!) could be added easily right now without any BC break:

I'd say having some configuration like

form.extension.common:
    class: CommonExtension
    tags:
        - { name: form.type_extension, alias: form }
        - { name: form.type_extension, alias: button }
        - { name: form.type_extension, alias: reset }
        - { name: form.type_extension, alias: submit }

is way more readable and easier to understand.

To enable this feature only a few lines would need to be changed in FormPass:

foreach ($container->findTaggedServiceIds('form.type_extension') as $serviceId => $tags) {
  foreach ($tags as $tag) {
    $alias = isset($tag['alias']) ? $tag['alias'] : $serviceId;
    $typeExtensions[$alias][] = $serviceId;
  }
}

At the moment only the first tag is evaluated.

@stof
Collaborator

@czenker introducing a mismatch between the FormTypeExtensionInterface and the way the lazy extensions are applied is not an acceptable option.

@Misiur

Hey, anybody still working on this?

@egeloen

@Misiur If you want to finish it your welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 9, 2014
  1. @egeloen
  2. @liutec @egeloen

    [Form] Allow extending multiple types with a single type extension

    liutec authored egeloen committed
    Conflicts:
    	src/Symfony/Component/Form/AbstractExtension.php
    	src/Symfony/Component/Form/FormFactoryBuilder.php
    	src/Symfony/Component/Form/Tests/Fixtures/TestExtension.php
  3. @egeloen

    Minor CS fixes

    egeloen authored
This page is out of date. Refresh to see the latest.
View
4 src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/FormPass.php
@@ -51,7 +51,9 @@ public function process(ContainerBuilder $container)
? $tag[0]['alias']
: $serviceId;
- $typeExtensions[$alias][] = $serviceId;
+ foreach (explode(',', $alias) as $extendedType) {
+ $typeExtensions[$extendedType][] = $serviceId;
+ }
}
$definition->replaceArgument(2, $typeExtensions);
View
8 src/Symfony/Component/Form/AbstractExtension.php
@@ -171,9 +171,13 @@ private function initTypeExtensions()
throw new UnexpectedTypeException($extension, 'Symfony\Component\Form\FormTypeExtensionInterface');
}
- $type = $extension->getExtendedType();
+ $extendedTypes = $extension instanceof FormTypeExtensionMultiInterface
+ ? $extension->getExtendedTypes()
+ : array($extension->getExtendedType());
- $this->typeExtensions[$type][] = $extension;
+ foreach ($extendedTypes as $extendedType) {
+ $this->typeExtensions[$extendedType][] = $extension;
+ }
}
}
View
10 src/Symfony/Component/Form/AbstractTypeExtension.php
@@ -16,7 +16,7 @@
/**
* @author Bernhard Schussek <bschussek@gmail.com>
*/
-abstract class AbstractTypeExtension implements FormTypeExtensionInterface
+abstract class AbstractTypeExtension implements FormTypeExtensionInterface, FormTypeExtensionMultiInterface
{
/**
* {@inheritdoc}
@@ -45,4 +45,12 @@ public function finishView(FormView $view, FormInterface $form, array $options)
public function setDefaultOptions(OptionsResolverInterface $resolver)
{
}
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getExtendedTypes()
+ {
+ return array($this->getExtendedType());
+ }
}
View
10 src/Symfony/Component/Form/FormFactoryBuilder.php
@@ -100,7 +100,13 @@ public function addTypes(array $types)
*/
public function addTypeExtension(FormTypeExtensionInterface $typeExtension)
{
- $this->typeExtensions[$typeExtension->getExtendedType()][] = $typeExtension;
+ $extendedTypes = $typeExtension instanceof FormTypeExtensionMultiInterface
+ ? $typeExtension->getExtendedTypes()
+ : array($typeExtension->getExtendedType());
+
+ foreach ($extendedTypes as $extendedType) {
+ $this->typeExtensions[$extendedType][] = $typeExtension;
+ }
return $this;
}
@@ -111,7 +117,7 @@ public function addTypeExtension(FormTypeExtensionInterface $typeExtension)
public function addTypeExtensions(array $typeExtensions)
{
foreach ($typeExtensions as $typeExtension) {
- $this->typeExtensions[$typeExtension->getExtendedType()][] = $typeExtension;
+ $this->addTypeExtension($typeExtension);
}
return $this;
View
4 src/Symfony/Component/Form/FormTypeExtensionInterface.php
@@ -67,9 +67,9 @@ public function finishView(FormView $view, FormInterface $form, array $options);
public function setDefaultOptions(OptionsResolverInterface $resolver);
/**
- * Returns the name of the type being extended.
+ * Returns the names of the types being extended.
*
- * @return string The name of the type being extended
+ * @return string|array The names of the types being extended.
*/
public function getExtendedType();
}
View
25 src/Symfony/Component/Form/FormTypeExtensionMultiInterface.php
@@ -0,0 +1,25 @@
+<?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;
+
+/**
+ * @author Andrei Liutec <andrei@liutec.ro>
+ */
+interface FormTypeExtensionMultiInterface
+{
+ /**
+ * Returns an array with the names of the types being extended.
+ *
+ * @return array The names of the types being extended.
+ */
+ public function getExtendedTypes();
+}
View
9 src/Symfony/Component/Form/Tests/Extension/DataCollector/Type/DataCollectorTypeExtensionTest.php
@@ -35,6 +35,15 @@ public function testGetExtendedType()
$this->assertEquals('form', $this->extension->getExtendedType());
}
+ public function testGetExtendedTypes()
+ {
+ $types = $this->extension->getExtendedTypes();
+
+ $this->assertInternalType('array', $types);
+ $this->assertArrayHasKey(0, $types);
+ $this->assertEquals($this->extension->getExtendedType(), $types[0]);
+ }
+
public function testBuildForm()
{
$builder = $this->getMock('Symfony\Component\Form\Test\FormBuilderInterface');
View
10 src/Symfony/Component/Form/Tests/Fixtures/TestExtension.php
@@ -46,13 +46,13 @@ public function hasType($name)
public function addTypeExtension(FormTypeExtensionInterface $extension)
{
- $type = $extension->getExtendedType();
+ $extendedTypes = $extension instanceof FormTypeExtensionMultiInterface
+ ? $extension->getExtendedTypes()
+ : array($extension->getExtendedType());
- if (!isset($this->extensions[$type])) {
- $this->extensions[$type] = array();
+ foreach ($extendedTypes as $extendedType) {
+ $this->extensions[$extendedType][] = $extension;
}
-
- $this->extensions[$type][] = $extension;
}
public function getTypeExtensions($name)
Something went wrong with that request. Please try again.