Problem/Motivation

hook_entity_extra_field_info() allows to declare extra fields for forms as well as displays. Currently, the Extra Field module hardcodes implementations to display:

ExtraFieldFormatterManager::fieldInfo()

$info[$entityType][$bundle]['display'][$fieldName] = [
          'label' => $definition['label'],
          'weight' => $definition['weight'],
          'visible' => $definition['visible'],
        ];

Proposed resolution

Allow ExtraField plugins to be a 'display' or 'form' implementation.

Remaining tasks

  1. Write a patch
  2. Community review and testing
  3. Update documentation
  4. Include a few plugin examples
  5. Commit

User interface changes

Extra Field Formatters will be available in the Form UI admin interface.

API changes

Only additions to the API:

  1. A new annotation, @ExtraFieldForm(), with the same fields as @ExtraFieldDisplay,
  2. A new hook, hook_extra_field_form_info_alter() to alter @ExtraFieldForm() annotation data,
  3. A new plugin manager service, plugin.manager.extra_field_form_display which maps to \Drupal\extra_field\Plugin\ExtraFieldFormManager, which conforms to a new \Drupal\extra_field\Plugin\ExtraFieldFormManagerInterface,
  4. A \Drupal\extra_field\Plugin\ExtraFieldFormBase abstract class to provide a default implementation for extra form field plugins, and,
  5. A \Drupal\extra_field\Plugin\ExtraFieldFormInterface interface to provide a specification for extra form field plugins.

Data model changes

No changes.

CommentFileSizeAuthor
#39 2882061-39.patch30.1 KBSutharsan
#39 interdiff-2882061-38-39.txt9.8 KBSutharsan
#37 2882061-37.patch23.84 KBSutharsan
#37 interdiff-2882061-36-37.txt5.62 KBSutharsan
#36 2882061-36.patch18.6 KBSutharsan
#36 interdiff-2882061-35-36.txt831 bytesSutharsan
#35 2882061-35.patch18.59 KBSutharsan
#35 interdiff-2882061-34-35.txt1.88 KBSutharsan
#34 interdiff-2882061-33-34.txt6.25 KBSutharsan
#34 2882061-34.patch18.61 KBSutharsan
#33 2882061-33.patch18.6 KBSutharsan
#31 2882061-31.patch23.82 KBidebr
#31 interdiff-29-31.txt15.9 KBidebr
#29 interdiff-2882061-24-29.txt14.17 KBSutharsan
#29 extra_field-using_in_forms-2882061-29.patch21.48 KBSutharsan
#27 interdiff--14-24.txt4.54 KBmparker17
#24 extra_field-using_in_forms-2882061-24.patch20.03 KBandrew_tspkh
#22 extra_field-using_in_forms-2882061-22.patch4.79 KBandrew_tspkh
#14 extra_field-using_in_forms-2882061-14.patch19.49 KBandrew_tspkh
#10 extra_field-using_in_forms-2882061-10.patch17.9 KBandrew_tspkh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr created an issue. See original summary.

Sutharsan’s picture

With class naming I anticipated on having separate plugins and plugin managers for EF Formatters and EF Widgets. That is the reason behind a hard coded 'display' key in ExtraFieldFormatterManager::fieldInfo(). This does not mean that we can not integrate formatters and widgets in one plugin (manager).

I'm open to suggestions. But once this module reaches beta, I will not allow disruptive changes anymore. @idebr If you plan to work on this issue (you have not assigned it to yourself) let me know. Beta is not too far away anymore.

idebr’s picture

With class naming I anticipated on having separate plugins and plugin managers for EF Formatters and EF Widgets. That is the reason behind a hard coded 'display' key in ExtraFieldFormatterManager::fieldInfo()

Ah, having separate managers would make sense since a vanilla implementation (without Extra Field) would also require implementing different hooks (hook_entity_view vs. hook_form_alter). However the current naming scheme does not indicate if the plugin implements a 'display' or 'form' extra_field. Perhaps these naming conventions could be updated to more accurately reflect their implementation?

Current implementation:

