-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[RFC] [Form] Add Partial Bind Support #5576
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
if (!$child->isValid()) { | ||
if (!$this->config->getPartial() && !$child->isValid()) { | ||
return false; | ||
} elseif ($this->config->getPartial() && $child->isBound() && !$child->isValid()) { |
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.
use if
, not elseif
as the previous if
returns
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.
btw, I don't think this makes much sense: the validator would still validate the object graph so it could add errors on these children. So it should be taken into account for isValid
too.
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 know, I wasn't sure how to deal with that. Personally, I kind of want the child to still be validated even if the parent skips binding. It's not the child's fault data hasn't been bound. This way, $child->isValid()
would throw You cannot call isValid() on a form that is not bound.
.
So should I make it so the graph walker skips this child? This wouldn't make the child valid, but at least it wouldn't have any errors.
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.
no, we should not make the GraphWalker skip this child. It does not make any sense to forbid validating some values because the form has not changed them (what if you have a constraint validating that a property must be bigger than another one and only one of them is bound ?)
IMO, a better solution would be to bind all child forms, but using the existing data instead of null
when the field is not submitted an partial is allowed.
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.
Just to be clear, you're saying to use the existing model's data when there is missing submitted data?
Let's say the existing model has an Author
. The form will have something like <option name="form[author]" value="1" selected />
. If we did a partial bind and omitted &author=1
, we would have to do the equivalent of:
$author = $this->config->getData()->getAuthor();
$authorId = doSomeTransformOnTheEntity($author);
$child->bind($authorId);
In short, reproduce the setData
part as if the user actually provided the transformed, submitted data?
+1 for solution 1, I used this same concept in my own Form extension. I think the semantic 'partial' might be a bit confusing for people first coming to the Form component. Something like 'allow_partial_update', or 'allow_partial_bind' might be better IMO. |
You're right. @bschussek Recommended @housebolt Because Stof suggested that I still bind with existing model data, |
I do have one use case where I have a field that is an embedded form that needs full binding (if it exists in the submitted data array) while the rest of the form fields can be partially bound. Is there a way that ALLOW_MISSING or IGNORE_MISSING can handle this case? Or is there a better way to work around that on my end? |
@housebolt Can you describe this a bit more for me? I'd like to build a test around the scenario, but I don't fully comprehend it. Perhaps you can give an example of how the form is setup (via the types) and what submitted data you expect & how it behaves. Thanks! |
I think I figured out a better way to handle it on the controller or handler side, if you do something like this: $form; // FormType
$model; // Model
if ($this->get('request')->request->has('child')) {
$model->setChild(new Child());
}
$form->setData($model);
$form->bind($this->request); Then the form's view data won't contain the child data and when it's merged it won't validate if it's a partial bind. After some thought on the subject, my particular use case is pretty fringe. I tried coming up with a generic use case that could generally apply but couldn't. |
@ericclemmons the |
Looking at this code, this is exactly what I wanted to do, but I wanted to bind it to each field. Binding it to the form type does make sense if you are assuming that an form submission matches the entity directly. However, for additional flexibility, one could allow a 'incumbent' value that gets used if the entity property does not match a form submission. For example: <?php
class CampaignJobType extends AbstractType
{
public function buildForm(FormBuilderInterface $builder, array $options)
{
$builder
->add('name', 'text', array(
'incumbent' => $options['data']->getName()
))
;
} This allows a user to choose if they want to use a value from an old entity (Found in $options['data']) or another value if they choose when the form fails to submit an entity property. This allows for more flexibility, especially with REST APIs. Furthermore, it will make you compliant with RFC 5789 (http://tools.ietf.org/html/rfc5789) which, in its current state, the Forms are not. |
@ericclemmons can you update your work to inherit the partial flag from the parent form by default ? |
@stof Sure, I almost had that working, until I spoke with @bschussek about difficulties I was having. So, I moved some of the logic into |
@@ -448,6 +453,14 @@ public function getCompound() | |||
/** | |||
* {@inheritdoc} | |||
*/ | |||
public function getIgnoreMissing() |
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.
The getter needs to be added in the interface too
@ericclemmons please squash your commits to remove the reverted commits from the history. |
Sonofa. Rebased too much :) |
Defaults to "false". Bound data should bind all children.
All cleaned up now. RFC... |
@fabpot Use case: PATCH requests to a RESTful API |
|
||
$this->assertEquals('app', $form->getData()); | ||
$this->assertEquals('norm', $form->getNormData()); | ||
$this->assertEquals('client', $form->getClientData()); |
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.
getClientData
is deprecated in favor of getViewData
@ericclemmons Thank you for the tremendous work you are doing on this so far. I like the direction that this is heading. Some remarks:
At last, we had the two alternatives of specifying "allow_missing" as form option vs. as parameter to the I would appreciate a listing of pros and cons of specifying the setting as an option vs. as parameter to Otherwise, great progress 👍 |
Consequently we probably also need to adapt |
The name |
I'm very sorry for the delay on this. After over a month of considering this, we're aware of the following :
With these in mind, I would like to revert f14d753 (where I named the option
The option should not describe how it works (which We use I think it's time we nail down a decision & move on, especially since I have extra time over the Christmas break :) |
+1 for
|
The option as passed as the third argument to the form builder right? So we can set it per form and not only globally? |
@alex88 it can be set for each form (not only the root of the form tree) and it is inherited from the parent by default |
@salmanpk I was curious about the template confusion as well, but Symfony doesn't really have a I do recommend (per my 3rd bullet up above) that we not use I'll give it another day for comments to roll in, then continue with this feature & other updates... |
@ericclemmons I just became aware of a major problem with the implementation as option. We currently use the sugar $form->bind($request); to hide away the extraction of request data from a form. What happens if the logic behind this sugar discovers that the request is a PATCH request? It can't change the So the only real solution seems to be your original proposal with an optional argument to public function bind($submittedData, $partial = false); |
I was just thinking that we'd not lock that option and allow it to be modified by the listener. Otherwise, we would have to modify the method signature and I agree, it's not pretty =/ |
Was this a convoluted way of saying "I agree"? :) I'm sorry, I don't understand what you just said.. |
It was my convoluted way of saying "what about allowing modifications to the Is that wrong? |
Yes, this is wrong. Changing the |
In that case, it looks like we have an architectural limitation with changing a form option after a form has been built: Maybe we have two options:
1 fits in better with the existing architecture, while 2 would allow for all-in-one form creation + binding... $form = $factory->create('user', $user, array('request' => $request));
if ($form->isValid()) {
...
} |
Option 2 won't work for those of us who use DIC to create the forms... |
Yikes, you're right. Looks like there's a lot of bending over backwards to make something like |
Why can't we add an argument to |
So with the 2nd ambiguous argument on |
No |
Ok, so the ability to force complete binding would be removed per @stof's suggestion. |
Why bindPartially() cannot be used? |
@alex88 "I don't like an extra method in the interface" - @bschussek Even with the extra interface, internally $form = $factory->create('user', $user);
...
$form->bind($request, $request->getMethod() === 'PATCH')); |
@ericclemmons oh sorry missed that |
I'd also vote for additional parameter to bind |
I'll update this branch and remove all the sexiness from it then. Kind of a shame, since this is what I figured we would do back in #1341 days... |
For those that are looking forward for this PR for a seamless Backbone.js integration, I thought I'd chime in to let you know that it is now officially supported as of v0.9.9 (released a few days ago) by passing |
@bschussek Man, I was thinking about this more and I just don't feel right about it as a parameter for Just for comparison... 1 - As an Option (PR): $form = $factory->create('user', $user, array('partial' => $request->getMethod() === 'PATCH'));
$form->bind($request); 2 - As a $form = $factory->create('user', $user);
$form->bind($request, $request->getMethod() === 'PATCH'); They look pretty similar, no? The difference I envisioned was in the ability for other types & extensions to explicitly set/override If a parameter on |
@ericclemmons First of all, I'm sorry for the lost work. It just was not clear from the beginning that an option would not be feasible. Second, don't worry about |
The problem with your comparison of points 1. and 2. is that in both cases you have to check for the PATCH method yourself. My goal is a usage like this (again, see #5493): $form = $this->createForm('my_type');
if ($form->process($request)->isValid()) {
// save
} This is not possible with "bind_partial" as option. |
Well, crap. I was making this work for the current version of the form component, not with #5493 in mind, since that drastically changes how this sort of functionality would work. |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1341
Todo: [list of todos pending]
License of the code: MIT
Documentation PR: Not yet
After much discussion in #1341, I've added the ability for
FormType
to support binding partial data viaignore_missing
when client data is not set:false
, the current behavior of bindingnull
onto the form takes placetrue
, the original model data (which is transformed into view data) is bound to the form (as if the user chose the existing value)null
(or not explicitly set), the parent form'signore_missing
value is used, and then follows option 1 or 2.I'm still considering a way to turn on
ignore_missing
when calling$form->bind($request)
(and the request isPATCH
), but this can be done when creating the form: