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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
32.21 KB

Alright. Let's see what breaks :-)

Status: Needs review » Needs work

The last submitted patch, field_form_langcode-2083811-1.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
158.88 KB

Most. Boring. Patch. Ever.
No interdiff, don't bother reviewing until this is green :-)

yched’s picture

Side 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...

Status: Needs review » Needs work

The last submitted patch, field_form_langcode-2083811-3.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
208.17 KB

More fixes...

Status: Needs review » Needs work

The last submitted patch, field_form_langcode-2083811-6.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
216.98 KB
plach’s picture

Status: Needs review » Needs work

Awesome cleanup! We should now be able to get rid of the EntityFormController::submitEntityLanguage() method too.

  1. +++ b/core/modules/field/field.deprecated.inc
    @@ -488,10 +488,9 @@ function field_read_instances($conditions = array(), $include_additional = array
      *   // The '#language' key holds the same value of $langcode and it is used
      *   // to access the field sub-array when $langcode is unknown.
    

    Does this need to be updated?

  2. +++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.php
    @@ -130,17 +112,31 @@ public function form(EntityInterface $entity, $langcode, FieldInterface $items,
    +        'wrapper' => $elements,
    

    Why are we replacing the langcode level with a static wrapper?

  3. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldTestBase.php
    @@ -8,8 +8,10 @@
    +
    

    Surplus blank line?

  4. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldUnitTestBase.php
    @@ -138,14 +139,14 @@ function _generateTestFieldValues($cardinality) {
    + * @param $langcode
    + * The language code for the values.
    * @param $column
    * (Optional) the name of the column to check.
    */
    - function assertFieldValues(EntityInterface $entity, $field_name, $langcode, $expected_values, $column = 'value') {
    + function assertFieldValues(EntityInterface $entity, $field_name, $expected_values, $langcode = Language::LANGCODE_NOT_SPECIFIED, $column = 'value') {
    

    Missing (optional) + description of the default value.

    Moreover, given that the $langcode argument is being passed to EntityInterface::getTranslation(), it would more appropriate to default to Language::LANGCODE_DEFAULT.

yched’s picture

Status: Needs work » Needs review
FileSize
3.03 KB
217.58 KB

@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.

plach’s picture

I'll try to provide a 'do-not-test' patch with the non-test changes only (those are only a few KBs) for easier review.

Wonderful :)

Please remember to remove EntityFormController::submitEntityLanguage(): its sole purpose was updating form state field language codes after an entity language change.

yched’s picture

[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.

yched’s picture

Updated 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...)

yched’s picture

Priority: Normal » Major
Issue tags: +API change, +Field API

Updated 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.

yched’s picture

Issue summary: View changes

summary & API changes

yched’s picture

Issue summary: View changes

motivations

plach’s picture

Status: Needs review » Reviewed & tested by the community

This makes me feel so good :D

RTBC if green, otherwise the following nits could be fixed:

+++ b/core/modules/field/field.form.inc
@@ -209,8 +205,8 @@ function field_add_more_js($form, $form_state) {
+function field_form_get_state($parents, $field_name, &$form_state) {

@@ -231,8 +225,8 @@ function field_form_get_state($parents, $field_name, $langcode, &$form_state) {
+function field_form_set_state($parents, $field_name, &$form_state, $field_state) {

@@ -243,30 +237,14 @@ function field_form_set_state($parents, $field_name, $langcode, &$form_state, $f
+function _field_form_state_parents($parents, $field_name) {

These could use array type-hints as the PHP docs.

plach’s picture

Issue summary: View changes

fix formatting

yched’s picture

Issue summary: View changes

grammar

yched’s picture

Issue summary: View changes

reword

yched’s picture

Issue summary: View changes

drupalPostForm()

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field_form_langcode-2083811-15-FULL.patch, failed testing.

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
234.42 KB
3.56 KB

Just a typo in the Forum test. Also fixed #16.

webchick’s picture

Title: Remove langcode nesting for fields in entity $form structures » Change notice: Remove langcode nesting for fields in entity $form structures
Status: Reviewed & tested by the community » Active
Issue tags: +DX (Developer Experience), +Needs change record, +Approved API change

OMG. I approve this sooooo effing hard.

101 files changed, 598 insertions, 903 deletions.

Lovely diffstat!

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockCreationTest.php
@@ -49,9 +48,8 @@ protected function setUp() {
     // Create a block.
     $edit = array();
-    $langcode = Language::LANGCODE_NOT_SPECIFIED;
     $edit['info'] = $this->randomName(8);
-    $edit["block_body[$langcode][0][value]"] = $this->randomName(16);
+    $edit['block_body[0][value]'] = $this->randomName(16);
     $this->drupalPostForm('block/add/basic', $edit, t('Save'));

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

yched’s picture

Title: Change notice: Remove langcode nesting for fields in entity $form structures » Remove langcode nesting for fields in entity $form structures
Status: Active » Fixed
Issue tags: -Needs change record

Thanks @webchick, glad that you like it so much ;-)

Created two separate change notices:
https://drupal.org/node/2088511
https://drupal.org/node/2088515

plach’s picture

@yched++

Change notices look good to me. I just performed a very minor adjustment to https://drupal.org/node/2088511.

andypost’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

API change