Skip to content

[Form] simplify the form type extension registration #24530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 10, 2018

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Oct 12, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22833, #27906
License MIT
Doc PR

@xabbuh
Copy link
Member Author

xabbuh commented Oct 12, 2017

changelog and upgrade files need to be updated

@@ -65,7 +65,7 @@
"phpunit/phpunit": "<4.8.35|<5.4.3,>=5.0",
"symfony/asset": "<3.4",
"symfony/console": "<3.4",
"symfony/form": "<3.4",
"symfony/form": "<4.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

must be changed to <4.1 when the 4.0 branch is created

@vudaltsov
Copy link
Contributor

Great!

By the way, does it make sense to be able to extend multiple types with one extension?
Can it be implemented?

@xabbuh xabbuh force-pushed the issue-22833 branch 2 times, most recently from b26fc2d to aecdc5f Compare October 12, 2017 11:55
@@ -39,7 +39,7 @@
"symfony/dom-crawler": "~3.4|~4.0",
"symfony/polyfill-intl-icu": "~1.0",
"symfony/security": "~3.4|~4.0",
"symfony/form": "~3.4|~4.0",
"symfony/form": "~4.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

would need to be changed to ~4.1

@xabbuh
Copy link
Member Author

xabbuh commented Oct 12, 2017

@vudaltsov Implementing it wouldn't be hard. But IMO it's not really worth it.

@xabbuh
Copy link
Member Author

xabbuh commented Oct 12, 2017

We could also think about deprecating the FormTypeExtensionInterface::getExtendedType() method (providing an implementation in AbstractTypeExtension for a smooth upgrade path) as it will be pretty useless in Symfony 5.

UPGRADE-4.1.md Outdated
----

* Support for the `extended_type` attribute inside the `FormPass` is deprecated and will be dropped in Symfony 5.0.
Implement the `public static function extends(): string` method in your form type extension class instead.
Copy link
Member

Choose a reason for hiding this comment

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

extendsType()? Same in UPGRADE-5.0.md and src/Symfony/Component/Form/CHANGELOG.md

UPGRADE-4.1.md Outdated
Implement the `public static function extends(): string` method in your form type extension class instead.

* Not implementing the `extendsType()` method when implementing the `FormTypeExtensionInterface` is deprecated. The
method will be added to the `FormTypeExtensionInterface` in Symfony 4.0.
Copy link
Member

Choose a reason for hiding this comment

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

in Symfony 5.0? Same in src/Symfony/Component/Form/CHANGELOG.md

