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

[Form] simplify the form type extension registration #24530

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@xabbuh
Member

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
License MIT
Doc PR
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

changelog and upgrade files need to be updated

Member

xabbuh commented Oct 12, 2017

changelog and upgrade files need to be updated

@vudaltsov

This comment has been minimized.

Show comment
Hide comment
@vudaltsov

vudaltsov Oct 12, 2017

Contributor

Great!

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

Contributor

vudaltsov commented Oct 12, 2017

Great!

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

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

Member

xabbuh commented Oct 12, 2017

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Oct 12, 2017

Member

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.

Member

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.

Show outdated Hide outdated UPGRADE-4.1.md
Show outdated Hide outdated UPGRADE-4.1.md
@@ -134,4 +135,9 @@ public function getExtendedType()
{
return 'Symfony\Component\Form\Extension\Core\Type\FormType';

This comment has been minimized.

@yceruto

yceruto Oct 12, 2017

Contributor

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

@yceruto

yceruto Oct 12, 2017

Contributor

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

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Nov 23, 2017

Member

Status: Needs Review

Member

xabbuh commented Nov 23, 2017

Status: Needs Review

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Jan 29, 2018

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Mar 22, 2018

Member

Based on the feedback I have entirely reworked the implementation. There is now a new SelfConfigurableFormTypeExtension interface . If your form type extension implements its static extendsType() method, the registration of the extension does not require the extended-type attribute anymore.

The only thing that is open now is to find a better name for the new interface as I am not very happy with it.

ping @symfony/deciders

Member

xabbuh commented Mar 22, 2018

Based on the feedback I have entirely reworked the implementation. There is now a new SelfConfigurableFormTypeExtension interface . If your form type extension implements its static extendsType() method, the registration of the extension does not require the extended-type attribute anymore.

The only thing that is open now is to find a better name for the new interface as I am not very happy with it.

ping @symfony/deciders

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Mar 22, 2018

Member

Oh very nice feature. I'm currently upgrading an app to the new architecture and I hit this "not so good DX" where I had to add the tag explicitly.

About the naming I have no strong opinion but what about: GetExtentedTypeInterface? Because it's quite explicite about what it does

Member

lyrixx commented Mar 22, 2018

Oh very nice feature. I'm currently upgrading an app to the new architecture and I hit this "not so good DX" where I had to add the tag explicitly.

About the naming I have no strong opinion but what about: GetExtentedTypeInterface? Because it's quite explicite about what it does

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

This comment has been minimized.

@yceruto

yceruto Mar 22, 2018

Contributor

@xabbuh following your comment, how do you plan to deprecate this method now?

@yceruto

yceruto Mar 22, 2018

Contributor

@xabbuh following your comment, how do you plan to deprecate this method now?

This comment has been minimized.

@yceruto

yceruto Mar 22, 2018

Contributor

What about a deprecation message + BC layer here:

$type = $extension->getExtendedType();

here:

$this->typeExtensions[$typeExtension->getExtendedType()][] = $typeExtension;

and here:

$this->typeExtensions[$typeExtension->getExtendedType()][] = $typeExtension;

@yceruto

yceruto Mar 22, 2018

Contributor

What about a deprecation message + BC layer here:

$type = $extension->getExtendedType();

here:

$this->typeExtensions[$typeExtension->getExtendedType()][] = $typeExtension;

and here:

$this->typeExtensions[$typeExtension->getExtendedType()][] = $typeExtension;

This comment has been minimized.

@xabbuh

xabbuh Mar 22, 2018

Member

IMO we no longer need to deprecate the method.

@xabbuh

xabbuh Mar 22, 2018

Member

IMO we no longer need to deprecate the method.

This comment has been minimized.

@xabbuh

xabbuh Mar 22, 2018

Member

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 the getExtendedType() method. And if we extend the AbstractTypeExtension 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.

@xabbuh

xabbuh Mar 22, 2018

Member

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 the getExtendedType() method. And if we extend the AbstractTypeExtension 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.

This comment has been minimized.

@xabbuh

xabbuh Mar 22, 2018

Member

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.

@xabbuh

xabbuh Mar 22, 2018

Member

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.

This comment has been minimized.

@yceruto

yceruto Mar 23, 2018

Contributor

My concern about getExtendedType() in 5.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 in 5.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() in AbstractTypeExtension 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 o 6.0) to see the static version in FormTypeExtensionInterface instead of the current getExtendedType(), is it possible in long term? I'm dreaming, probably a nightmare :)

@yceruto

yceruto Mar 23, 2018

Contributor

My concern about getExtendedType() in 5.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 in 5.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() in AbstractTypeExtension 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 o 6.0) to see the static version in FormTypeExtensionInterface instead of the current getExtendedType(), is it possible in long term? I'm dreaming, probably a nightmare :)

