October: disabled:true on form recordpicker results in integrity constraint violation

Created on 14 Dec 2018  Â·  4Comments  Â·  Source: octobercms/october

Expected behavior

Adding disabled: true to a recordpicker in fields.yaml should not send the (unchanged) value to the FormController.

Actual behavior

The unchanged value is sent to the FormController, where it is set to null (because it is disabled), resulting in an attempt to override the record id with null.

Reproduce steps

Make a fields.yaml file with a recordpicker element and set disabled: true.

October build

434

It seems that this issue lies in the getSaveData() method in the Backend\Widgets\Form class. Adding the following code into the foreach on line 1166 appears to fix the issue but I haven't been able to run tests to confirm.

if ((isset($widget->config->disabled) && $widget->config->disabled) ||
    (isset($widget->config->hidden) && $widget->config->hidden)) {
        continue;
}
Completed Bug

Most helpful comment

Ran this against the existing test suite without errors. I’m happy to submit a PR!

Sent from my iPhone

On 14 Dec 2018, at 5:18 pm, Luke Towers notifications@github.com wrote:

In theory that should work, and that is already in place for regular fields, so I don't see why it wouldn't. Please test it, perhaps @w20k can take a look as well, and then submit a PR

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

All 4 comments

In theory that should work, and that is already in place for regular fields, so I don't see why it wouldn't. Please test it, perhaps @w20k can take a look as well, and then submit a PR

Ran this against the existing test suite without errors. I’m happy to submit a PR!

Sent from my iPhone

On 14 Dec 2018, at 5:18 pm, Luke Towers notifications@github.com wrote:

In theory that should work, and that is already in place for regular fields, so I don't see why it wouldn't. Please test it, perhaps @w20k can take a look as well, and then submit a PR

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Sorry, about that, mobile glitch :(

Side note: If you could provide unit tests @tmus, would be awesome!

Go for it @tmus. No worries @w20k, I've done that more times than I can count

Was this page helpful?
0 / 5 - 0 ratings