Problem/Motivation

Inline Entity Form was frequently cited in the thread that added forms modes:

#2014821: Introduce form modes UI and their configuration entity

Currently Inline Entity Form does not take advantage of the ability to add and configure multiple form modes.

Proposed resolution

Make form mode configurable at the widget level by adding on to InlineEntityFormBase::settingsForm()

Remaining tasks

  • Post initial patch
  • Find out why passing a form mode name to getFormObject other than 'default' throws an error.

User interface changes

And additional select list will be added to the field widget configuration.

API changes

I don't know if this change would be considered an API addition.

Data model changes

The settings form seems to handle this additional option without any special consideration.

CommentFileSizeAuthor
#51 ief-form_mode_select-2510274-51-TEST_ONLY.patch12.63 KBtedbow
#51 ief-form_mode_select-2510274-51.patch24.21 KBtedbow
#49 ief-form_mode_select-2510274-49.patch24.21 KBtedbow
#49 ief-form_mode_select-2510274-49-TEST_ONLY.patch12.63 KBtedbow
#48 ief-form_mode_select-2510274-48.patch12.61 KBtedbow
#45 ief-form_mode_select-2510274-45.patch9.99 KBtedbow
#41 ief-form_mode_select-2510274-41.patch9.43 KBtedbow
#37 add_ability_to_select-2510274-37.patch12.07 KBtassilogroeper
#37 2510274-32-37.txt572 bytestassilogroeper
#33 add_ability_to_select-2510274-32.patch11.51 KBtassilogroeper
#32 add_ability_to_select-2510274-32.patch0 bytestassilogroeper
#32 2510274-25-32.txt14.26 KBtassilogroeper
#1 form-modes-2510274-1.patch4.46 KBstevector
#10 inline_entity_form-2510274-2.patch4.32 KBEyal Shalev
#16 inline_entity_form-2510274-3.patch6.33 KBEyal Shalev
#22 inline_entity_form-2510274-4.patch8.59 KBEyal Shalev
#25 2510274-4-25.txt3.38 KBtassilogroeper
#25 add_ability_to_select-2510274-25.patch9.92 KBtassilogroeper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stevector’s picture

Assigned: stevector » Unassigned
Status: Active » Needs review
FileSize
4.46 KB

Here is an initial patch. It is built on #2491527-16: [Drupal8] IEF FieldWidget merge & refactor so the patch likely won't apply cleanly to HEAD.

The widget config seems to save correctly. However using it records this error in the log:

Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: The "node" entity type did not specify a "ief" form class. in Drupal\Core\Entity\EntityManager->getFormObject() (line 294 of /var/www/workbench.local/web/core/lib/Drupal/Core/Entity/EntityManager.php).

To test this functionality I've

  • Added a reference field from page nodes that points at article nodes
  • Added a custom form mode "IEF" at admin/structure/display-modes/form
  • Activated the form mode for articles at admin/structure/types/manage/article/form-display
  • Selected the custom form mode on the widget config

I gather that the custom form display mode somehow needs to be associated with a form class. I don't know how to do that. Anyone have insight on how to render a non-default form mode?

Status: Needs review » Needs work

The last submitted patch, 1: form-modes-2510274-1.patch, failed testing.

stevector’s picture

In researching something else, I came across the documentation for hook_entity_type_build https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

The example code shows a form class being set for a form mode

function hook_entity_type_build(array &$entity_types) {
  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
  // Add a form for a custom node form without overriding the default
  // node form. To override the default node form, use hook_entity_type_alter().
  $entity_types ['node']->setFormClass('mymodule_foo', 'Drupal\mymodule\NodeFooForm');
}

Is implementing this hook required to use a form mode that was added through the UI?

slashrsm’s picture

Issue tags: +Media Initiative, +sprint
slashrsm’s picture

Issue tags: -sprint +D8Media
stevector’s picture

Micha1111’s picture

Another related issue here #2530086

bojanz’s picture

I think IEF should define and use "Inline" form mode by default.
When designing the D7 module we always said that the inline context was very different (different enough to warrant entirely different forms at the time), that still feels true.

Doing that means we don't need a setting.

slashrsm’s picture

While working on #2508107: Port "Single" IEF field widget to D8 I noticed another issue related to form display. If we have a reference field that references the same entity type and uses the same form display type as "host" node simple widget won't work. Core is statically caching form objects which in our case results in both forms (IEF and host) sharing the same object. This causes all sorts of strange problems.

We can avoid this problems if:
a) we use dedicated form type for IEF
b) allow to configure form type for IEF and check for collision (and throw an error)

Eyal Shalev’s picture

A patch against the current dev version.

This patch adds a setting for the display mode that provides all the entity form displays for the targeted entity.

And passes the loaded display mode in the render array to be used to build the form.

Status: Needs review » Needs work

The last submitted patch, 10: inline_entity_form-2510274-2.patch, failed testing.

The last submitted patch, 10: inline_entity_form-2510274-2.patch, failed testing.

The last submitted patch, 10: inline_entity_form-2510274-2.patch, failed testing.

The last submitted patch, 10: inline_entity_form-2510274-2.patch, failed testing.

The last submitted patch, 10: inline_entity_form-2510274-2.patch, failed testing.

Eyal Shalev’s picture

Fixed the paths in my previous patch.
Made it possible to override the inline form object by using the operation variable.

To override form object you need to add the path to a form class in the entity annotation with the same name as the description.

Drupal\foo\Entity\Foo.php

/**
 * Defines the node entity class.
 *
 * @ContentEntityType(
 *   id = "foo",
 *   label = @Translation("Foo"),
 *   handlers = {
 *     "form" = {
 *       "default" = "Drupal\foo\FooForm",
 *       "delete" = "Drupal\foo\Form\FooDeleteForm",
 *       "edit" = "Drupal\foo\FooForm",
 *       "inline" = "Drupal\foo\FooInlineForm"
 *     },

core.entity_view_mode.foo.inline.yml

langcode: en
status: false
dependencies:
  module:
    - foo
id: foo.inline
label: 'Foo content'
targetEntityType: foo
cache: true
Eyal Shalev’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: inline_entity_form-2510274-3.patch, failed testing.

The last submitted patch, 16: inline_entity_form-2510274-3.patch, failed testing.

The last submitted patch, 16: inline_entity_form-2510274-3.patch, failed testing.

The last submitted patch, 16: inline_entity_form-2510274-3.patch, failed testing.

Eyal Shalev’s picture

Eyal Shalev’s picture

Status: Needs work » Needs review
tassilogroeper’s picture

Assigned: Unassigned » tassilogroeper

I ran into a problem with the dropdown not displaying options, when applying this patch.

tassilogroeper’s picture

So when trying to inline a taxonomy_term the patch will not work. Due to the fact, that for taxonomy_terms the form is created on the fly (if you use all the defaults) and will not return any "entity_form_display" configuration. Thus, I just add a default option for the dropdown and it works.

Also I suggest to set a constant for the "default" view_mode.

tassilogroeper’s picture

Assigned: tassilogroeper » Unassigned
tassilogroeper’s picture

axe312’s picture

I might help and have already experience with this since I am one of the maintainers of https://www.drupal.org/project/view_mode_selector.

Also I should be online in #drupal-contribute during this week.

dawehner’s picture

  1. +++ b/src/Form/EntityInlineForm.php
    @@ -138,13 +139,30 @@ class EntityInlineForm implements InlineFormInterface {
    +    return empty($entity_form['#entity']->getEntityType()->getFormClass($display_mode_name)) ? $default_operation : $display_mode_name;
    

    I'm wondering whether a fallback is the right thing to do? Should we not just validate that the configuration of the display_mode is valid during configuration time?

  2. +++ b/src/Form/EntityInlineForm.php
    @@ -360,4 +378,27 @@ class EntityInlineForm implements InlineFormInterface {
    +  public function getEntityFormDisplay($display_mode_name = 'default') {
    +    $display_modes = $this->getEntityFormDisplays();
    +    $display_mode = current($display_modes);
    +    if (isset($display_modes[$display_mode_name])) {
    +      $display_mode = $display_modes[$display_mode_name];
    +    }
    +    return $display_mode;
    +  }
    

    That is a bit confusing, why do we not execute a special entity query which just returns the one we need? Getting all of them first and then just using one of them seems a bit pointless, or is this just me?

  3. +++ b/src/InlineFormInterface.php
    @@ -78,6 +83,33 @@ interface InlineFormInterface extends EntityHandlerInterface {
    +   * Returns the id of entity type managed by this handler.
    +   *
    +   * @return string
    +   *   The [entity form] operation name
    +   */
    +  public static function getOperation($entity_form, $default_operation = self::DEFAULT_DISPLAY_MODE);
    +
    

    This method is just used internal, should we make it protected instead?

  4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
    @@ -187,10 +190,44 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
    +   * @return array
    

    Nitpick: We could use @return string[] to be a bit more specific.

