[2.0.0-beta2] ComponentFormBuilder: make it more dynamic [#3444770]

Problem/Motivation

1: In https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/modules/ui_p...

  public function build() {
    $build = $this->buildComponentRenderable();
    return $build;
  }

  public function blockForm($form, FormStateInterface $form_state) {
    return $this->buildComponentsForm($form_state);
  }

  public function blockSubmit($form, FormStateInterface $form_state) {
    $this->submitComponentsForm($form_state);
  }

$form should be passed to the ComponentFormBuilderTrait methods, and/or the return of those methods should only target a key in $form and not replacing it entirely.

Not taking the provided $form into account may lead to extensibility problems if someone alters the form in other contrib or custom.

I had a problem years ago in a field formatter plugin where I had not called a parent method, it was ok in "manage display" but ot in layout builder because it was preparing additional stuff: https://git.drupalcode.org/project/file_extractor/-/commit/cf3b74dcc1312...

That's why I think not taking the provided $form into account may lead to problems.

2: In https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/Form/Com...

  protected function getComponentConfiguration(): array {
    return $this->configuration['ui_patterns'] ?? [];
  }

  protected function setComponentConfiguration($configuration): void {
    $this->configuration['ui_patterns'] = $configuration;
  }

  ...

I think the "ui_patterns" key should be transformed into an argument, in case a form needs multiple component form elements.

3: In https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/src/Form/Com...

  public function buildComponentRenderable($component_id = NULL, $source_contexts = []) {

This method is used for rendering, the trait is named ComponentFormBuilderTrait, maybe it should be placed in another trait? I understand that it is there because it is using the common method "$this->getComponentConfiguration()".

Proposed resolution

Remaining tasks

API changes

Issue fork ui_patterns-3444770

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3444770-2.0.0-beta1-componentformbuilder-make-merge changes, plain diff MR !180
Check out this branch for the first time

Check out existing branch, if you already have it locally
3444770-2.0.0-beta1-componentformbuilder-make changes, plain diff MR !150
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

2 May 2024 at 07:41

Grimreaper created an issue. See original summary.

Comment #2

pdureau commented 2 May 2024 at 08:03

Title:

ComponentFormBuilder: make it more dynamic

» [2.0.0-alpha3] ComponentFormBuilder: make it more dynamic

Comment #3

pdureau commented 10 June 2024 at 13:55

Title:

[2.0.0-alpha3] ComponentFormBuilder: make it more dynamic

» [2.0.0-beta1] ComponentFormBuilder: make it more dynamic

Comment #4

pdureau commented 4 July 2024 at 10:37

Assigned:

Unassigned

» christian.wiedemann

To have a look

Comment #5

christian.wiedemann commented 14 July 2024 at 10:54

1. This is done wrong in Block Source which we propably remove.
2. I am not sure about that. Actually I think it is good to have allways the same key in every consumer. We may provide a getConfigurationKey() function to overwrite that? Should we? @pdureau @Grimreaper
3. Should we provide an own trait for that? I would vote for that- @pdureau @Grimreaper

Comment #6

christian.wiedemann commented 21 July 2024 at 17:44

Status:

Active

» Needs work

Comment #7

pdureau commented 21 July 2024 at 23:03

Assigned:	christian.wiedemann	» grimreaper
Status:	Needs work	» Postponed (maintainer needs more info)

This is done wrong in Block Source which we propably remove.

We have an issue about that: #3460784: [2.0.0-beta1] Blocks implementing PluginFormInterface don't work with BlockSource
We will not remove BlockSource but introduce a whitelist.

2. I am not sure about that. Actually I think it is good to have allways the same key in every consumer. We may provide a getConfigurationKey() function to overwrite that? Should we? @pdureau @Grimreaper
3. Should we provide an own trait for that? I would vote for that- @pdureau @Grimreaper

No opinion. I assign to @Grimreaper

Comment #8

22 July 2024 at 19:33

Grimreaper opened merge request !150

Comment #9

grimreaper

French

France 🇫🇷

commented 22 July 2024 at 19:36

Assigned:	grimreaper	» Unassigned
Status:	Postponed (maintainer needs more info)	» Needs work

Hi,

1. Now that BlockForm return $form that is passed to it, no problem.
2. I opened a MR with a POC of what I had in mind and with current codebase. I have just edited the files, not tested. This can be used to discuss.
3. Vote for a new trait too.

Comment #10

pdureau commented 22 July 2024 at 19:40

Assigned:

Unassigned

» christian.wiedemann

Comment #11

christian.wiedemann commented 28 July 2024 at 17:05

@Grimreaper I tested your code and add the configuration_id to all places. From my point of view it should work now. (But we realy need e2e tests now)

Comment #12

christian.wiedemann commented 28 July 2024 at 17:05

Status:

Needs work

» Needs review

Comment #13

pdureau commented 28 July 2024 at 17:25

Assigned:

christian.wiedemann

» grimreaper

Florent, can you have a look?

Comment #14

grimreaper

French

France 🇫🇷

commented 30 July 2024 at 07:39

Assigned:	grimreaper	» christian.wiedemann
Status:	Needs review	» Needs work

@Christian.wiedemann,

Thanks for the update.

I put some code review comments.

Also point 3, creating a new trait remains to be done.

Comment #15

christian.wiedemann commented 4 August 2024 at 10:17

@Grimreaper I updated/fixed your comments. We decided to no implement an own trait for rendering because they belong somehow together. Thanks!

Comment #16

christian.wiedemann commented 4 August 2024 at 10:17

Status:

Needs work

» Needs review

Comment #17

pdureau commented 4 August 2024 at 11:11

Assigned:

christian.wiedemann

» grimreaper

Comment #18

grimreaper

French

France 🇫🇷

commented 5 August 2024 at 08:20

Assigned:

grimreaper

» christian.wiedemann

Hi,

@Christian.wiedemann, thanks for your work. Ok for me now.

I have rebased and removed the todo.

You can give a last review in case my rebase broke stuff.

Comment #19

11 August 2024 at 07:40

Christian.wiedemann opened merge request !180

Comment #20

christian.wiedemann commented 11 August 2024 at 07:45

Rebase was the hell. So I created a merge branch and opened a pull request. I tested Views Layouts and Field formatters. We should merge it.

https://git.drupalcode.org/issue/ui_patterns-3444770/-/tree/3444770-2.0....

Comment #21

pdureau commented 11 August 2024 at 08:08

Status:

Needs review

» Needs work

Hi Christian,

So I created a merge branch and opened a pull request.

So, this one? https://git.drupalcode.org/project/ui_patterns/-/merge_requests/180

I am not very comfortable about opening a new branch, instead of doing the rebase. I guess rebase was hell because it was done late. So, I will ash @grimreaper to have a look, because I don't feel competent enough in this subject.

However, before that, it seems phpstan check has failed: https://git.drupalcode.org/project/ui_patterns/-/jobs/2407677

  Line   src/Form/ComponentFormBuilderTrait.php (in context of class                                                          
         Drupal\ui_patterns_field_formatters\Plugin\Field\FieldFormatter\ComponentFormatterBase)                              
  121    Method                                                                                                               
         Drupal\ui_patterns_field_formatters\Plugin\Field\FieldFormatter\ComponentFormatterBase::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                                                
  159    Method                                                                                                               
         Drupal\ui_patterns_field_formatters\Plugin\Field\FieldFormatter\ComponentFormatterBase::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                                                
  192    Method                                                                                                               
         Drupal\ui_patterns_field_formatters\Plugin\Field\FieldFormatter\ComponentFormatterBase::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                                                

  Line   src/Form/ComponentFormBuilderTrait.php (in context of class                          
         Drupal\ui_patterns_views\Plugin\views\row\ComponentRow)                              
  121    Method                                                                               
         Drupal\ui_patterns_views\Plugin\views\row\ComponentRow::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                
  159    Method                                                                               
         Drupal\ui_patterns_views\Plugin\views\row\ComponentRow::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                
  192    Method                                                                               
         Drupal\ui_patterns_views\Plugin\views\row\ComponentRow::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                

  Line   src/Form/ComponentFormBuilderTrait.php (in context of class                              
         Drupal\ui_patterns_views\Plugin\views\style\ComponentStyle)                              
  121    Method                                                                                   
         Drupal\ui_patterns_views\Plugin\views\style\ComponentStyle::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                    
  159    Method                                                                                   
         Drupal\ui_patterns_views\Plugin\views\style\ComponentStyle::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.                                                    
  192    Method                                                                                   
         Drupal\ui_patterns_views\Plugin\views\style\ComponentStyle::getComponentConfiguration()  
         invoked with 1 parameter, 0 required.

Can you run the checks locally before pushing?

Also, there is one commit more to rebase from 2.0.x

Comment #22

pdureau commented 11 August 2024 at 20:42

Title:

[2.0.0-beta1] ComponentFormBuilder: make it more dynamic

» [2.0.0-beta2] ComponentFormBuilder: make it more dynamic

Too late for beta1. Moved to beta2

Comment #23

pdureau commented 13 August 2024 at 08:37

Priority:

Normal

» Major

Comment #24

christian.wiedemann commented 5 September 2024 at 19:27

Status:

Needs work

» Needs review

Comment #25

christian.wiedemann commented 5 September 2024 at 19:28

I rebased to the latest 2.0.x. Check the merge branch.

Comment #26

pdureau commented 6 September 2024 at 09:09

Assigned:

christian.wiedemann

» grimreaper

Comment #27

6 September 2024 at 09:24

grimreaper committed 66fb5737 on 2.0.x authored by christian.wiedemann

Issue #3444770 by christian.wiedemann, grimreaper, pdureau: [2.0.0-beta2...

Comment #28

grimreaper

French

France 🇫🇷

commented 6 September 2024 at 09:24

Assigned:	grimreaper	» Unassigned
Status:	Needs review	» Fixed

Merged.

Thanks!

Comment #29

pdureau commented 15 September 2024 at 19:27

Status:

Fixed

» Closed (fixed)