Consider renaming the 'inputs' column on ComponentTreeItem to 'input'

Created on 21 May 2025, 15 days ago

Overview

See this discussion

Proposed resolution

User interface changes

๐Ÿ“Œ Task
Status

Active

Version

0.0

Component

Data model

Created by

๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

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

Merge Requests

Comments & Activities

  • Issue created by @larowlan
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I cross-posted with you while you created this. I just wrote:

    I want this to be consistent with ComponentSourceInterface::getExplicitInput() et cetera.

    That method and all others on the source is specifying the singular โ€œinputโ€.

    But youโ€™re right that it could be argued that the plural is better.

    So: would you prefer keeping this as-is and doing a follow-up to update the source plugins to match? ๐Ÿค”

    Curious what you think ๐Ÿค“ Getting mental models and naming right is hard!

    โ€ฆ because I agree with your statement:

    there can still be multiple 'inputs' because of multiple props - I think the naming is appropriate - if we need to change it to 'input' could we do that in a follow up?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I am leaning towards . Because @larowlan already articulated on the discussion he linked why he feels it should be plural.

    Why is it singular now?

    The reason they are currently singular:

       * @param array $inputs
       *   Component inputs โ€” both implicit and explicit.
    โ€ฆ
       */
      public function renderComponent(array $inputs, string $componentUuid, bool $isPreview): array;
    

    which then results in this in the block source plugin:

      public function renderComponent(array $inputs, string $componentUuid, bool $isPreview = FALSE): array {
        $block = $this->getBlockPlugin();
        foreach ($inputs[self::EXPLICIT_INPUT_NAME] ?? [] as $key => $value) {
    โ€ฆ
    

    and this in the sdc source plugin:

      public function renderComponent(array $inputs, string $componentUuid, bool $isPreview = FALSE): array {
        return [
          '#type' => 'component',
          '#component' => $this->configuration['local_source_id'],
          '#props' => ($inputs[self::EXPLICIT_INPUT_NAME] ?? []) + [
    โ€ฆ
    

    Proposal

    It'd be much clearer to have multiple parameters instead ( ๐Ÿ“Œ Handle block contexts Active could add another parameter to ::renderComponent() if needed).

    So I propose:

    1. every single method and parameter on ComponentSourceInterface should NOT say "input" or "inputs", but "explicit_inputs"
    2. this means GeneratedFieldExplicitInputUxComponentSourceBase::EXPLICIT_INPUT_NAME and \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::EXPLICIT_INPUT_NAME become obsolete

    See outline in the attached patch.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia libbna New Delhi, India

    Hi @wim leers,
    I was considering working on this issue and, before proceeding, I wanted to confirm my understanding based on your comments. From what I gather, the goal is to rename "explicit_input" to "explicit_inputs" specifically in the ComponentSourceInterface.

    Could you please confirm if this change should be limited to the interface only, or should I also update the related implementations accordingly?

    Additionally, should I go ahead and create a merge request for the patch, or would you suggest any prerequisites before that?

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    or should I also update the related implementations accordingly?

    Yes, please! ๐Ÿ™

    should I go ahead and create a merge request for the patch

    Yes! ๐Ÿ˜Š

    Thank you!

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia libbna New Delhi, India

    Thank you for confirming. Assigning the issue to myself.

  • Merge request !1064changed input/inputs to explicit_inputs. โ†’ (Open) created by libbna
  • Pipeline finished with Failed
    13 days ago
    Total: 339s
    #504490
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia libbna New Delhi, India

    There is one more file,

    GeneratedFieldExplicitInputUxComponentSourceBase.php

    , under the same interface in which $inputs is used. Should I update the code there as well?

    public function getDefaultExplicitInput(): array {
        $inputs = [];
        foreach (array_keys($this->configuration['prop_field_definitions']) as $prop_name) {
          assert(is_string($prop_name));
          $inputs[$prop_name] = $this->getDefaultStaticPropSource($prop_name)->toArray();
        }
        return $inputs;
      }
    
  • Pipeline finished with Failed
    13 days ago
    Total: 328s
    #504493
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia libbna New Delhi, India
  • Pipeline finished with Failed
    13 days ago
    Total: 373s
    #504497
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Thanks so much for getting this going! ๐Ÿ˜Š๐Ÿ™

    Unfortunately, the phpcs and phpstan CI jobs are failing, as are the tests. That means this is not quite ready for review yet.

    Any chance you could look into fixing those failures?๐Ÿคž

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia libbna New Delhi, India

    Sure, I will do that. Also, I'm not able to run drush cr because I'm getting the following error:

    'Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent' does not implement the methods 'requiresExplicitInputs', 'getDefaultExplicitInputs', 'getExplicitInputs', 'explicitInputsToClientModel'.

    I believe I need to define all the required methods in every file where the interface is implemented.

  • Pipeline finished with Failed
    13 days ago
    Total: 784s
    #504724
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia libbna New Delhi, India

    I have resolved the PHPCS and PHPStan issues and unassigned myself so that others can pick it up and work on the test issues.

  • Pipeline finished with Failed
    13 days ago
    Total: 703s
    #504744
  • First commit to issue fork.
  • Pipeline finished with Failed
    13 days ago
    Total: 627s
    #504791
  • Pipeline finished with Failed
    13 days ago
    Total: 426s
    #504815
  • Pipeline finished with Failed
    13 days ago
    Total: 787s
    #504829
  • Pipeline finished with Failed
    13 days ago
    Total: 730s
    #504850
  • Pipeline finished with Running
    13 days ago
    #504952
  • Pipeline finished with Failed
    13 days ago
    #504969
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Nice progress here! ๐Ÿคฉ

    I think

    Exception: Warning: Undefined array key "resolved"
    Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase->hydrateComponent()() (Line: 200)
    

    explains the majority of the test failures, so I recommend looking at that first :)

  • First commit to issue fork.
Production build 0.71.5 2024