GridField and GridState_component bug

[t3hn0]

Hi!

I'm not sure if it's intentional but if you construct GridField through injector like in example bellow:

GridField::create('MyRelation')
    ->setTitle('My Title')
    ->setList($this->MyRelation())
    ->setConfig( GridFieldConfig_Base::create() )

then GridfieldState_Component is not set on GridField config. GridState_Component is added to current GridFieldConfig in GridField constructor, but if we redefine GridFieldConfig with GridField::setConfig, GridState_Component is never set. That can cause JavaScript error while using GridFieldSortableRows and probably others.

Shouldn't you merge these lines

public function __construct($name, $title = null, SS_List $dataList = null, GridFieldConfig $config = null) {
    ...
    $this->setConfig($config ?: GridFieldConfig_Base::create());

    $this->config->addComponent(new GridState_Component());
    $this->state = new GridState($this);        
    ...
}

and put it all in GridField::setConfig method?

[robbieaverill]

Hey @t3hn0 - which version of SilverStripe are you running? Looks like 3.x, just want to confirm.

Quick investigation shows that you can create a GridField with a _Base config by either omitting the config from the constructor arguments and allowing it to be created by default, by providing it to the constructor or by setting it after the construct has run. The first two of those scenarios will also set a GridState_Component into the config, regardless of whether you've passed it in or allowed a default config to be created.

I think the point of this issue is that these two scenarios produce a different config - one with a GridState_Component and one without:

// Config (Base) contains a grid state component
$gridField = GridField::create('Grid');

// Config (Base) does not contain a grid state component
$gridField = GridField::create('Grid')
    ->setConfig(GridFieldConfig_Base::create());

My impression of the purpose of the code below (from the GridField constructor) is to add a GridState_Component to all GridFieldConfig's:

$this->setConfig($config);

$this->config->addComponent(new GridState_Component());
$this->state = new GridState($this);

If this is causing trouble, then perhaps the bottom two lines could be moved into GridField::setConfig to ensure they're also called whenever you set a new config class via the public API?

[t3hn0]

Hey @robbieaverill it was version 3.5.

It's not only GridFieldConfig_Basethat's affected but as you guessed, every config which is set with GridField::setConfig is missing GridState_Component.

Suggested update should be ok.

[robbieaverill]

Hey @t3hn0 - cool. Would you be willing to submit a patch for this?

[tractorcow]

I agree with @t3hn0. The original behaviour (assign state on constructor) is flawed, and as this issue demonstrates, isn't reliable.

I would also agree with moving to setConfig(), and having the constructor call this. Maybe have this code check to see if one such config already exists, and add it only conditionally.

[t3hn0]

@robbieaverill ok this is my first try, so please don't kill me if I got it wrong :) I created pull request #6896