Contrib.social

eca_form: Form field actions are not working well for multi-property fields

Open on Drupal.org →
Created on 7 March 2023, over 1 year ago
Updated 16 March 2023, over 1 year ago

Problem/Motivation

We're currently creating many ECA process configurations that extend and change the behavior of forms. Using form field actions, like "Form field: set access", works for entity fields that have one property (e.g. text fields that only have a "value" property). However such actions currently don't work well for fields having multiple properties.

Example fields are date range and link fields. When trying to hide such a field by only specifying the machine name of the field, then the action only hides one property in the form. For example on date range fields, the start date ("value") is being hidden, but the end date ("end_value") is still visible. Even when hiding both properties explicitly using two actions (one for "myfield.0.value" and another one for "myfield.0.end_value"), then both fields are hidden, but still the HTML details element (fieldset) is being shown, although it's empty.

The expectation would be that the whole field is being hidden.

Steps to reproduce

  • Have a content type, add a date range field
  • Create an ECA process config, react upon "Process form" event for entity type "node", bundle "your bundle"
  • Add the action "Form field: set access" as a successor to the event, specify the machine name of the date range field (without properties)
  • Look into the node form: The date range field still shows up with the end date widget.

Proposed resolution

Currently, all form field actions share the same trait Drupal\eca\Plugin\FormFieldPluginTrait. The trait jumps to the first child when the targeted element has no render element type specified:

<?php
  protected function &getTargetElement(): ?array {
    $nothing = NULL;
    if (!($form = &$this->getCurrentForm()) || !($name_array = $this->getFieldNameAsArray())) {
      return $nothing;
    }

    $key = array_pop($name_array);
    foreach ($this->lookupFormElements($form, $key) as &$element) {
      if (empty($name_array) || (isset($element['#parents']) && array_intersect($name_array, $element['#parents']) === $name_array) || (isset($element['#array_parents']) && array_intersect($name_array, $element['#array_parents']) === $name_array)) {
        // Found an element due to defined parents or array_parents.
        if (!isset($element['#type']) && Element::children($element)) {
          // Some field widgets are additionally nested. And since we need
          // a form field element here, catch the first defined child element.
          $element = &$this->jumpToFirstFieldChild($element);
        }
// ...
?>

Maybe this place can be adjusted in the way that it's able to recognize when dealing with a multi-property field.

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Fixed

Version

1.2

Component

Miscellaneous

Created by

🇩🇪Germany mxh Offenburg

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @mxh
  • Comment over 1 year ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    Not sure about all the details involved, I was just thinking about how I would hide such a field when writing a hook in PHP. Isn't it possible there to set "#access" => FALSE on the top level of a field widget? In a case of a date range field, that would be the mentioned fieldset and by that, the whole widget would be excluded from the form and not only the value field(s) contained inside.

    Isn't it possible to get to something like that in the Trait above?

  • Comment over 1 year ago →
    🇩🇪Germany mxh Offenburg

    Isn't it possible there to set "#access" => FALSE on the top level of a field widget?

    I think this was implemented in the very first beginning of eca_form. Then we realized that it wasn't sufficient and ended up with the current logic, that tries to always provide a "real" field element. The fieldset for the date range field is not a "real" field element, as it's just a theme function (not a render element plugin).

    The thing is, that the current mechanic seems to work well now over all form field actions, but only for "simple" single-column fields. Changing the logic at this point will probably change the behavior of existing process configurations, which should be avoided. I'd like to find a least-invasive approach for this problem.

  • Comment over 1 year ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    That's usually OK. However, as we will be going for 2.0 as the next release, a breaking change would be acceptable, if we provided a reasonable change record and let people know what to look out for to get their existing models updated.

    It feels like long term, it would be preferable, to do a restart on this now. I remember some other issues where we touched that trait. It could well be kind of a time-bomb, so I'd be OK to go for a "real" fix for 2.0

  • Comment over 1 year ago →
    🇩🇪Germany mxh Offenburg

    There is a reason why the trait is implemented the way it is right now. And as mentioned in #3, the mechanic works well for single-column fields. I don't see a justification for a "restart", and I don't know how to "restart" this. We've come a long way to make this work, and a "restart" will more likely just break existing things. I vote for extending the existing logic to be more sensitive regarding multi-column fields.

  • Assigned to mxh
  • Comment over 1 year ago →
    🇩🇪Germany mxh Offenburg
  • @mxh opened merge request.
  • Issue was unassigned.
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇩🇪Germany mxh Offenburg

    Turns out the affected components are flagging actions, where it makes sense to not automatically jump to a field element.

  • Status changed to RTBC over 1 year ago
  • Comment over 1 year ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    Looking good.

  • Comment over 1 year ago →
    System Message
  • Comment over 1 year ago →
    System Message
  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    🇩🇪Germany jurgenhaas Gottmadingen

    Merged and backported to 1.1

  • Comment over 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024