class ExampleArticle extends ExtraFieldFormatterBase {

Suggested implementation:

class ExampleArticle extends ExtraFieldDisplayBase {

and possible (future) implementation:

class ExampleArticle extends ExtraFieldFormBase {
I'm open to suggestions. But once this module reaches beta, I will not allow disruptive changes anymore. @idebr If you plan to work on this issue (you have not assigned it to yourself) let me know. Beta is not too far away anymore.

Form extra fields are currently not in scope for the project I am working on, so I will probably not be working on this feature any time shortly.

Sutharsan’s picture

I did choose 'Formatter' and 'Widget' as part of the ExtraField formatters to be in line with core's field formatter and field widget plugins.

idebr’s picture

For what it's worth: a (senior) colleague today expected ExtraFieldFormatterBase to implement Drupal\Core\Field\FormatterInterface and was thoroughly confused about the class naming, so I'm still proposed to renaming these classes to prevent confusion with FieldWidget / FieldFormatter plugins.

Sutharsan’s picture

Issue tags: +alpha target
Sutharsan’s picture

Issue tags: -alpha target

This issue got derailed with the naming discussion. To rename the plugin I've created (and fixed) a new issue: #2894463: Rename plugins to be more developer friendly.

Sutharsan’s picture

Title: Allow form extra fields to be declared through ExtraField plugins » Allow extra Field Formatters to be declared through plugins
Issue summary: View changes
Related issues: +#1983398: Allow formatters & formatter settings for extra_fields (display), +#2904480: Allow editable display settings on extra fields
idebr’s picture

Title: Allow extra Field Formatters to be declared through plugins » Allow form extra fields to be declared through ExtraField plugins

Sorry for the confusion, this issue not about configurable extra fields. Its purpose is to support extra fields in forms.

andrew_tspkh’s picture

Status: Active » Needs review
FileSize
17.9 KB

Hi there
I've added new functionality to provide the new extra field plugins for using in the entity form displays.
The example plugin you can find here:extra_field/modules/extra_field_example/src/Plugin/ExtraField/Form/ExampleFormAllNodes.php
Please, review.
Thanks!

Status: Needs review » Needs work

The last submitted patch, 10: extra_field-using_in_forms-2882061-10.patch, failed testing. View results

Sutharsan’s picture

Thanks for contributing. Extra Field was made to allow this addition, to here we go ;)

  1. +++ b/extra_field.module
    @@ -5,15 +5,48 @@
    +  $info = _extra_field_merge_infos($info_form, $info_display);
    

    Can array_merge_recursive() be used here?

  2. +++ b/src/Annotation/ExtraFieldForm.php
    @@ -0,0 +1,56 @@
    + * Defines a ExtraFieldFormDisplay item annotation object.
    

    We should be carefull on naming. Not to confuse the two plugin types.

