-
-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
|
||
namespace Symfony\Component\Form; | ||
|
||
use Symfony\Component\Form\Exception\LogicException; | ||
use Symfony\Component\OptionsResolver\OptionsResolver; | ||
|
||
/** | ||
|
@@ -45,4 +46,22 @@ public function finishView(FormView $view, FormInterface $form, array $options) | |
public function configureOptions(OptionsResolver $resolver) | ||
{ | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
* | ||
* @deprecated since Symfony 4.2, use getExtendedTypes() instead. | ||
*/ | ||
public function getExtendedType() | ||
{ | ||
if (!method_exists($this, 'getExtendedTypes')) { | ||
throw new LogicException(sprintf('You need to implement the static getExtendedTypes() method when implementing the %s in %s.', FormTypeExtensionInterface::class, static::class)); | ||
chalasr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
@trigger_error(sprintf('The %s::getExtendedType() method is deprecated since Symfony 4.2 and will be removed in 5.0. Use getExtendedTypes() instead.', \get_class($this)), E_USER_DEPRECATED); | ||
|
||
foreach (static::getExtendedTypes() as $extendedType) { | ||
return $extendedType; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,9 +58,19 @@ public function getTypeExtensions($name) | |
foreach ($this->typeExtensionServices[$name] as $serviceId => $extension) { | ||
$extensions[] = $extension; | ||
|
||
// validate result of getExtendedType() to ensure it is consistent with the service definition | ||
if ($extension->getExtendedType() !== $name) { | ||
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())); | ||
if (method_exists($extension, 'getExtendedTypes')) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we shouldn't rather deprecate the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
$extendedTypes = array(); | ||
|
||
foreach ($extension::getExtendedTypes() as $extendedType) { | ||
$extendedTypes[] = $extendedType; | ||
} | ||
} else { | ||
$extendedTypes = array($extension->getExtendedType()); | ||
} | ||
|
||
// validate the result of getExtendedTypes()/getExtendedType() to ensure it is consistent with the service definition | ||
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, implode(', ', $extendedTypes))); | ||
} | ||
} | ||
} | ||
|
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.
@xabbuh following your comment, how do you plan to deprecate this method now?
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 about a deprecation message + BC layer here:
symfony/src/Symfony/Component/Form/AbstractExtension.php
Line 178 in 61da487
here:
symfony/src/Symfony/Component/Form/FormFactoryBuilder.php
Line 103 in 61da487
and here:
symfony/src/Symfony/Component/Form/FormFactoryBuilder.php
Line 114 in 61da487
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.
IMO we no longer need to deprecate the method.
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.
To elaborate a bit more on this: The new interface extends the existing
FormTypeExtensionInterface
. If we do not deprecate anything, we know that the extension will have thegetExtendedType()
method. And if we extend theAbstractTypeExtension
class, we even do not have to care about this method as it is then provided by the base class in case we implement the new 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.
And if we do not implement the new interface, nothing changes at all as we still need to implement the
getExtendedType()
method like we had to do before.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.
My concern about
getExtendedType()
in5.0
is that we have two methods almost equals (both return the type to extend) but the new one is much better, it also allows us autoconfigure the type extension, great! So when will it"getExtendedType()"
be used in5.0
userland?My point, we'll have two choice to return the type to extend for
5.0+
, the first one is a bad choice if the second one is better, good condition to deprecate it IMO.Plus, if we keep
getExtendedType()
inAbstractTypeExtension
forever, IDEs like PhpStorm will not be able to autodetect the missing method to implement anymore, this will be seen in the exception on complication time, worst DX then.At the end: I'd like (for
5.0
o6.0
) to see the static version inFormTypeExtensionInterface
instead of the currentgetExtendedType()
, is it possible in long term? I'm dreaming, probably a nightmare :)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.
getExtendedType()
will only there for BC concerns. But I am not convinced it's really necessary to deprecate it if the code will still work fine. That way we would save maintainers some hassle with updating their code.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.
@yceruto
getExtendedType()
is now deprecated in the interfaceThere 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?