getkirby / kirby

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

Structure field : Conflicting "fields" attribute/prop #4107

Closed Daandelange closed 2 years ago

Daandelange commented 2 years ago

Description

The structure field itself works great. But extending it, some unexpected behaviour happens. Context: I've been extending a structure field by overriding its fields prop to a custom value, to dynamically provide one column per language. Result: The structure throws an error because it tries to use the fields from the blueprint [aka $field->attr['fields']]. Undefined index: fields in file: /starterkit/kirby/config/fields/structure.php line: 162

Expected behavior
That the underlying structure field is able to use my changed fields prop. (Or rather expect the structure field not to have a fields prop, so I'm unable to over-ride it ? ).

Notes

Solving
Changing $this->attrs['fields'] to $this->fields solves the issue in kirby/config/fields/structure.php. https://github.com/getkirby/kirby/blob/351b6348348cf78aed9a65920358caf218473102/config/fields/structure.php#L163 Everywhere else, the structure field already uses the parsed props via $this->propName.
Only here the code directly uses the popName from the attrs. (is there a reason for this? do other fields have similar bottlenecks?) This solution would definitely make re-using the structure field easier.

To reproduce

Edit: Whoops, I pasted the "fields part" in the js snippet. The code should work now, with clarified instructions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

Daandelange commented 1 year ago

Hello, I'd like to re-open this tiny issue, I'm still running into it (while there are multiple ways of bypassing this).

So 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 instead of $this->attrs['fields'] so 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.

Kirby::plugin('kirbybugs/staticstructure', [
  'extends' => 'structure',
  'props' => [
    // Make fields static or whatever
    'fields' => function($fields=[]){
      //$this->attrs['fields']=[...]; // 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' => [
    // Overrides structure-field::methods::form() which grabs the form from $this->attrs['fields'] which we can't override.
    // 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
        'values' => $values,
        'model'  => $this->model
      ]);
    },
  ],
]);