webflo’s picture

  1. +++ b/src/Form/EntityInlineForm.php
    @@ -360,4 +378,27 @@ class EntityInlineForm implements InlineFormInterface {
    +  public function getEntityFormDisplays() {
    

    We should query the actual available form mode not the configured entity view displays.

    I think we should rename the function as well.

  2. +++ b/src/InlineFormInterface.php
    @@ -16,6 +16,11 @@ use Drupal\Core\Form\FormStateInterface;
    +  const DEFAULT_DISPLAY_MODE = 'default';
    

    I think the name DEFAULT_FORM_MODE is more accurate.

webflo’s picture

+++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
@@ -187,10 +190,44 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
+  protected function getDisplayModes() {

Replace the entire function with EntityDisplayRepository::getFormModes

tassilogroeper’s picture

So webflo and I sat together on this one.
- added two constants for default display_mode and default operation
- pay attention: the initial patch mixed up the form mode ("default") with the form handling ("default", "edit" etc.), thus we made the distinction here
- I removed redundant fallbacks for fetching the form mode

tassilogroeper’s picture

The last submitted patch, 32: add_ability_to_select-2510274-32.patch, failed testing.

tassilogroeper’s picture

Assigned: tassilogroeper » Unassigned

Status: Needs review » Needs work

The last submitted patch, 33: add_ability_to_select-2510274-32.patch, failed testing.

tassilogroeper’s picture

update test stubs with the new form field

Eyal Shalev’s picture

@tassilogroeper The reason I combined the form handler and the display mode was to allow for custom form classes to be used.
Without it the default class will always be used.

bojanz’s picture

Just clarifying that I'm now +1 on having this setting.

+      '#ief_display_mode' => NULL,

We should just call this #form_mode.

The reroll can wait until we fix #2667710: Rewrite the base inline form handling.

EDIT: No longer blocked, let's go.

tedbow’s picture

Assigned: Unassigned » tedbow

Ok, I am starting on re-rolling this.

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
9.43 KB

Ok here is re-roll

I changed 'display_mode' to 'form_mode' in variables and '#ief_form_mode'

I also made some other changes because we are no longer use the $controller of the entity.

  1. +++ b/src/Element/InlineEntityForm.php
    @@ -35,6 +35,9 @@ class InlineEntityForm extends RenderElement {
    +      // Instance of \Drupal\Core\Entity\Display\EntityFormDisplayInterface.
    +      // The entity form display that is used to display the entity.
    +      '#ief_display_mode' => NULL,
    

    It seems like don't actually need to be storing the object here just the form mode settings like 'default', 'inline', etc

  2. +++ b/src/Form/EntityInlineForm.php
    @@ -57,12 +66,15 @@ class EntityInlineForm implements InlineFormInterface {
        */
    
    +++ b/src/InlineFormInterface.php
    @@ -16,6 +16,16 @@ use Drupal\Core\Form\FormStateInterface;
    +   * The fallback form handler to call.
    +   */
    +  const DEFAULT_OPERATION = 'default';
    +
    +  /**
    

    This not need as it used in getFormObject which is not being called anymore

  3. +++ b/src/InlineFormInterface.php
    @@ -78,6 +88,30 @@ interface InlineFormInterface extends EntityHandlerInterface {
    +  /**
    +   * Returns the entity form display entity selected in the settings widget.
    +   * If the display mode doesn't exist in the available modes list then return
    +   * the first mode in the list.
    +   *
    +   * @param string $entity_type
    +   *   The entity type.
    +   * @param string $bundle
    +   *   The bundle.
    +   * @param string $display_mode_name
    +   *   The display mode to render.
    +   *
    +   * @return \Drupal\Core\Entity\Display\EntityFormDisplayInterface
    +   */
    +  public function getEntityFormDisplay($entity_type, $bundle, $display_mode_name = self::DEFAULT_DISPLAY_MODE);
    +
    

    This method is not need anymore because we don't need the instance just the string of the form_mode.

  4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
    @@ -187,6 +189,13 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
    +      '#options' => $this->iefHandler->getEntityFormModes()
    

    I left this in but do we need to rely on iefHandler for getEntityFormModes? Since we know that entity type from the field definition we could just form modes direclty here. There is no other place getEntityFormModes is being used and we could remove it. Is there any case where the handler would want to limit the options of form modes? Maybe?

  5. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
    @@ -304,6 +313,12 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
    +    $ief['#ief_display_mode'] = $this->iefHandler->getEntityFormDisplay($this->getFieldSetting('target_type'), $bundle, $this->getSetting('display_mode'));
    

    We don't need to get the instance here. We can simply

    $ief['#ief_form_mode'] = $this->getSetting('form_mode');
    

    Even if the form mode has been deleted it won't be problem because \Drupal\Core\Entity\Entity\EntityFormDisplay::collectRenderDisplay check for the existence of the form mode and will use 'default' if not available.

tedbow’s picture

Ok we need some tests for this.

To start of with in InlineEntityFormElementWebTest and InlineEntityFormSimpleWebTest

We could

  1. Export an 'inline' form mode to inline_entity_form/tests/modules/inline_entity_form_test/config for node.type.ief_test_custom
  2. In each of the above classes check using form modes: default, inline.

Do need some kind of dependency check? Should you able to delete a form mode if a instance is using it?

tedbow’s picture

Yay! current tests pass.

I just notice that we probably need to update \Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormBase::settingsSummary

To show the form mode setting.

I checked locally and the widget does respect the form mode.

bojanz’s picture

Generally looks good.

Needs fixing:
1) public function getEntityFormModes(); doesn't belong on the inline handler
It's used in the widget, add it to the widget (base class)

2) const DEFAULT_FORM_MODE = 'default'; isn't needed cause default is already the fallback

Ideally our configuration would could a dependency on the selected form mode, but I'm unsure how that can be done from inside the widget plugin.

tedbow’s picture

Added changes mention in #44

Also added InlineEntityFormBase::calculateDependencies

To add dependency on the Entity Form Mode.

Updated InlineEntityFormBase::settingsSummary to show form module in summary.

Status: Needs review » Needs work

The last submitted patch, 45: ief-form_mode_select-2510274-45.patch, failed testing.

tedbow’s picture

arrrgggh. always. test. locally.

Forgot to update \Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormComplex::__construct
to match.

Will fix soon

tedbow’s picture

Status: Needs work » Needs review
FileSize
12.61 KB

Fixed InlineEntityFormComplex::__construct

And phpdocs

tedbow’s picture

tedbow’s picture

Some notes about the new tests:

  1. +++ b/src/Tests/InlineEntityFormComplexWebTest.php
    @@ -26,13 +26,6 @@ class InlineEntityFormComplexWebTest extends InlineEntityFormTestBase {
    -   * User with permissions to create content.
    -   *
    -   * @var \Drupal\user\Entity\User
    -   */
    -  protected $user;
    -
    

    Moved the $user property to the base test class. All classes need this.

  2. +++ b/src/Tests/InlineEntityFormElementWebTest.php
    @@ -28,10 +28,14 @@ class InlineEntityFormElementWebTest extends InlineEntityFormTestBase {
    +    $this->drupalLogin($this->user);
    

    The element test was creating a user but never using/logging in with that user. It didn't matter before because there is no permission check on the custom from. Now to test the different fields on theform display the user needs 'administer nodes' permission.

  3. +++ b/src/Tests/InlineEntityFormElementWebTest.php
    @@ -41,40 +45,48 @@ class InlineEntityFormElementWebTest extends InlineEntityFormTestBase {
    +    $form_mode_possibilities = [
    +      'default',
    +      'inline',
    +    ];
    +    foreach ($form_mode_possibilities as $form_mode_possibility) {
    +      $title = $this->randomMachineName();
    

    Basically just taking the previous tests and running for each form mode.

  4. +++ b/src/Tests/InlineEntityFormTestBase.php
    @@ -50,7 +57,6 @@ abstract class InlineEntityFormTestBase extends WebTestBase {
    -    $this->assert('pass', "node = " . $node);
    

    This was just an old debugging line I had left in from previous test. Sorry

  5. +++ b/src/Tests/InlineEntityFormTestBase.php
    @@ -75,4 +81,54 @@ abstract class InlineEntityFormTestBase extends WebTestBase {
    +  /**
    +   * Checks for check correct fields on form displays based on exported config
    +   * in inline_entity_form_test module.
    +   *
    +   * @param $form_display
    +   *  The form display to check.
    +   */
    +  protected function checkFormDisplayFields($form_display, $prefix) {
    +    $form_display_fields = [
    

    Added this function to the base class so that we can check form displays from all test classes in the future.

  6. +++ b/src/Tests/InlineEntityFormTestBase.php
    --- /dev/null
    +++ b/tests/modules/inline_entity_form_test/config/install/core.entity_form_display.node.ief_test_custom.inline.yml
    

    Add new form display for just this bundle for now.

  7. +++ b/tests/modules/inline_entity_form_test/config/install/core.entity_form_display.node.ief_test_custom.inline.yml
    --- /dev/null
    +++ b/tests/modules/inline_entity_form_test/config/install/core.entity_form_mode.node.inline.yml
    

    Added "inline" form mode to node for testing.

tedbow’s picture

oh no TEST_ONLY passed!

foreach ($fields['unexpected'] as $unexpected_field) {
  $this->assertNoFieldByName($prefix . $unexpected_field, NULL);
}

I wasn't sending NULL as second arg here.

Updated patchs

Status: Needs review » Needs work

The last submitted patch, 51: ief-form_mode_select-2510274-51-TEST_ONLY.patch, failed testing.

  • bojanz committed 48f5536 on 8.x-1.x authored by tedbow
    Issue #2510274 by tedbow, tassilogroeper, Eyal Shalev, stevector: Add...
bojanz’s picture

Status: Needs work » Fixed
Issue tags: -Needs tests

Renamed #ief_form_mode to #form_mode, made the setting required and defaulting to 'default'. Committed.

This was an important one, thanks tedbow!

Status: Fixed » Closed (fixed)

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

mogio_hh’s picture

Just tested the form mode feature in the latest DEV.
Although there is a select control to pick the "form view mode", "default" is rendered - no matter what is selected.

Does this patch fix this problem?
To me it seems as if this patch went already to beta but its now broken again?!

Thanks

digihumsteve’s picture

@mogio_hh I have the same issue you report in #55. Renders the default form mode, whatever is set in form mode.

brooke_heaton’s picture

This was a great feature, but I'm confused as to why IEF does not respect or consider the Twig template of said Form mode? If you override the form mode template, nothing happens when the IEF is rendered. It's odd.

thronedigital’s picture

facing this issue as #58

Any suggestions would be appreciated