-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Form] RFC: Replace dynamic by proper type inheritance #3879
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
Comments
👍 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? |
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). |
@bschussek I see 2 potential issues here:
|
@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. |
I found this less intuitive ;) The effort should be on documentation and clean API. |
You can never prevent that, but one might expect from a decent developer to be smart enough to call a parent method right? |
@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. |
@stof well you definitely are the authority on that matter 😄 |
@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 think using php's inheritance instead of artificial one is better, because it seems equally complicated to understand for mid-level users. |
+1 |
Will it be the last change on this component?
Does it mean Symfony2 will support the old API forever? |
@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 |
@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
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 |
Thanks, @bschussek |
Commits ------- 6e4ed9e [Form] Fixed regression: bind(null) was not converted to an empty string anymore fcb2227 [Form] Deprecated FieldType, which has been merged into FormType bfa7ef2 [Form] Removed obsolete exceptions 2a49449 [Form] Simplified CSRF mechanism and removed "csrf" type Discussion ---------- [Form] Merged FieldType into FormType Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: #3878 Todo: update the documentation on theming  This PR is a preparatory PR for #3879. See also #3878. --------------------------------------------------------------------------- by juliendidier at 2012-04-13T14:25:19Z What's the benefit ? --------------------------------------------------------------------------- by henrikbjorn at 2012-04-13T14:26:40Z why `input_widget` ? and not just `widget` --------------------------------------------------------------------------- by Burgov at 2012-04-13T14:27:49Z @juliendidier dynamic inheritance is now obsolete which fixes some other issues --------------------------------------------------------------------------- by stloyd at 2012-04-13T14:37:26Z What about __not__ breaking API so *badly* and leaving `FieldType` which will be simple like (with marking as deprecated): ```php <?php class FieldType extends AbstractType { public function getParent(array $options) { return 'form'; } public function getName() { return 'field'; } } --------------------------------------------------------------------------- by bschussek at 2012-04-13T14:43:41Z @stloyd That's a very good idea. --------------------------------------------------------------------------- by mvrhov at 2012-04-13T17:41:21Z IMHO what @stloyd proposed sounds like a good idea, but removing FieldType class, if #3903 will come into life might ensure that more forms will broke and people will check them thoroughly. --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T18:46:08Z @bschussek looks great, but I'm concerned about how quickly will the third-party bundles adapt to this BC break. I hope really quick, because if they don't the whole stuff will be useless :S of course it's not your problem to solve. --------------------------------------------------------------------------- by stof at 2012-04-13T18:50:32Z @r1pp3rj4ck there is already another BC break requiring to update custom types for Symfony master. So third party bundles already have to do some work. --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T18:59:37Z @stof which one? I've looked into @bschussek 's RFC about these [foo].bar stuff, but it's not yet implemented. Are you refering to this or another one I've missed? --------------------------------------------------------------------------- by stof at 2012-04-13T19:04:06Z @r1pp3rj4ck the change regarding default options --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T19:06:10Z @stof oh, I forgot that one. Weird thing is that I've already changed my default options today and still forgetting these stuff :D --------------------------------------------------------------------------- by bschussek at 2012-04-14T08:58:29Z I restored and deprecated FieldType now. I'd appreciate further reviews. --------------------------------------------------------------------------- by stloyd at 2012-04-14T09:02:32Z Maybe we should try to avoid this BC in templates ? What do you think about similar move like with `FieldType` ? (hold old, but inside just render new) --------------------------------------------------------------------------- by bschussek at 2012-04-14T09:07:22Z @stloyd You mean for those cases where people explicitely render the block "field_*"? We can do that. --------------------------------------------------------------------------- by stloyd at 2012-04-14T09:09:45Z @bschussek Yes I mean this case =) Sorry for not being explicit, I need some coffee I think =) --------------------------------------------------------------------------- by bschussek at 2012-04-17T14:45:35Z I added the field_* blocks again for BC. Could someone please review again? Otherwise this can be merged. --------------------------------------------------------------------------- by Burgov at 2012-04-17T15:11:16Z @bschussek I'm not sure what has changed to cause this, but if I try out your branch on our forms, if I leave the value of an input empty, eventually the reverseTransform method receives a null value, rather than a '' (empty string) value, as on the current symfony master. DateTimeToLocalizedStringTransformer, for example, will throw an Exception if the value is not a string ```php if (!is_string($value)) { throw new UnexpectedTypeException($value, 'string'); } ``` Other than that, all forms render just the same as they do on symfony master --------------------------------------------------------------------------- by bschussek at 2012-04-17T15:30:29Z @Burgov Fixed.
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.:
with native inheritance results in (due to the parent::buildForm() calls at the very beginning)
but should result in the execution chain
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? |
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. |
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) |
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. |
@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
|
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. |
@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.
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. |
@bschussek you're right. Why not include this logic explicisely in each type:
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.
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. |
@Tobion forcing the developer to do that is pretty ugly, I think there should be another solution. |
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. |
I agree with @Nomack84. If we provide an extension mechanism, we have to guarantee it works. Otherwise it's flawed. |
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. |
Where would you put the event-dispatching code? On Tue, May 15, 2012 at 1:29 PM, Tobias Schultze <
|
Good point, I close my case. |
FWIW, in my current app, I've got some types that use |
using F438 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) |
@stof fair enough, but (luckily) in my case using |
@stof As I said, we can use native inheritance if we force the user not to do the |
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) |
@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. |
@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. |
@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) |
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. |
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 |
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. |
@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. |
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 |
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. |
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 |
Ok. Then the remaining changes are:
What do you think? |
Btw, for the second and the third case we can keep BC. For the first we cannot. |
is there any need for the options in for the other points, I think it is a good idea to clean things. The naming for |
Commits ------- bc15e2d [Form] Some code cleanup 94f6f77 Restructured Form section of UPGRADE 3d800af [Form] Remove usages of deprecated features ee803cd [Form] Renamed setVars() to addVars() in FormViewInterface 1c4f632 [Form] Fixed API docs and usage of FormBuilderInterface instead of FormBuilder 2e6cdd1 [Form] Inverted the logic of "single_control" and renamed it to "compound". The opposite is now "simple". 98a7c0c [Form] Consolidated FormInterface, FormBuilderInterface and FormViewInterface 8e128fc [Form][OptionsResolver] Fixed typos 877d8f7 [Form] Reversed the order of $type and $name in FormFactory::createNamed[Builder]() 33fecca [Form] Merged various form events and added class FormEvent bec8015 [Form] Renamed client and application format to view and model format 8cae328 [Form] setDefaultOptions() is now coded against OptionsResolverInterface 1ecddbc [OptionsResolver] Renamed recommended method to setDefaultOptions() dc2fa9d [OptionsResolver] Added OptionsResolverInterface 2cd99e8 [Form] Added FormBuilderInterface and FormViewInterface and cleaned up FormTypeInterface and FormTypeExtensionInterface 0ef4066 [Form] Options are now passed to buildView() and buildViewBottomUp() 027259e [Form] Changed getDefaultOptions() to setDefaultOptions(OptionsResolver $resolver) in FormTypeInterface b4e8bcf [OptionsResolver] Relaxed tests to check that allowed values can also be passed as scalars 97de004 [OptionsResolver] Added option type validation capabilities 0af5f06 [OptionsResolver] Added method setFilters() for augmenting the final option values Discussion ---------- [Form] Cleaned up the Form API Bug fix: no Feature addition: no Backwards compatibility break: **YES** Symfony2 tests pass: yes Fixes the following tickets: #3855, #3879, #4342, #4371, #4375 Todo: - This PR cleans up the Form API as described in the referenced tickets in order to stabilize and freeze this API in the future. BC is kept wherever possible. After this PR, form types are expected to follow the following interface: ```php <?php class MyType extends AbstractType { public function buildForm(FormBuilderInterface $builder, array $options) { } public function buildView(FormViewInterface $view, FormInterface $form, array $options) { } public function finishView(FormViewInterface $view, FormInterface $form, array $options) { } public function setDefaultOptions(OptionsResolverInterface $resolver) { } public function getParent() { return 'form'; } public function getName() { return 'my_type'; } } ``` Note that the options are now passed to `buildView` and `finishView` (formerly `buildViewBottomUp`) as well, reducing the need for creating form attributes in most cases. --------------------------------------------------------------------------- by travisbot at 2012-05-23T19:07:44Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414486) (merged 277f5f78 into e023807). --------------------------------------------------------------------------- by bschussek at 2012-05-23T19:51:40Z The PR now also contains the fix for #4342. --------------------------------------------------------------------------- by travisbot at 2012-05-23T19:51:55Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414932) (merged 13d284ba into e023807). --------------------------------------------------------------------------- by travisbot at 2012-05-24T06:55:35Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419776) (merged 5aba0778 into e023807). --------------------------------------------------------------------------- by travisbot at 2012-05-24T06:56:28Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419783) (merged 00c4f7e into b07fb3c). --------------------------------------------------------------------------- by travisbot at 2012-05-24T12:26:25Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1421748) (merged 73cd9f8 into b07fb3c). --------------------------------------------------------------------------- by bschussek at 2012-05-24T12:27:32Z The FormView changes described in #4371 are now contained as well. Invitation for final review. --------------------------------------------------------------------------- by travisbot at 2012-05-24T14:03:10Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1422116) (merged e1f502b into b07fb3c). --------------------------------------------------------------------------- by cordoval at 2012-05-25T03:26:05Z any ETA @bschussek ? I want to use it for tackling the problem of dynamic selects city-state here http://www.craftitonline.com/2011/08/symfony2-ajax-form-republish/ I am told: "getDefaultOptions is changed to setDefaultOptions which will allow you to set depenedent values based on other forms" so I am most interested +300! --------------------------------------------------------------------------- by bschussek at 2012-05-25T06:08:53Z @cordoval I think you misunderstood this. The OptionsResolver allows you to set options dependent on other options, but of the same field. Also, this is already possible before this merge by setting an option to a closure as described in the README of the OptionsResolver component. --------------------------------------------------------------------------- by travisbot at 2012-05-25T06:35:53Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1430534) (merged b61cc55 into b07fb3c). --------------------------------------------------------------------------- by vicb at 2012-05-25T06:42:24Z @bschussek great changes ! I have just sent you a PR with some modifs related to deprecated features. I'll rebase and submit the other one we have already discussed. --------------------------------------------------------------------------- by travisbot at 2012-05-25T07:16:18Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1430699) (merged e18830d into b07fb3c). --------------------------------------------------------------------------- by cordoval at 2012-05-25T07:19:07Z @bschussek what is already possilble @ "Also, this is already possible before" I am confused could you link to what you are referring to please? --------------------------------------------------------------------------- by travisbot at 2012-05-25T07:22:07Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1430727) (merged 20c02a7 into b07fb3c). --------------------------------------------------------------------------- by bschussek at 2012-05-25T07:22:29Z ``` public function getDefaultOptions() { return array( 'a' => 'foo', 'b' => function (Options $options) { return 'bar' === $options['a'] ? 'x' : 'y'; } ); } --------------------------------------------------------------------------- by travisbot at 2012-05-25T10:38:04Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1431903) (merged bc15e2d into 45849ce).
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:
API after:
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:
$form->getType() instanceof MyType
Negative effects:
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.The text was updated successfully, but these errors were encountered: