Contrib.social

Refactor \Drupal\eca\Plugin\FormFieldPluginTrait::jumpToFirstFieldChild

Open on Drupal.org →
Created on 9 February 2023, almost 2 years ago
Updated 12 June 2024, 5 months ago

Problem/Motivation

The method \Drupal\eca\Plugin\FormFieldPluginTrait::jumpToFirstFieldChild needs a review and, potentially, some refactoring.

The argument $element is handed over as a reference. Inside the method, that variable is being modified, and it is returned in the end. So, the calling function has the forwarded variable being modified and receives the very same object also as a return value. In 🐛 eca_form: Ajax handler does not work for checkboxes in content entity forms Fixed where this was identified first, it was argued that Drupal's NestedArray::getValue did the same, but that's not entirely true because that works with a $ref variable internally and returns that, which is not the same as the variable that's being referenced in the method call.

Where the ECA function does $element = &$element[....]; and continues working with that variable, the NestedArray function does $ref = &$array; and continues working with the local variable.

Referencing a variable in a method call and also returning that variable, is redundant.

Finally, TBH I don't understand what all those referencing is there for. In the context of that method, there are 5 references, and I'm not getting why. I'm sure there are good reasons. First, I need to learn and understand them and second, we need to document them so that we can recall years later, when it happens that somebody needs to fix a bug here and there.

As this is perfectly correct working code, as far as we can tell, there is no reason to back port the potential refactoring. The whole purpose of this exercise is only to get better maintainable code for the future.

📌 Task
Status

Active

Version

2.1

Component

Code

Created by

🇩🇪Germany jurgenhaas Gottmadingen

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

  • Issue created by @jurgenhaas
  • Comment almost 2 years ago →
    🇩🇪Germany mxh Offenburg

    Referencing a variable in a method call and also returning that variable, is redundant.

    Yes, it's redundant and could be reduced in the way that it returns the reference to the first child, but won't change the passed reference.
    To achieve that, we could adopt the behavior from Drupal's NestedArray function:

    public static function &getValue(array &$array, array $parents, &$key_exists = NULL) {
    $ref = &$array;

    .. where it's defining a new local reference variable and finally returns this one.

    All current usages already use the returned reference. We should cover this refactor with a unit test to make sure, that this method exactly behaves as expected.

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

    @mxh is that something for you? If not, please unassign yourself again.

  • Issue was unassigned.
  • Comment over 1 year ago →
    🇩🇪Germany mxh Offenburg
  • Assigned to jurgenhaas
  • Comment 11 months ago →
    🇩🇪Germany jurgenhaas Gottmadingen
  • Issue was unassigned.
  • Comment 6 months ago →
    🇩🇪Germany jurgenhaas Gottmadingen
  • Comment 5 months ago →
    🇩🇪Germany jurgenhaas Gottmadingen
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024