Serialization of forms is a problem for #1863816: Allow plugins to have services injected so here's a patch that shifts entities (which are plugins) out of the recursive form array into the form_state arrays so that in a later step we can decide on how not to cache them (hardwire, move to form_state['no cache']['entity'], introduce form_state['no cache keys']). Small moves.
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | 1955874-26.patch | 3.56 KB | alexandre.todorov |
| #23 | 1955874-23.patch | 2.62 KB | alexandre.todorov |
| #20 | drupal-better-form-serialization-1955874-20.patch | 2.84 KB | mfernea |
| #16 | drupal-better-form-serialization-1955874-16.patch | 963 bytes | mfernea |
| #12 | drupal-better-form-serialization-1955874-12.patch | 954 bytes | mfernea |
Comments
Comment #1
yched commentedI do share the worries about entity serialization, but entites are not plugins currently, right ?
Comment #2
yched commentedAlso - field.module organizes its use of $form_state so that it can support fields on different entities in different parts of the form, so we can't put $entity directly at the top-level of $form_state.
This needs to go through field_form_get_state() / field_form_set_state().
Comment #4
chx commentedAlso #node
Comment #5
dawehnerLet's run the testbot.
Comment #7
berdirThose forms moved to classes, but it still seems like a good thing to do.
Comment #8
berdirMight be a good novice issue.
Comment #9
chx commentedComment #11
mfernea commentedI'll have a look at this.
Comment #12
mfernea commentedHere is the rerolled patch.
Comment #14
mfernea commentedProbably I should have used:
$form_state->setValue('entity', $node);
and
drupal_set_message($this->t('Updated book %title.', array('%title' => $form_state->getValue('entity')->label())));
I will try to test to make sure this is correct.
Comment #15
berdirYes, you can no longer use $form_state as an array. Instead, use $form_state->set()/get(). getValue()/setValue() is for form values.
Comment #16
mfernea commentedHere is the new patch. I'm using set() and get().
Comment #19
berdirLooks good!
As the title actually says #entity, I checked for that too and found one more instance in FieldEditForm.php, let's update that in a similar way.
Comment #20
mfernea commentedHere is the updated patch.
Comment #21
berdirLooks good to me, thanks.
Comment #22
alexpottshould we also be fixing #term in the OverviewTerms form? - This looks a bit more complex this it appears the terms already are in form state and the form is extended by Forum's Overview form.
Comment #23
alexandre.todorov commented$form_state->getValue('terms') is coming from validation of $form['terms'][$key] elements. So as suggested above I added $form_state set and get of 'terms'. Forum's Overview form also updated accordingly.
Comment #24
yched commented@alexandre.todorov : your patch #23 does not seem to include the previous patch #20 ?
Comment #25
alexandre.todorov commentedComment #26
alexandre.todorov commentedUpdated #23 (taking into account rerolled #20)
Comment #27
ajitsLooks like the 'novice' tasks was added in before the beta release. After checking the issue, I think this could mean changing some part of the API.
I am removing the tag as per https://www.drupal.org/core-mentoring/novice-tasks
Comment #39
smustgrave commentedIf still a valid task what's currently needed for 10.1?
Comment #41
smustgrave commentedJust following up if still valid? If no follow up could close in 3 months
Comment #42
berdirUnsure. form serialization has changed, we now basically only start to persist them on the first ajax request. So I think that basically makes this a non-issue unless those forms use ajax.
Lets just close this unless someone has proof that this results in a measurable improvement?