Symfony: [Form] RFC: Replace dynamic by proper type inheritance

[webmozart]

Since dynamic inheritance is inherently flawed (#3793), it should be replaced by proper PHP inheritance. This should make it much more intuitive for newcomers to extend a type. This change has been made possible by the recently added class DefaultOptions (#3789).

A BC layer should allow to use the old and new API side by side. Core types should be moved to the new API.

API before:

class MyType extends AbstractType
{
    public function getName()
    {
        return 'my_type';
    }

    public function getParent()
    {
        return 'form';
    }

    public function buildForm(FormBuilder $builder, array $options)
    {
        // ...
    }

    public function buildView(FormView $view, FormInterface $form)
    {
        // ...
    }

    public function buildViewBottomUp(FormView $view, FormInterface $form)
    {
        // ...
    }

    public function getDefaultOptions()
    {
        return ...
    }
}

API after:

class MyType extends FormType
{
    public function getName()
    {
        return 'my_type';
    }

    public function buildForm(FormBuilder $builder, array $options)
    {
        // no parent::buildForm() call

        // ...
    }

    public function buildView(FormView $view, FormInterface $form)
    {
        // no parent::buildView() call

        // ...
    }

    public function finishView(FormView $view, FormInterface $form)
    {
        // no parent::finishView() call

        // ...
    }

    public function setDefaultOptions(OptionsResolver $resolver)
    {
        // no parent::setDefaultOptions() call

        $resolver->setDefaults(array(...));
    }
}

As you can see, the type methods must not call their parent methods. This allows the factory to call each single method at the appropriate time and to interline the type extension methods in between.

Positive effects:

• Understandable inheritance
• Use of instanceof, e.g. $form->getType() instanceof MyType
• Reuse of protected methods from the parent type

Negative effects:

• If you call parent::.. in any of the interface methods, this will lead to strange side effects. I'm not sure if we can catch these situations and throw an exception.

[marijn]

:+1: I never really understood why it wasn't like this in the first place. Great to see this will be changed. Any chance this will be finished for a 2.1 release?

[webmozart]

It was like this in the first place because the differentiation between FieldType and FormType seemed necessary. As I know now, this is not the case anymore (#3878).

[stof]

@bschussek I see 2 potential issues here:

• people are likely to forget to overwrite getName, which would break things as it would replace the parent type in the factory (which is the reason why we removed the guessing implementation in the AbstractType during the beta cycle to force the user to specify a name)
• forgetting to call the parent method is likely to break things in the rendering (or in other methods)

[webmozart]

@stof As for the first issue, I just updated the description. As for the second, yes. We can't change this, but people will (should) notice immediately.

[rande]

I found this less intuitive ;) The effort should be on documentation and clean API.

[marijn]

[...]forgetting to call the parent method[...]

You can never prevent that, but one might expect from a decent developer to be smart enough to call a parent method right?

[stof]

@marijn From my experience on providing support for Symfony during one year, I see 2 possible answers: either decent developers are not (always) smart enough for it or symfony users are not (always) decent developers. They are able to forget the parent call even when they copy-paste code from the doc and the doc example contains it.

[marijn]

@stof well you definitely are the authority on that matter :smile:

[alvarezmario]

@stof it is always possible to add a big tip on the Form documentation to clarify to such developers that they should call the parent method.
I don't think this point to be a valid reason to be against this change, which seems pretty good.

[docteurklein]

I think using php's inheritance instead of artificial one is better, because it seems equally complicated to understand for mid-level users.
I would even say that understanding how getParent() is used is more hard to explain than simple class inheritance.
So i'm +1 for this change.

[tystr]

+1

[willdurand]

Will it be the last change on this component?

A BC layer should allow to use the old and new API side by side. Core types should be moved to the new API.

Does it mean Symfony2 will support the old API forever?

[stof]

@willdurand no. A BC layer would be marked a deprecated, meaning it would be removed later. See for instance the Session methods related to flash messages. They are tagged as deprecated and documented as being dropped for 2.3

[jmikola]

@bschussek: I recently upgraded our Symfony submodule to yesterday's pointer, as we needed all of the recent Form PR's. This one obviously is not in our fork, since it has yet to be merged.

We're still having an issue with a string-based date field. We have a custom form type that uses DateType as its parent, and sets the widget option's default value to single_text. The problem is that the field ends up being created with a data mapper. I believe this is due to:

[BC BREAK] FormType::getParent() does not see default options anymore

The rest of the default options in the base DateType include my child type's default as I'd expect, but the form type itself isn't correct (well, it's technically "correct" given the noted BC break, but it's broken for me :)

Will this PR address the issue of option defaults being used to decide parent types? For now, we can get by with redundantly specifying the widget option when building the form that uses our custom field.

[webmozart]

@jmikola Your problem will be addressed by #3878.

[jmikola]

Thanks, @bschussek

[webmozart]

I discovered a major issue: type extensions are very difficult to implement with native inheritance. They need to be triggered after each method of the type they are extending, i.e.:

$type = EntityType();
$type->buildForm(...)

with native inheritance results in (due to the parent::buildForm() calls at the very beginning)

1.  FormType::buildForm()
2.  ChoiceType::buildForm()
3.  EntityType::buildForm()

but should result in the execution chain

1.  FormType::buildForm()
1a. all FormType extensions ::buildForm()
2.  ChoiceType::buildForm()
2a. all ChoiceType extensions ::buildForm()
3.  EntityType::buildForm()
3a. all EntityType extensions ::buildForm()

Obviously, this is non-trivial to implement. It would require to generate Proxy classes for the types in order to simulate AOP-like behavior, which sucks somewhat and is just as intransparent as the current architecture.

Ideas?

[marijn]

I'm not entirely sure I understand your question. The order of execution is incorrect or you don't want an execution chain down to the parent class? If you could explain it with an example it would be easier to help.

[stof]

I don't want to remove the type extensions as they are a great extension point of the component. And if the new version is not cleaner than the current one, I don't really see the added value about breaking BC (and I see the drawback of breaking BC)

[marijn]

I agree with @stof on that: the only reason reason for a BC break in the type system is if we can replace it with native inheritance.

[Tobion]

@bschussek I guess the extensions must be called inbetween because they might modify the form and these changes must be available to sub-types, correct? Looking at your example, my first idea would be to include this logic in AbstractType, so all extensions get called as soon as a the method of the type is executed. I guess that is too simple. But why does it not work?

AbstractType::buildForm(FormBuilder $builder, array $options)
{
    forearch ($this->getExtensions() as $extension) {
        $extension->buildForm($builder, $options);
    }
}

[schmittjoh]

Maybe just do not pass in the options to the getParent() method and leave inheritance as is otherwise. At least, that should address the original ticket which triggered this.

As @stof said, the extensions are a great feature, and I'd even say that they are not only difficult but impossible to implement. You would not only have to generate proxy classes, but also rewrite userland code to extend a different class.

[webmozart]

@Tobion: Your example does not work, because buildForm() in your example is only called on top of the inheritance chain (AbstractType) and not between each layer.

@stof, @schmittjoh: Removing extensions is out of question. The only feasible way I see (which is basically leaving in the current approach but using PHP inheritance) is to avoid calling the parent methods, i.e.

class MyType extends FormType
{
    public function buildForm()
    {
        // no call to parent::buildForm here
    }
}

The framework could then take a look at the inheritance chain and invoke each method in the chain separately "FormType::buildForm", then the extensions, then "MyType::buildForm" etc.). But I don't think this makes the code any clearer.

[Tobion]

@bschussek you're right. Why not include this logic explicisely in each type:

class FormType extends AbstractType
{
    public function buildForm(...)
    {
        parent::buildForm(...)
        forearch ($this->getExtensions() as $extension) {
            $extension->buildForm($builder, $options);
        }
        // or offer a shortcut method for this logic
    }
}

It wouldn't be a problem to always use this for all core types. And for custom user types, it's the developers responsibility to implement this logic if he needs it. And he only needs it when he has added extensions to one of his own types, so he would already know that he must call it.

class MyType extends FormType
{
    public function buildForm(...)
    {
        parent::buildForm(...)
        // call it or not (depending on needs)
        forearch ($this->getExtensions() as $extension) {
            $extension->buildForm($builder, $options);
        }
    }
}

Maybe I'm expecting too much of the devs. But on the other hand, they would already need to be aware to call the parent method, so calling another method should not be that unexpected if he needs this extensibility for his type.

[alvarezmario]

@Tobion forcing the developer to do that is pretty ugly, I think there should be another solution.

[Tobion]

As I said, he is not forced to do that as it's his decision to make it extensible or not. I agree it's not perfect but doing it explicitely also has some advantages.

[webmozart]

I agree with @nomack84. If we provide an extension mechanism, we have to guarantee it works. Otherwise it's flawed.

