Deletion warning when adding options to Select Options field

[adrianbj]

Short description of the issue

@ryancramerdesign - this is a weird one. Originally reported by @Toutouwai here: https://github.com/adrianbj/TracyDebugger/issues/47

When Tracy's RequestInfo panel is loaded, you get a warning about deleting options when adding options to a new Select Options field that does not yet have any options.

Turns out it's because the RequestInfo panel calls $field->getExportData() and if I remove that call, the problem goes away.

Optional: Screenshots/Links that demonstrate the issue

For some reason on my production servers I see the same issue as @Toutouwai, but on my local dev, I actually get this error stack:

ProcessWire\WireException: value must be string in /Users/ajones/Sites/ecoreportcard/wire/modules/Fieldtype/FieldtypeOptions/SelectableOptionManager.php:257
Stack trace:
#0 /Users/ajones/Sites/ecoreportcard/wire/modules/Fieldtype/FieldtypeOptions/SelectableOptionManager.php(419): ProcessWire\SelectableOptionManager->optionsStringToArray(NULL)
#1 /Users/ajones/Sites/ecoreportcard/wire/modules/Fieldtype/FieldtypeOptions/SelectableOptionConfig.php(86): ProcessWire\SelectableOptionManager->setOptionsStringLanguages(Object(ProcessWire\Field), Array, false)
#2 /Users/ajones/Sites/ecoreportcard/wire/modules/Fieldtype/FieldtypeOptions/SelectableOptionConfig.php(202): ProcessWire\SelectableOptionConfig->process(Object(ProcessWire\InputfieldTextarea))
#3 /Users/ajones/Sites/ecoreportcard/wire/modules/Fieldtype/FieldtypeOptions/FieldtypeOptions.module(485): ProcessWire\SelectableOptionConfig->getConfigInputfields()
#4 /Users/ajones/Sites/ecoreportcard/wire/core/Wire.php(383): ProcessWire\FieldtypeOptions->___getConfigInputfields(Object(ProcessWire\Field))
#5 /Users/ajones/Sites/ecoreportcard/wire/core/WireHooks.php(823): ProcessWire\Wire->_callMethod('___getConfigInp...', Array)
#6 /Users/ajones/Sites/ecoreportcard/wire/core/Wire.php(450): ProcessWire\WireHooks->runHooks(Object(ProcessWire\FieldtypeOptions), 'getConfigInputf...', Array)
#7 /Users/ajones/Sites/ecoreportcard/wire/core/Fieldtype.php(347): ProcessWire\Wire->__call('getConfigInputf...', Array)
#8 /Users/ajones/Sites/ecoreportcard/wire/modules/Fieldtype/FieldtypeOptions/FieldtypeOptions.module(408): ProcessWire\Fieldtype->___exportConfigData(Object(ProcessWire\Field), Array)
#9 /Users/ajones/Sites/ecoreportcard/wire/core/Wire.php(386): ProcessWire\FieldtypeOptions->___exportConfigData(Object(ProcessWire\Field), Array)
#10 /Users/ajones/Sites/ecoreportcard/wire/core/WireHooks.php(823): ProcessWire\Wire->_callMethod('___exportConfig...', Array)
#11 /Users/ajones/Sites/ecoreportcard/wire/core/Wire.php(450): ProcessWire\WireHooks->runHooks(Object(ProcessWire\FieldtypeOptions), 'exportConfigDat...', Array)
#12 /Users/ajones/Sites/ecoreportcard/wire/core/Field.php(482): ProcessWire\Wire->__call('exportConfigDat...', Array)
#13 /Users/ajones/Sites/ecoreportcard/site/assets/cache/FileCompiler/site/modules/TracyDebugger/panels/RequestInfoPanel.php(110): ProcessWire\Field->getExportData()
#14 /Users/ajones/Sites/ecoreportcard/site/assets/cache/FileCompiler/site/modules/TracyDebugger/tracy-2.7.x/src/Tracy/Bar/Bar.php(150): RequestInfoPanel->getPanel()
#15 /Users/ajones/Sites/ecoreportcard/site/assets/cache/FileCompiler/site/modules/TracyDebugger/tracy-2.7.x/src/Tracy/Bar/Bar.php(122): Tracy\Bar->renderPanels('-r0')
#16 /Users/ajones/Sites/ecoreportcard/site/assets/cache/FileCompiler/site/modules/TracyDebugger/tracy-2.7.x/src/Tracy/Bar/Bar.php(94): Tracy\Bar->renderHtml('redirect', '-r0')
#17 /Users/ajones/Sites/ecoreportcard/site/assets/cache/FileCompiler/site/modules/TracyDebugger/tracy-2.7.x/src/Tracy/Debugger/Debugger.php(293): Tracy\Bar->render()
#18 [internal function]: Tracy\Debugger::shutdownHandler()
#19 {main}

