-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
xabbuh
commented
Oct 12, 2017
•
edited
Loading
edited
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 |
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", |
There was a problem hiding this comment.
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
Great! By the way, does it make sense to be able to extend multiple types with one extension? |
b26fc2d
to
aecdc5f
Compare
@@ -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", |
There was a problem hiding this comment.
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
@vudaltsov Implementing it wouldn't be hard. But IMO it's not really worth it. |
We could also think about deprecating the |
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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(); | ||
} |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Status: Needs Review |
This needs a rebase. Also I think we don't necessarily need to deprecate Also this change allows to autoconfigure form type extensions. So I guess this should be added as well. |
@@ -58,8 +58,18 @@ public function getTypeExtensions($name) | |||
foreach ($this->typeExtensionServices[$name] as $serviceId => $extension) { | |||
$extensions[] = $extension; | |||
|
|||
if (method_exists($extension, 'getExtendedTypes')) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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())); |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
@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. |
There was a problem hiding this 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.
Thank you @xabbuh. |
…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
…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
…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