  3. +++ b/src/Plugin/ExtraFieldFormManager.php
    @@ -0,0 +1,246 @@
    +  protected function matchEntityBundleKey(array $plugin_bundles, $entity_bundle_key) {
    

    This is duplicate code. Add a todo for later refactoring.
    Also supportedEntityBundles, allEntityBundles, fieldName, entityBundleKey

Sutharsan’s picture

Sorry, no time for more review now.

andrew_tspkh’s picture

Status: Needs work » Needs review
FileSize
19.49 KB

hi, thank you for reviewing!
Here is new patch with improvements :-)

Status: Needs review » Needs work

The last submitted patch, 14: extra_field-using_in_forms-2882061-14.patch, failed testing. View results

idebr’s picture

I created a new issue to fix the failure on the test runner on the Drupal infrastructure: #2986211: Missing @group annotation in Drupal\Tests\extra_field\Functional\ExtraFieldBrowserTestBase

andrew_tspkh’s picture

Status: Needs work » Needs review
mparker17’s picture

Since #2986211: Missing @group annotation in Drupal\Tests\extra_field\Functional\ExtraFieldBrowserTestBase was committed earlier today, I re-ran the test, which shows up green.

Furthermore, I was able to successfully create an ExtraFieldForm plugin which extends ExtraFieldFormBase and have it show up on the form for the entity type and bundle I specified in the description.

Overall the code looks clear and conforms to coding standards and best practices. That being said...

1. I did notice the patch adds a bunch of TODO comments: were we planning to solve any/some/all of those in this patch, or fix them in future patch(es)?

2. In modules/extra_field_example/src/Plugin/ExtraField/Form/ExampleFormAllNodes.php, there's a call on line 49 to drupal_set_message('Custom submit was triggered.'). The Coder module's DrupalPractice linter says: "Messages are user facing text and must run through t() for translation". Since this code is in an example file, I don't know if it's worth fixing this issue.

3. On that note, drupal_set_message() was deprecated in favor of MessengerTrait in 8.5. Since this code is in an example file, and drupal_set_message() will stick around until D9 (since removing it would break backwards compatibility), I don't know if it's worth fixing this.

mparker17’s picture

Status: Needs review » Reviewed & tested by the community

Boldly marking this as RTBC; feel free to bump it back if we want to fix the issues in #18.

The last submitted patch, 10: extra_field-using_in_forms-2882061-10.patch, failed testing. View results

mparker17’s picture

Could I request an addition? Could we pass the $form_state from extra_field_form_alter() through \Drupal\extra_field\Plugin\ExtraFieldFormManager::entityView() to the plugin's formView()?

When I am displaying a form extra field on an \Drupal\Core\Entity\EntityForm, I'd like to be able to access the entity (which is usually at $form_state->getBuildInfo()['callback_object']->getEntity() on EntityForms).

As a work-around in my specific case, I can look for parameters in the route, but that isn't ideal, because it wouldn't work for, say, inline entity forms, or forms in blocks.

(note that I'm not asking for $form to be passed through - it would also work for my use case, but I suspect someone might be tempted to modify the form directly in formView(), rather than returning what they wanted added)

Is anyone willing to update the patch in #14? If so, I can review and RTBC! If not, I'm happy to write the patch, but someone else will have to review and RTBC for me!

andrew_tspkh’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.79 KB

Hi there
Yes @mparker17 it is good idea.
I've added $form too, but not through the link - so user cannot change the form :-)
Patch updated.

Status: Needs review » Needs work

The last submitted patch, 22: extra_field-using_in_forms-2882061-22.patch, failed testing. View results

andrew_tspkh’s picture

andrew_tspkh’s picture

Status: Needs work » Needs review
mparker17’s picture

Woot, I'll review it shortly, thanks for the prompt response!

mparker17’s picture

FileSize
4.54 KB

Adding an interdiff between the patch in #14 and #24 for review (note this skips the patch in #22).

mparker17’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Code review of the patch in #24: Looks great! The patch is clear, it conforms to coding standards, and it makes good use of the Drupal API.

Manual testing on a fresh install of Drupal 8.6.x core: Works as expected!

Manual testing on my project: Works as expected!

Excellent work! Back to RTBC! Thank you very much @andrew_tspkh!

(I have updated the issue summary with some additional information now that the patch is built)

Sutharsan’s picture

Assigned: Unassigned » Sutharsan
Status: Reviewed & tested by the community » Needs review
FileSize
21.48 KB
14.17 KB

Various renaming while getting acquainted with the code. See patch.

Interface changes:
- ExtraFieldFormManagerInterface::entityView to ExtraFieldFormManagerInterface::entityFormAlter (because it is meant to be used in hook_form_alter())
- ExtraFieldFormInterface::formView to ExtraFieldFormInterface::view (to be consistent with \Drupal\Core\Field\FormatterInterface::view)

I agree with using a trait for the common code. I will add the trait to the existing code before this patch lands.

Needs review for more in-depth check.

idebr’s picture

+++ b/src/Plugin/ExtraFieldFormManager.php
@@ -0,0 +1,253 @@
+          $element = $plugin->view($form, $formState);
+          if (!empty($element)) {
+            $form[$fieldName] = [
+              '#weight' => $extraField['weight'],
+              '#type' => 'container',
+              'widget' => $element,
+            ];
+          }

I checked some real world implementations of form extra fields. This implementation would not be able to achieve a similar form structure:

Redirect module:

    $form['url_redirects'] = [
      '#type' => 'details',
      '#title' => t('URL redirects'),
      '#group' => 'advanced',
      '#open' => FALSE,
      'table' => [
        '#type' => 'table',
        '#header' => $header,
        '#rows' => $rows,
        '#empty' => t('No URL redirects available.'),
        '#attributes' => ['class' => ['redirect-table']],
      ],
      '#attached' => [
        'library' => [
          'redirect/drupal.redirect.admin',
        ],
      ],
    ];

Scheduler module:

$form['scheduler_settings'] = [
    '#type' => 'details',
    '#title' => t('Scheduling options'),
    '#open' => $expand_details,
    '#weight' => 35,
    '#attributes' => ['class' => ['scheduler-form']],
    '#optional' => FALSE,
  ];

From a developer's perspective, it would be nice to:

  1. Force the Extra Field plugin ID to match the info key, so the configured weight in the form display is applied correctly. This is already done in #29.
  2. Allow control of the $form[$fieldName] array structure. This is currently hardcoded to a '#type' => 'container',, but real world implementations suggest this is not always the case.

The method name ExtraFieldFormInterface::view() is consistent with ExtraFieldDisplayInterface::view(), but since it uses different arguments perhaps a different method name would make it easier to distinguish the two. Maybe formElement() to match \Drupal\Core\Field\WidgetInterface::formElement?

idebr’s picture

Attached patch implements the following changes:

  1. The view() return value is now assigned to the root of the $form element
  2. Added setEntity() and getEntity() to the ExtraFieldFormInterface so you have access to the current entity.
  3. The $form is now passed to the plugin by reference, since this variable is available from hook_form_alter() by reference.
Sutharsan’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev

Moving to to a new feature branch to allow new functionality.

Sutharsan’s picture

Sutharsan’s picture

Maybe formElement() to match \Drupal\Core\Field\WidgetInterface::formElement?

Good suggestion.

In the patch some personal code style changes: Variables in camel case, early returns and added empty lines.

Sutharsan’s picture

Replaced ExtraFieldFormInterface::view by ExtraFieldFormInterface::formElement.

I played around with some plugins and realized that ExtraFieldForm plugins can be used form alter too. Not sure if I like it, but in the same manner ExtraFieldDisplay plugins can modify the content of the host entity object. It just comes with passing data around by reference.

The code is getting mature, but needs more tests before I want to commit it.

Sutharsan’s picture

Sutharsan’s picture

Adding one kernel and one functional test.

Sutharsan’s picture

Issue summary: View changes

Can someone provide me with some practical examples for using EF form plugins? Perhaps one or two can be turned into an example plugin.

Further, when would you use an extra field form plugin and when use an hook_form_alter implementation?

Sutharsan’s picture

Renamed $formState to $form_state in ExtraFieldFormInterface for consistent developer experience.
Updated the readme with documentation for form type extra fields.

idebr’s picture

#38 If I have an element to add to the form, I would use an extra field form element. This API is not new; Drupal Core already enables it. This module only provides a Plugin wrapper.

Below is a form element that integrates the Group module with the node form. It uses both a form submit handler as as well an entity builder. Either could be used for an example plugin.

+++ b/modules/extra_field_example/src/Plugin/ExtraField/Form/CustomSubmit.php
@@ -0,0 +1,54 @@
+  /**
+   * Handle custom submit.
+   *
+   * @param array $form
+   *   The entity form array.
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The form state object.
+   */
+  public static function addCustomMessage(array $form, FormStateInterface $form_state) {
+
+    \Drupal::messenger()->addMessage(t('Custom submit was triggered.'));
+  }

Let's not use calls to the \Drupal:: namespace, but use dependency injection instead. Perhaps use messengerTrait here?


namespace Drupal\uu_group\Plugin\ExtraField\Form;

use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Menu\MenuParentFormSelectorInterface;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\Core\Url;
use Drupal\extra_field\Plugin\ExtraFieldFormBase;
use Drupal\group\Entity\GroupInterface;
use Drupal\node\NodeInterface;
use Drupal\pathauto\PathautoState;
use Drupal\uu_group\GroupHelperInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Allows a node to be moved to a different group.
 *
 * @ExtraFieldForm(
 *   id = "group_selection",
 *   label = @Translation("UU Group: group selection"),
 *   bundles = {
 *     "node.filterpage",
 *     "node.page",
 *   }
 * )
 */
class GroupSelection extends ExtraFieldFormBase implements ContainerFactoryPluginInterface {

  use DependencySerializationTrait;

  /**
   * The entity type manager.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

  /**
   * Group helper.
   *
   * @var \Drupal\uu_group\GroupHelperInterface
   */
  protected $groupHelper;

  /**
   * The menu parent form selector.
   *
   * @var \Drupal\Core\Menu\MenuParentFormSelectorInterface
   */
  protected $menuParentFormSelector;

  /**
   * GroupSelection constructor.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   * @param \Drupal\uu_group\GroupHelperInterface $group_helper
   *   The group helper.
   * @param \Drupal\Core\Menu\MenuParentFormSelectorInterface $menu_parent_form_selector
   *   The menu parent form selector.
   */
  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, GroupHelperInterface $group_helper, MenuParentFormSelectorInterface $menu_parent_form_selector) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->entityTypeManager = $entity_type_manager;
    $this->groupHelper = $group_helper;
    $this->menuParentFormSelector = $menu_parent_form_selector;
  }

  /**
   * {@inheritDoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('entity_type.manager'),
      $container->get('uu_group.group_helper'),
      $container->get('menu.parent_form_selector')
    );
  }

  /**
   * {@inheritDoc}
   */
  public function view(array &$form, FormStateInterface $form_state) {
    $entity = $this->getEntity();
    /** @var \Drupal\group\Entity\GroupInterface[] $groups */
    $groups = $this->entityTypeManager->getStorage('group')->loadByProperties();
    $options = [];
    foreach ($groups as $group) {
      $create_url = Url::fromRoute('entity.group_content.create_form', [
        'plugin_id' => 'group_node:' . $entity->bundle(),
        'group' => $group->id(),
      ]);
      if ($create_url->access()) {
        $options[$group->id()] = (string) $group->label();
      }
    }
    asort($options);

    if ($entity->isNew()) {
      $current_group = $this->groupHelper->getGroupFromRoute();
      $form['actions']['submit']['#submit'][] = [$this, 'addNewNodeToGroup'];
    }
    else {
      $current_group = $this->groupHelper->getGroupByEntity($entity);
      $form['#entity_builders'][] = [$this, 'groupSelectionEntityBuilder'];
    }
    $default_value = NULL;
    if ($current_group instanceof GroupInterface) {
      $default_value = $current_group->id();
      $form_state->set('current_group', $default_value);
    }

    return [
      '#type' => 'container',
      '#attributes' => ['id' => 'group-selection'],
      'group_selection' => [
        '#type' => 'select',
        '#required' => TRUE,
        '#title' => $this->t('Part of group'),
        '#empty_option' => $this->t('Select a group'),
        '#options' => $options,
        '#default_value' => $default_value,
      ],
    ];
  }