@@ -134,4 +135,9 @@ public function getExtendedType()
{
return 'Symfony\Component\Form\Extension\Core\Type\FormType';
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about return self::extendsType(); to make clearer the migration path?


$this->fail();
} catch (InvalidArgumentException $e) {
$this->assertSame('"form.type_extension" tagged services must have the extended type configured using the extended_type/extended-type attribute, none was configured for the "my.type_extension" service.', $e->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

why not keeping @expectedException and @expectedExceptionMessage ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@expectedDeprecation would not be evaluated then


if (method_exists($this, 'extendsType')) {
return $this::extendsType();
}
Copy link
Member

Choose a reason for hiding this comment

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

you must throw an exception here, telling to implement the other method or to override this one, because returning null is not allowed.

@@ -70,6 +72,8 @@ public function configureOptions(OptionsResolver $resolver);
* Returns the name of the type being extended.
*
* @return string The name of the type being extended
*
* @deprecated since version 4.1, implement extendsType() instead
Copy link
Contributor

Choose a reason for hiding this comment

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

we often add the to-be-added method as commented code in the interface

Copy link
Contributor

Choose a reason for hiding this comment

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

using @method seems like a new approach. works both I guess

@xabbuh
Copy link
Member Author

xabbuh commented Nov 23, 2017

Status: Needs Review

@Tobion
Copy link
Contributor

Tobion commented Jan 29, 2018

This needs a rebase. Also I think we don't necessarily need to deprecate extended_type tag attribute. Just making it optional and using the static extendsType method by default should be enough. This covers 99% of the cases. If someone uses the same class with different services instances for different extensions, they can still use the extended_type attribute. This would then be similar to the command attribute of the console.command tag.

Also this change allows to autoconfigure form type extensions. So I guess this should be added as well.

@xabbuh xabbuh added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 12, 2018
@@ -58,8 +58,18 @@ public function getTypeExtensions($name)
foreach ($this->typeExtensionServices[$name] as $serviceId => $extension) {
$extensions[] = $extension;

if (method_exists($extension, 'getExtendedTypes')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This validation prevents to use different instances of the same class to be used for different extensions. So it prevents the only advantage of extended_type attribute that I mentioned in #24530 (comment) as people will be forced to implement the method in 5.0 when its part of the interface. So they need to return an empty array and then overwrite it with the service attribute.

IMO we do not need the validation anymore as now both the registration and implementation uses getExtendedTypes. So there should not be a mismatch anymore unless you wire something wrong manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

class FooBarTypeExtension extends AbstractTypeExtension
{
    public function __construct(string $type)
    {
    }

    public static function getExtendedTypes(): iterable
    {
        return array(
            FooType::class,
            BarType::class,
        );
    }
}
App\Form\FooBarTypeExtension:
    arguments: ['foo']
    tags:
        - name: form.type_extension
          extended_type: FooType

App\Form\FooBarTypeExtension:
    arguments: ['bar']
    tags:
        - name: form.type_extension
          extended_type: BarType

Do I understand correctly that you have something like this in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. By returning both types from getExtendedTypes, it actually works even with the validation. So I'm fine keeping it like this. But I think it would make more sense to remove the validation as it should not matter what is returned by getExtendedTypes if you set the extended_type attribute (and you might not even know that in advance if you create a dynamic form extension).

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if we shouldn't rather deprecate the extended_type attribute too instead I could imagine that using it adds more confusion than an implementation where getExtendedTypes() is the single source of truth.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it potentially is complicating things for no real use-case. But I also like that it allows to configure extensions dynamically via DI (instead of hardcoding it into a class). As I said in #24530 (comment), its similar to the command attribute of the console.command tag. So it would be consistent to keep it. But I would not object to deprecate it as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it for now. We can still consider to deprecate it later when we noticed that it adds too much confusion.

// validate result of getExtendedType() to ensure it is consistent with the service definition
if ($extension->getExtendedType() !== $name) {
if (!\in_array($name, $extendedTypes, true)) {
throw new InvalidArgumentException(sprintf('The extended type specified for the service "%s" does not match the actual extended type. Expected "%s", given "%s".', $serviceId, $name, $extension->getExtendedType()));
Copy link
Contributor

Choose a reason for hiding this comment

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

$extension->getExtendedType() would need to be replaced by $extendedTypes. But as said above, the whole thing could be removed.

@@ -45,4 +46,13 @@ public function finishView(FormView $view, FormInterface $form, array $options)
public function configureOptions(OptionsResolver $resolver)
{
}

public function getExtendedType()
Copy link
Contributor

Choose a reason for hiding this comment

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

With getExtendedType deprecated, all the calls that are still done to it need to use the new static method instead when it exists. E.g. in AbstractExtension that yceruto mentioned. And it should also trigger a deprecation message or is this done automatically?

@xabbuh
Copy link
Member Author

xabbuh commented Oct 8, 2018

@Tobion I added some deprecation triggers at some places where type extensions are registered. This also allowed me to discover some places that still needed to be updated.

Copy link
Contributor

@Tobion Tobion left a comment

Choose a reason for hiding this comment

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

Good job. I think moving this to a static method makes sense as it allows autowiring and is consistent with similar concepts that need to be known at container comilation like EventSubscriberInterface and MessageHandlerSubscriberInterface.

@fabpot
Copy link
Member

fabpot commented Oct 10, 2018

Thank you @xabbuh.

@fabpot fabpot merged commit 6a1d4c5 into symfony:master Oct 10, 2018
fabpot added a commit that referenced this pull request Oct 10, 2018
…xabbuh)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Form] simplify the form type extension registration

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22833, #27906
| License       | MIT
| Doc PR        |

Commits
-------

6a1d4c5 simplify the form type extension registration
@xabbuh xabbuh deleted the issue-22833 branch October 10, 2018 09:21
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
symfony-splitter pushed a commit to symfony/form that referenced this pull request May 29, 2019
…ethod (yceruto)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Form] Remove legacy code related to getExtendedType() method

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Ref: symfony/symfony#24530

Commits
-------

208b729bca Remove legacy code related to getExtendedType() method
nicolas-grekas added a commit that referenced this pull request May 29, 2019
…ethod (yceruto)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Form] Remove legacy code related to getExtendedType() method

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Ref: #24530

Commits
-------

208b729 Remove legacy code related to getExtendedType() method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.