Split from #2075095-14: Widget / Formatter methods signatures needlessly complex :
Entity forms are structured with field widgets nested under langcode keys:
$form['body']['fr'] = ...
$form['field_image']['und'] = ...
Those langcode subkeys are basically useless now, and do not determine in which langode the submitted values are stored: WidgetBase::extractFormValues() assigns values into the $items object that gets passed to it, how the form is structured doesn't change a thing.
- We should get rid of those langcode subkeys in the $form structure, we're not editing entities in multiple languages in the same form, core entity form controllers edit an entity in one single language. Within that language, some fields are translatable and some are not, that's it. It doesn't make a difference for the form structure.
- Even if some contrib module wanted to do "side editing in two languages in one form", it's then its responsibility to structure those forms accordingly.
At any rate, it's not the job of *widgets* to spit form $elements nested below a langcode subkey, as they do currently (see the end of WidgetBase::form()). Widgets generate "what it takes to edit this field", that's it. Multilingual structure, *if needed*, should be added on top of the result of $widget::form() by the calling code.
To which @plach replied:
Totally +1 on removing the langcode level in the form widget structures, it has always been a major source of troubles. I agree that for advanced use cases the implementing module should build its form(s) by assembling form widgets or possibily entire entity forms with their respective languages, as suggested in #1728816: Ensure multiple entity forms can be embedded into one form
Motivations:
Having the langcodes structure $form / $form_state when they are not needed:
- means chances of inconsistencies and weird behavior
- makes it vastly painful to change the language assignment logic if we need to (see #2076445: Make sure language codes for original field values always match entity language regardless of field translatability). Language assignment is internal, it should stay internal.
Patch summary:
- Renames $form[$field_name][$langcode] to $form[$field_name]['widget'] in entity forms.
(see #10 item 2 for why we still need a separate sub-element below $form[$field_name])
- Removes the $langcode param in field_form_(get|set)_state(). Those functions are used internally to structure the processing data that each field places into $form_state and avoid clashes. Just like for $form, we don't need $langcode to be part of the default structure of $form_state. The code that builds the form can provide the additional structure it needs, if it needs one.
API changes:
- Change of structure in entity $forms:
code doing form_alter() on those needs to be updated (see forum_form_node_form_alter())
tests doing drupalPost() / drupalPostForm() with field values on entity forms need to be updated
- field_form_(get|set)_state() lose their $langcode param.
Comments
Comment #1
yched CreditAttribution: yched commentedAlright. Let's see what breaks :-)
Comment #3
yched CreditAttribution: yched commentedMost. Boring. Patch. Ever.
No interdiff, don't bother reviewing until this is green :-)
Comment #4
yched CreditAttribution: yched commentedSide note: the mere amount of tests that had to deal with that, and needed to "use Drupal\Core\Language\Language" just to get a Language::LANGCODE_NOT_SPECIFIED to put in their drupalPost() data, shows it's a huge win...
Comment #6
yched CreditAttribution: yched commentedMore fixes...
Comment #8
yched CreditAttribution: yched commentedComment #9
plachAwesome cleanup! We should now be able to get rid of the
EntityFormController::submitEntityLanguage()
method too.Does this need to be updated?
Why are we replacing the langcode level with a static wrapper?
Surplus blank line?
Missing (optional) + description of the default value.
Moreover, given that the
$langcode
argument is being passed toEntityInterface::getTranslation()
, it would more appropriate to default toLanguage::LANGCODE_DEFAULT
.Comment #10
yched CreditAttribution: yched commented@plach: Thanks for the review :-)
1. Right, fixed
2. Look at the structure of what WidgetBase::form() returns:
$form[$field_name] is a '#type' => 'container'. So we cannot put the widget (e.g. '#type' => 'select') directly at that level, we need a separate wrapper. The langcode level used to do that - we don't need it to be the langcode, and we don't need it to be reflected in the structure of the submitted values, but we still need a separate wrapper level in the $form.
3. Fixed.
4. Doc fixed. All the code that currently calls assertFieldValues() in HEAD (where the argument is not optional) passes Language::LANGCODE_NOT_SPECIFIED - so I kept that ?
Note that I'm not fully done yet, field_form_[set|get]_state() needs its $langcode param removed too.
I'll try to provide a 'do-not-test' patch with the non-test changes only (those are just a few KBs) for easier review.
Comment #11
plachWonderful :)
Please remember to remove
EntityFormController::submitEntityLanguage()
: its sole purpose was updating form state field language codes after an entity language change.Comment #12
yched CreditAttribution: yched commented[edit: crosspost with #11 :-)]
Missed the part about submitEntityLanguage() - yes it does look like it's stale now, let's see what happens without it.
Comment #13
yched CreditAttribution: yched commentedUpdated patch:
- renamed $form[$field_name]['wrapper'] to $form[$field_name]['widget'], for clarity - the "wrapper" is $form[$field_name] itself.
- removed the $langcode argument in field_form_(get|set)_state()
See interdiff.
As mentioned in #10, attached are:
- a 'REVIEW-do-not-test' patch with just the actual code changes for review
- a 'FULL' patch with the test updates (90% of the patch size...)
Comment #14
yched CreditAttribution: yched commentedUpdated the OP with patch summary & API change sections.
Adding tags, and raising to major: having the langcodes structure $form / $form_state when they are not needed
- means chances of inconsistencies and weird behavior
- makes it vastly painful to change the language assignment logic if needed (see #2076445: Make sure language codes for original field values always match entity language regardless of field translatability). Language assignment is internal, it should stay internal.
Comment #14.0
yched CreditAttribution: yched commentedsummary & API changes
Comment #14.1
yched CreditAttribution: yched commentedmotivations
Comment #15
yched CreditAttribution: yched commentedReroll after #2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests (massively conflicted in tests)
Comment #16
plachThis makes me feel so good :D
RTBC if green, otherwise the following nits could be fixed:
These could use array type-hints as the PHP docs.
Comment #16.0
plachfix formatting
Comment #16.1
yched CreditAttribution: yched commentedgrammar
Comment #16.2
yched CreditAttribution: yched commentedreword
Comment #16.3
yched CreditAttribution: yched commenteddrupalPostForm()
Comment #18
plachJust a typo in the Forum test. Also fixed #16.
Comment #19
webchickOMG. I approve this sooooo effing hard.
Lovely diffstat!
LOVELY code sample!!
OMG, this is seriously one of my favourite patches of all time!! :D :D
Committed and pushed to 8.x. YEAAAAHH!!!
Will need a lovely, lovely change notice. :D
Comment #20
yched CreditAttribution: yched commentedThanks @webchick, glad that you like it so much ;-)
Created two separate change notices:
https://drupal.org/node/2088511
https://drupal.org/node/2088515
Comment #21
plach@yched++
Change notices look good to me. I just performed a very minor adjustment to https://drupal.org/node/2088511.
Comment #22
andypostThe change caused the content translation totally broken #1946462-52: Convert content_translation_translatable_form() to the new form interface
Comment #23.0
(not verified) CreditAttribution: commentedAPI change