This comment has been minimized.

@xabbuh

xabbuh Apr 20, 2018

Member

getExtendedType() will only there for BC concers. 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.

@xabbuh

xabbuh Apr 20, 2018

Member

getExtendedType() will only there for BC concers. 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.

@xabbuh xabbuh changed the title from [Form] deprecate the extended_type tag attribute to [Form] simplify the form type extension registration Mar 23, 2018

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Mar 23, 2018

Member

And of course we can now also autoconfigure these form type extensions.

Member

xabbuh commented Mar 23, 2018

And of course we can now also autoconfigure these form type extensions.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jul 9, 2018

Member

@symfony/deciders I would like to hear your opinions on this proposal. :)

Member

xabbuh commented Jul 9, 2018

@symfony/deciders I would like to hear your opinions on this proposal. :)

@ogizanagi

Looks good to me

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 9, 2018

Member

I like this feature a lot ... but I dislike the proposed implementation. I understand that there are some technical constraints that prevent us from doing it better ... but the proposed resulting code is too verbose and a bit ugly:

class DataCollectorTypeExtension
      extends AbstractTypeExtension
      implements SelfConfigurableFormTypeExtension
{
    // ...
}
  • Using both extends and implements is a bit ... ugly
  • One of them is called just "type" (...TypeExtension) but the other is "form type" (...FormTypeExtension)
  • "SelfConfigurable..." prefix is a bit ugly too.

Now, I'll make a proposal (but I don't know forms, so maybe it won't work). What if we add the new extendsType(): string method to the existing AbstractTypeExtension and we add this code to it:

public static function extendsType()
{
    throw new \RuntimeException('...');
}

And the error message should say:

  • You are relying on form type autoconfiguration but you haven't defined the new extendsType() method. You have two options:
    • Either configure the form type service manually...
    • ...or implement the extendsType() method to return the same value as getExtendedType()

If this works, we could even deprecate the getExtendedType() method at the same time?

Member

javiereguiluz commented Jul 9, 2018

I like this feature a lot ... but I dislike the proposed implementation. I understand that there are some technical constraints that prevent us from doing it better ... but the proposed resulting code is too verbose and a bit ugly:

class DataCollectorTypeExtension
      extends AbstractTypeExtension
      implements SelfConfigurableFormTypeExtension
{
    // ...
}
  • Using both extends and implements is a bit ... ugly
  • One of them is called just "type" (...TypeExtension) but the other is "form type" (...FormTypeExtension)
  • "SelfConfigurable..." prefix is a bit ugly too.

Now, I'll make a proposal (but I don't know forms, so maybe it won't work). What if we add the new extendsType(): string method to the existing AbstractTypeExtension and we add this code to it:

public static function extendsType()
{
    throw new \RuntimeException('...');
}

And the error message should say:

  • You are relying on form type autoconfiguration but you haven't defined the new extendsType() method. You have two options:
    • Either configure the form type service manually...
    • ...or implement the extendsType() method to return the same value as getExtendedType()

If this works, we could even deprecate the getExtendedType() method at the same time?

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jul 9, 2018

Member

@javiereguiluz Most of what you suggest could be implemented. There is just one single drawback with not having a dedicated interface: We would not be able to support autoconfiguration of form type extensions. This could only happen as of Symfony 5 where we could introduce a new method in the interface.

Member

xabbuh commented Jul 9, 2018

@javiereguiluz Most of what you suggest could be implemented. There is just one single drawback with not having a dedicated interface: We would not be able to support autoconfiguration of form type extensions. This could only happen as of Symfony 5 where we could introduce a new method in the interface.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Jul 9, 2018

Member

Thinking more about it here is what we could do:

  • deprecate FormTypeExtensionInterface::getExtendedType()
  • implement extendsType() in AbstractTypeExtension but let it throw an exception
  • skip definitions in the compiler pass that are tagged with form.type_extension without additional attributes, but whose classes do not implement the extendsType() method or where the implementation comes from AbstractTypeExtension
  • trigger deprecations when a class implementing the FormTypeExtensionInterface does not implement extendsType()
  • autoconfigure service definitions implementing FormTypeExtensionInterface

What do you think?

Member

xabbuh commented Jul 9, 2018

Thinking more about it here is what we could do:

  • deprecate FormTypeExtensionInterface::getExtendedType()
  • implement extendsType() in AbstractTypeExtension but let it throw an exception
  • skip definitions in the compiler pass that are tagged with form.type_extension without additional attributes, but whose classes do not implement the extendsType() method or where the implementation comes from AbstractTypeExtension
  • trigger deprecations when a class implementing the FormTypeExtensionInterface does not implement extendsType()
  • autoconfigure service definitions implementing FormTypeExtensionInterface

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment