EntityFormController has code to support entity language and changes made to entity language in form submission.
However, only those features only make sense for content entities (translatability for config entities is entirely different), and the ContentEntityFormController subclass has its own support for content entity translation.

So it seems this language related code in EntityFormController could be simplified ?

Comments

yched’s picture

tags - not sure those are the ones currently used by the initiatives...

plach’s picture

plach’s picture

Issue tags: +API change
FileSize
6.73 KB
FAILED: [[SimpleTest]]: [MySQL] 51,502 pass(es), 2,060 fail(s), and 1,228 exception(s). View

Here is a first attempt

plach’s picture

Status: Active » Needs review
plach’s picture

FileSize
7.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,971 pass(es). View

Adding new files usally helps

plach’s picture

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

plach’s picture

FileSize
7.97 KB
PASSED: [[SimpleTest]]: [MySQL] 59,893 pass(es). View
785 bytes

FIxed #6.

yched’s picture

Thanks!
Patch seems sensible to me, but this could sure use other eyes...

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

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.

plach’s picture

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

kfritsche’s picture

Issue summary: View changes
FileSize
7.75 KB
FAILED: [[SimpleTest]]: [MySQL] 51,286 pass(es), 2,062 fail(s), and 1,226 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 12: et-form_controller_langcode-2123867-12.patch, failed testing.

kfritsche’s picture

Status: Needs work » Needs review
FileSize
7.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch et-form_controller_langcode-2123867-14.patch. Unable to apply patch. See the log in the details link for more information. View

Messed up the reroll. This should work.

plach’s picture

Status: Needs review » Reviewed & tested by the community

So in my mind this can go back to RTBC.

Sounds good :)

Gábor Hojtsy’s picture

Agreed.

Xano’s picture

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: et-form_controller_langcode-2123867-14.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
7.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entity-language-2123867.18.patch. Unable to apply patch. See the log in the details link for more information. View

no longer applies, reroll

Status: Needs review » Needs work

The last submitted patch, 14: et-form_controller_langcode-2123867-14.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: entity-language-2123867.18.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

LinL’s picture

LinL’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.71 KB
PASSED: [[SimpleTest]]: [MySQL] 59,327 pass(es). View

Patch no longer applies, rerolled.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks like the same as above.

fago’s picture

Status: Reviewed & tested by the community » Needs work

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?

The last submitted patch, 20: entity-language-2123867.18.patch, failed testing.

Gábor Hojtsy’s picture

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

yched’s picture

Gábor Hojtsy’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2014_JANUARY, +SprintWeekend2014
FileSize
7.43 KB
PASSED: [[SimpleTest]]: [MySQL] 63,203 pass(es). View

Another re-roll...

YesCT’s picture

Assigned: Unassigned » YesCT
Status: Needs review » Needs work
Issue tags: +Needs reroll

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

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2123867-language-EntityFormController-36.patch. Unable to apply patch. See the log in the details link for more information. View

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.

YesCT’s picture

Gábor Hojtsy’s picture

Assigned: Unassigned » fago

@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? :)

fago’s picture

Assigned: fago » Unassigned

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.

Gábor Hojtsy’s picture

Issue tags: -sprint

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.

Status: Needs review » Needs work

The last submitted patch, 36: 2123867-language-EntityFormController-36.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +sprint
FileSize
6.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,923 pass(es). View

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 :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Indeed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 7e02386 on 8.0.x
    Issue #2123867 by plach, vijaycs85, kfritsche, YesCT, LinL, larowlan |...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay!

Status: Fixed » Closed (fixed)

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