Basically the issue seems to be that calling $field->getExportData() before / during the setting of the selectable options messes with $deletedOptionIDs, which on quick inspection is originally coming from the array_merge() here: https://github.com/processwire/processwire/blob/51629cdd5f381d3881133baf83e1bd2d9306f867/wire/core/Field.php#L457-L460

Please let me know if there is anything additional we can provide, but if you follow the steps in @Toutowai's screencast above, you should see either the described behavior, or the error in the Tracy debug bar.

[adrianbj]

FYI - here is the block of code from the RequestInfo panel that is the trigger of the problem: https://github.com/adrianbj/TracyDebugger/blob/baa07b186f3ac92e9062ec9244ff89d040961b6d/panels/RequestInfoPanel.php#L109-L119

[BernhardBaumrock]

I can confirm this problem, thx for reporting!

[adrianbj]

@ryancramerdesign - any chance of taking a look at this please. I've tried to code a workaround in Tracy without any success - I think this needs to be fixed in the PW core.

[adrianbj]

@ryancramerdesign - this also affects modifying options as well. I am also seeing SQL integrity violations which might also be related to this. Can you at least let me know if you can or can't reproduce please?

[matjazpotocnik]

@adrianbj There have been some changes in the getExportData() method, could you verify if this issue still persists?

[adrianbj]

I am still seeing that error stack posted above.

Note that initially, $value is null before it finally becomes the correct string.

[matjazpotocnik]

I can't replicate this, unfortunately. I'm dumping value, but get this when I create a new field and save it:

After adding another option and saving it again:

[adrianbj]

Please make sure this option is checked in Tracy's settings.

[matjazpotocnik]

Aha, now I can replicate the null value, but I still don't get any errors. The question is why is there null in the first place.

[matjazpotocnik]

I know where null is coming from and fixing that would be easy, but the main issue remains...

[matjazpotocnik]

It looks like calling $field->getExportData() from the request panel is triggering getConfigInputfields() in SelectableOptionConfig.php which in turn calls process() method, so the process() method is called twice. A warning about deleting options is issued because the post request for the _options is the same (and not null), but the data in the database is not. For example, when you enter option One and submit the form, it's saved in the database as 1=One (option gets an ID). As both values differ, this is an indication that you want to delete the option.

So, how to solve this? Don't know for sure, but looks like processing a form (post values) inside getConfigInputfields() is causing troubles. I fixed this like this, in process() method:

replaced

        if(!is_null($value)) {
            // _options has been posted

with

        if(!is_null($value) && $session->get($ns, 'processed') != '1') {
            // _options has been posted
            $session->set($ns, 'processed', '1');

and

        } else {
            // options not posted, check if there are any pending session activities

with

        } else {
            // options not posted, check if there are any pending session activities
            $session->remove($ns, 'processed');

This is most likely not the correct way, but it demonstrates the issue - moving the process() call out of getConfigInputfields() would be better. I hope it helps, @ryancramerdesign

Regarding null value, I added if($input->post($key) === null) continue; before $valuesPerLanguage[$language->id] = $input->post($key); in process() method.

[Toutouwai]

@ryancramerdesign, could you please take a look at this issue? It's been so long that I forgot about it and rediscovered it all over again. The issue would need to be solved before this request can be viable.