Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The main premise of entity forms is that we get to work with an entity object at all times instead of checking submitted values from the form state. However, this premise is currently broken in the main form()
method when the form is rebuilt via AJAX.
Proposed resolution
Add an #after_build callback that updates the entity object with user-submitted values from the form state.
Remaining tasks
Review the patch.
User interface changes
Nope.
API changes
Nope.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 2.21 KB | amateescu |
#22 | 2448357-22.patch | 12.21 KB | amateescu |
#18 | interdiff.txt | 3.31 KB | amateescu |
#18 | 2448357-18.patch | 12.08 KB | amateescu |
#15 | interdiff.txt | 1.87 KB | amateescu |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedHere's the patch.
Comment #4
amateescu CreditAttribution: amateescu commentedThis fixes the test-only patch so it shows only the relevant failures. Now let's see why I broke all of core :/
Comment #6
amateescu CreditAttribution: amateescu commentedI seem to be finding all kinds of subtle broken things this weekend.
Test-only patch from #4 is still accurate for review.
Comment #8
amateescu CreditAttribution: amateescu commentedIt seems that content entity forms really need the current flow, probably because field api has its own #after_build callback for widgets, so let's make it specific to them. This allows us to fix config entity forms which use only raw Form API code.
Comment #10
amateescu CreditAttribution: amateescu commentedI need to stop thinking that I solved all the problems in the world every time I do small change accompanied by a victorious comment :)
Let's just fix what this issue was about: we need to be able to access updated $entity values in the
form()
method when it is rebuilt after an AJAX submit.No interdiff because I'm just reverting the crap from #6 and #8.
Comment #13
amateescu CreditAttribution: amateescu commented#sigh
Comment #15
amateescu CreditAttribution: amateescu commentedCKEditor was relying on the
id()
method to return NULL for config entities that are not yet saved, but that assumption is not correct.Comment #16
Wim LeersComment #17
Wim LeersGreat patch! Mostly nits:
Why? Can't some widget also be doing AJAXy stuff?
s/process/#process/
s/after_build/#after_build/
Oh my!
#after_build
! Been a long time since we saw that! :DWoah, nice catch! This should also update
FormBuilderTest
to prevent this regression in the future.array()
->[]
s/non-js/non-JS/
s/js-hidden/'js-hidden'/
This makes it clear that in the first case, you're describing a situation/context, and in the second, you're describing an element class.
ConfigTest::load()
s/non-js/non-JS/
Yay, better :) Thanks!
Comment #18
amateescu CreditAttribution: amateescu commentedThanks for the review :)
ConfigEntityTest
throughConfigTestForm
.Comment #19
Wim Leers1. Ah! That really helps — thanks :)
4. Hm, fair — that's probably okay.
7. I wasn't sure, but I suspected that that was the reason why. I ran into that too a long time ago. Fine then :)
Comment #20
yched CreditAttribution: yched commentedPatch looks great, just clarification nitpicks :
Looks a bit surprising when seen on its own. Would help seeing the big picture if the comment made it clearer that we're undoing part of what the parent did. Also maybe by removing the empty line and grouping the code more ?
Comments could be enhanced a bit IMO. Proposals below are what I feel would have helped me grasp the logic faster :-)
- Phpdoc : "This is the entity object builder function that allows..." : slightly confusing, an #after_build callback is not an entity builder function.
Proposal :
"Updates the internal $this->entity object with submitted values when the form is being rebuilt (e.g. submitted via AJAX), so that subsequent processing (e.g. AJAX callbacks) can rely on it." ?
- Inner code comment : the logic around "detect if we're doing the initial build or a rebuild" could be more explicit.
Proposal : "Rebuild the entity If #after_build" is being called as part of a form rebuild, i.e. if we are processing input.".
Comment #21
alexpottNeeds work for #20
Comment #22
amateescu CreditAttribution: amateescu commentedUpdated the comments according to the suggestions from #20, I hope it's ok to put it back to RTBC after these documentation changes :)
Comment #23
yched CreditAttribution: yched commentedThanks ! RTBC +1
Comment #24
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 1c72fda and pushed to 8.0.x. Thanks!
Comment #27
yched CreditAttribution: yched commentedTrying to simplify the Ajax flow in Field UI "Manage Display" forms in #2497847: Simplify EntityDisplayEditFormBase ajax rebuild flow to work only with $this->entity, it does seem we call buildEntity() quite a lot.
Opened #2497981: EntityForms call buildEntity() a lot on AJAX calls to see what happens if we only rebuild when submitting values.