[Tobion]

Then how about using events for this that are registered when adding an extension? So we don't need proxy classes, it's explicit and works consistently without any burden on the developer.

[schmittjoh]

Where would you put the event-dispatching code?

On Tue, May 15, 2012 at 1:29 PM, Tobias Schultze <
[email protected]

wrote:

Then how about using events for this that are registered when adding an
extension? So we don't need proxy classes, it's explicit and works
consistently without any burden on the developer.

---

Reply to this email directly or view it on GitHub:
https://github.com/symfony/symfony/issues/3879#issuecomment-5723463

[Tobion]

Good point, I close my case.

[jonathaningram]

FWIW, in my current app, I've got some types that use getParent and some that use PHP's extends as I did not know which was the preferred method and my original call on the user group did not have any response. So I'd at least vote for a big fat message in the docs (if it isn't there - maybe I missed it?) explaining what is the best practice. But at the end of the day, I think that both solutions are elegant enough as long as they are not fundamentally flawed and do not cause un-fixable bugs.

[stof]

using the PHP inheritance currently does not produce the expected result: it will not inherit the rendering of the parent type (as it will not be considered as a child of the type)

[jonathaningram]

@stof fair enough, but (luckily) in my case using extends has not resulted in any problems thus far (that I know of), however unfortunately I got in that position in the first place because native inheritance is more intuitive I guess.

[webmozart]

@stof As I said, we can use native inheritance if we force the user not to do the parent:: calls by himself. I updated the description above in this regard.

[AurelC2G]

I only see one issue here: in this setup, wouldn't it become impossible to easily overwrite the behaviour of the parent type? (= to avoid calling the parent's method and to replace it completely)

[stof]

@AurelC2G if you don't want to call the logic of the parent type, simply don't make your type a child of the other type.

[AurelC2G]

@stof Well some methods could still be common (inherited without any changes), and some other completely rewritten (buildView for example). I do not have any specific use case in mind, I am just trying to figure out whether this solution will hinder us later on.

[stof]

@AurelC2G if you change the logic, you will likely break the parent type as you would change the way the form is built (buildView depends of what has been done in buildForm and the form theme depends of what has been done in buildView)

[AurelC2G]

Well if you are positive about this, @bschussek's solution seems both clear and efficient to me. We will just have to emphasize in the docs that the parent methods must not be called.

[stof]

Using native inheritance but forbidding to call the parent method (and calling it in an hidden way) does not seem better than the current solution IMO. Especially as child types could then mess things in the parent type if they are modifying some protected properties somewhere

[stof]

and it would also break things if a child type does not overwrite all methods: calling the method on it would call the parent logic, which would then be applied twice (as it will be applied again when calling the parent type). This is due to the fact that omitting the method will call the parent one, which is exactly what breaks things in this proposal.

[webmozart]

@stof Not the latter. We need to do introspection for this anyway, so we can just as well avoid to call a method that is not defined on a specific class.

[stof]

Well, doing such introspection would not make the inheritance easier to understand than currently IMO (and maybe even more complicated as people will expect the PHP inheritance to work for calling the parent method). Btw, I see a big issue with the getName method here too (currently, if you don't define it, PHP will complain directly about a missing method of the interface, and most IDEs will warn you even before executing the code)

[webmozart]

Yes, introspection definitely is not straight-forward. Btw, we could also catch the case where getName() is not defined and throw an exception.

The question is whether the benefits mentioned above outweigh the downsides or not.

[stof]

I don't think we need instanceof so much. It cannot be checked in Twig templates anyway.

And for the understandable inheritance, I think that the magic applied using introspection and the restriction about the calls to parent methods would make it confusing and not so much understandable. So IMO, it is not worth breaking the BC here

[webmozart]

Ok. Then the remaining changes are:

• getParent: remove argument $options
• getDefaultOptions: rename to setDefaultOptions and pass options resolver
• buildViewBottomUp: rename to finishView (I hate the old name, better suggestions welcome)

What do you think?

[webmozart]

Btw, for the second and the third case we can keep BC. For the first we cannot.

[stof]

is there any need for the options in getParent now ? I think the merge of the field and form types removed this needs for all core types, right ?

for the other points, I think it is a good idea to clean things. The naming for buildViewBottomUp is weird and could be confusing for people.