  /**
   * Ajax callback when the language changes.
   */
  public static function updateGroupSelection(array &$form, FormStateInterface $form_state) {
    return $form['extra_field_group_selection']['group_selection'];
  }

  /**
   * Entity builder callback.
   */
  public function groupSelectionEntityBuilder($entity_type, NodeInterface $node, &$form, FormStateInterface $form_state) {
    // Entity builders run twice, once during validation and again during
    // submission, so we only run this code after validation has been performed.
    if ($form_state->getTemporaryValue('entity_validated')) {
      $current_group = $form_state->get('current_group');
      $selected_group = $form_state->getValue('group_selection');
      if ($current_group === $selected_group) {
        return;
      }

      $pathauto_state = $node->path->pathauto;
      // Skip automatic alias generation while transferring groups.
      if ($pathauto_state !== PathautoState::SKIP) {
        $node->path->pathauto = PathautoState::SKIP;
      }

      // Remove all existing group content for this entity.
      /** @var \Drupal\group\Entity\Storage\GroupContentStorageInterface $group_content_storage */
      $group_content_storage = $this->entityTypeManager->getStorage('group_content');
      $group_contents = $group_content_storage->loadByEntity($node);
      foreach ($group_contents as $group_content) {
        $group_content->delete();
      }

      // Add to the selected group.
      if ($selected_group) {
        $group_storage = $this->entityTypeManager->getStorage('group');
        /** @var \Drupal\group\Entity\GroupInterface $selected_group */
        $selected_group = $group_storage->load($selected_group);
        $selected_group->addContent($node, 'group_node:' . $node->bundle());
      }

      // Restore selected pathauto generation state.
      $node->path->pathauto = $pathauto_state;
    }
  }

  /**
   * Form submit callback for new nodes.
   */
  public function addNewNodeToGroup($form, FormStateInterface $form_state) {
    /** @var \Drupal\node\NodeInterface $node */
    $node = $form_state->getFormObject()->getEntity();
    $selected_group = $form_state->getValue('group_selection');

    // Add to the selected group.
    if ($selected_group) {
      $group_storage = $this->entityTypeManager->getStorage('group');
      /** @var \Drupal\group\Entity\GroupInterface $selected_group */
      $selected_group = $group_storage->load($selected_group);
      $selected_group->addContent($node, 'group_node:' . $node->bundle());
    }

    // Remove any redirects that may have been created in the process.
    redirect_delete_by_path('internal:/' . $node->toUrl()->getInternalPath());
    redirect_delete_by_path('entity:' . $node->getEntityTypeId() . '/' . $node->id());
  }

}

  • Sutharsan committed 3472df1 on 8.x-2.x
    Issue #2882061 by Sutharsan, andrew_tspkh, idebr, mparker17: Allow form...
Sutharsan’s picture

Status: Needs review » Fixed

Let's get this one in and extend with examples and more tests is follow-up issues.

Status: Fixed » Closed (fixed)

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