Simplify/cleanup language handling in EntityFormController

More tags

Here is a first attempt

Adding new files usally helps

+++ b/core/lib/Drupal/Core/Entity/ContentEntityFormControllerInterface.php
@@ -0,0 +1,37 @@
+ * Contains \Drupal\Core\Entity\EntityFormControllerInterface.
+ */
+
+namespace Drupal\Core\Entity;
+
+/**
+ * Defines a common interface for entity form controller classes.
+ */

To be fixed on the next reroll.

FIxed #6.

Looks good to me as well, it's just moving some code to the content entity form controller.

I agree with the patch. Config entities do not have or use form language like content entities do. I'd like to ensure default config entity language is properly handled for when language element is not on the form though:

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -152,21 +150,12 @@ public function form(array $form, array &$form_state) {
-    if (!isset($form['langcode'])) {
-      // If the form did not specify otherwise, default to keeping the existing
-      // language of the entity or defaulting to the site default language for
-      // new entities.
-      $form['langcode'] = array(
-        '#type' => 'value',
-        '#value' => !$entity->isNew() ? $entity->language()->id : language_default()->id,
-      );
-    }

Where is the config entity equivalent of this default? There probably (hopefully) is somewhere and hopefully there are tests for that :D #1966436: Default *content* entity languages are not set for entities created with the API may be related.

That hunk is exactly what was bothering me: I hoped to see some test failing, I guess we are missing some coverage.

Reroll.

To the issue with:

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -152,21 +150,12 @@ public function form(array $form, array &$form_state) {
-    if (!isset($form['langcode'])) {
-      // If the form did not specify otherwise, default to keeping the existing
-      // language of the entity or defaulting to the site default language for
-      // new entities.
-      $form['langcode'] = array(
-        '#type' => 'value',
-        '#value' => !$entity->isNew() ? $entity->language()->id : language_default()->id,
-      );
-    }

In ConfigEntities we have add always the language on entity_create.
See ConfigStorageController::create()

$values += array('langcode' => language_default()->id);

This is also tested in for example testCustomBlockTypeCreation() in CustomBlockTypeTest.

Thats why removing this doesn't bring up new errors, as we already ensure ConfigEntities have a language set.
And I'm not aware of other base entities, except of Config and Content, which both are getting a language. So in my mind this can go back to RTBC.

Messed up the reroll. This should work.

So in my mind this can go back to RTBC.

Sounds good :)

Agreed.

14: et-form_controller_langcode-2123867-14.patch queued for re-testing.

14: et-form_controller_langcode-2123867-14.patch queued for re-testing.

no longer applies, reroll

20: entity-language-2123867.18.patch queued for re-testing.

Back to RTBC

20: entity-language-2123867.18.patch queued for re-testing.

Patch no longer applies, rerolled.

Looks like the same as above.

hm, +1 on simplifying, but why does that mean we have to separate things into an ContentEntityFormInterface? Having a different base class is one thing, but a different interface everywhere worries me. Then, "content entity interface" is just an agglomeration of interface, if we need a special variant of entity forms for translatable entities, then it should be bound to translatable interface imho.

But then, the concept of entities having a language is part of EntityInterface, consequently I don't see a problem with having getFormLanguage() part of the regular EntityFormInterface. If the problem is only isDefaultFormLangcode() I don't see this as satisfying the need of a separate interface, it can be done by getting the entity + comparing the language anyway, i.e. could be solved in a helper at ContentTranslationController also.

Finally, I don't really see how the patch simplifies things?

@fago: the point is to simplify the base entity form interface, because form language code does not apply to that; at least config entities don't use their base forms to edit the entity in other language variants (because the forms are not component base, the route permission models don't support this, etc). So the simplification would be to make the base class simpler in favor of having the relevant logic only at the relevant place.

Looks like you are arguing for a won't fix and have useless code in entity form base class instead that is misleading for config forms?

@fago: what do you think? Would be good to move on with this. It's been on our sprint for long :/

Another re-roll...

I'm trying to reroll this. (https://drupal.org/patch/reroll)

Here was the conflict in Drupal/Core/Entity/EntityFormController:

<<<<<<< HEAD
    if ($entity->getEntityType()->isFieldable()) {
      field_attach_form($entity, $form, $form_state, $this->getFormLangcode($form_state));
=======
    if ($entity->entityInfo()->isFieldable()) {
      field_attach_form($entity, $form, $form_state);
>>>>>>> from 26th

patch said to do:
if ($entity->entityInfo()->isFieldable()) {
- field_attach_form($entity, $form, $form_state, $this->getFormLangcode($form_state));
+ field_attach_form($entity, $form, $form_state);

So I kept the context line from head, and took out the 4th argument like the patch said to do.

Ended up with:

    if ($entity->getEntityType()->isFieldable()) {
      field_attach_form($entity, $form, $form_state);

--
No interdiff since this was a reroll.
New patch attached.

Next steps,
see what testbot says, and
still waiting on a look from @fago I think.

36: 2123867-language-EntityFormController-36.patch queued for re-testing.

@fago you did not agree with the premise of this patch in December? Do you have any updated thinking? Is this worth even working on? :)

My thinking is still as said in #29 - so, yeah given that there is not really something left here. One thing I could see us doing is removing function isDefaultFormLangcode(array $form_state) from the interface as this is trivial to implement.

So sounds like almost a won't fix then. Does not seem to be anywhere close to as important to need to be on the sprint then.

One of the main concern on #29 is the introduction of new interface just for the language methods makes things harder. However we already got this ContentEntityFormInterface for form display getter/setter. So moving language methods going to be a cleanup now :). Like remove method with just return TRUE. Adding back to D8MI sprint to track and push or reject and kill :)

Let's do this - it is a nice API tidy up and was rtbc'd pre the beta and the impact is minimal.

Committed 7e02386 and pushed to 8.0.x. Thanks!

Yay!