getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Structure-field : Use props.fields instead of attrs.fields for building the Form. #4909

Open Daandelange opened 1 year ago

Daandelange commented 1 year ago

Hello, I'm reopening a ticket for #4107 as it got lost in stale issues.

Description

The structure field currently uses $this->attrs['fields'] to build the form. I think it should use $this->props['fields']. https://github.com/getkirby/kirby/blob/3e5899b628a2b59c1833a8d79eda292a7fe06843/config/fields/structure.php#L176

The purpose of my request is to make the structure-field better re-useable. Unless you really need to use the user-provided attributes from the blueprint, I think it makes more sense to use $this->props['fields'] (or $this->fields to accept computed fields too) instead of $this->attrs['fields'] so that the StructureField is easier to extend, with less redondant code. (see snippet below)

If other people are running into this issue, the easiest way I found is to add a form to your extended-structure-field's methods. This will also ensure compatibility with current Kirby version (3.8.3) and before.

// See comments for different workarounds.
Kirby::plugin('kirbybugs/staticstructure', [
  'extends' => 'structure',
  'props' => [
    // Make fields static or whatever
    'fields' => function($fields=[]){
      //$this->attrs['fields']=[...]; // Solution 2 : Here you could eventually force the prop into the attrs so that the native methods.form picks the right one.
      return [ 'test'=>['label'=>'TestField','name'=>'test', 'type'=>'text'] ];
    },
  ],
  'methods' => [ // Solution 1
    // Overrides structure-field::methods::form() which grabs the form from $this->attrs['fields'] instead of the props.
    // Until https://github.com/getkirby/kirby/issues/4107 is merged, and after, this will still be needed this for compatibility with older kirby versions.
    'form' => function (array $values = []) { 
      return new Form([
        'fields' => $this->fields, // <-- only line changed compared to native function. Fix proposal.
        'values' => $values,
        'model'  => $this->model
      ]);
    },
  ],
]);

Expected behavior
I assume/expect that the panel Form is build with my modified props when extending the structure-field.

Proposed solution

Changing $this->attrs['fields'] to $this->fields solves the issue in kirby/config/fields/structure.php.

To reproduce

Use the snippet above without the 'methods' part, put the field in a blueprint and go to the corresponding panel page. See the structure form fields corresponding to the page blueprint fields (user-fields); not the static fields injected in `props['fields']. The template code works as expected however.

Your setup

Kirby Version At least 3.6 -> 3.8.3

distantnative commented 1 year ago

See https://github.com/getkirby/kirby/pull/4936, there is a too big risk of side effects and regressions with the current architecture.

Daandelange commented 1 year ago

Ok, thanks for the feedback and investigation. ( It seems I'm often running in edge-cases-needing-deeper-refactoring ^_^ )

So one more question: I don't understand the refactoring reasons and regression stuff behind this, but which one of the 2 solutions would be better to use ? What's the risk of using Solution 1 and what should I double-check to ensure it's working correctly ?