Add default for ShardPreferences->showCheckboxes

[maccabeelevine]

searches.ini says

;[ShardPreferences]
; This setting controls whether or not to display checkboxes to allow the user to
; select which shard(s) to search (default if commented out = false)
;showCheckboxes = true

But there is no actual default; if IndexShards and SharePreferences are enabled but this param is not defined, an error is thrown.

[maccabeelevine]

To be clear, I haven't tested this beyond getting past this particular error, and I have not configured VuFind to actually use shards correctly. Just trying to test MultiIndexListener as part of #4991.

[maccabeelevine]

And sorry, as I'm looking at this, maybe false is not good enough given null !==. But that's what searches.ini claims...

[demiankatz]

Thanks, @maccabeelevine -- see below for some suggestions (and history of where this bug came from).

[demiankatz]

This should be null rather than false; this way, the setting overrides whatever property value is default in the class, rather than forcing a particular default value.

Suggested change

	if (null !== ($visibleCheckboxes = $this->searchSettings['ShardPreferences']['showCheckboxes'] ?? false)) {
	if (null !== ($visibleCheckboxes = $this->searchSettings['ShardPreferences']['showCheckboxes'] ?? null)) {

This was not a problem until #4353, which refactored the code to use arrays instead of config objects and accidentally omitted a fallback here.

I'd suggest making this a bug fix PR targeted against the release-11.0 branch. If you have any trouble setting that up, please let me know and I'll be happy to help!

[maccabeelevine]

I changed the fallback to null as suggested, but there's still a problem here, with the language in searches.ini

; ... (default if commented out = false)
; showCheckboxes = true

Because those two lines, both specifying booleans, imply (IMHO) that you could declare

showCheckboxes = false

and get the default behavior. But you can't, because of !== null. It would be fine if the check were != null.

This is perhaps made worse because of the bug (that no default was defined), so it's possible people have actually declared something like that.

I can change the comment line to (default if commented out = don't show checkboxes) but I still think there's a misleading expectation when the valid options are true vs null.

All that said, I'm being persnickety, and this is probably a larger question than this one setting.

[demiankatz]

@maccabeelevine, I believe that the documentation remains accurate from the end-user's perspective. The setting is initialized to false in the property declaration. The config processing code says "only override the property's default with the value from the config if there IS a value in the config." If the user leaves the setting out, the setting will remain false; if the user sets the setting to false, we'll harmlessly overwrite the false with false. Even with the bug in place, I suspect that in non-strict mode, PHP will evaluate the expression to null, which will still get interpreted as false, so the setting always defaults to false.

Am I missing/misunderstanding something?

[demiankatz]

Odd: when I click the link I placed above, my highlighted lines don't highlight correctly unless I open it in a new tab. Some new bit of GitHub weirdness.

[maccabeelevine]

if the user sets the setting to false, we'll harmlessly overwrite the false with false.

In my testing, if the user sets the setting to false, the config is read as "" (empty string). (Is this another side effect of the Laminas config deprecation?) So then $this->visibleShardCheckboxes is assigned to "" since since "" !== null. So that violates the PHPdoc typing of that parameter, and the return type of showShardCheckboxes(). But in practice it makes no difference since the only template using that function will be fine with an empty string. So no problem that you merged it, but I still think there's a bigger issue here.

[demiankatz]

That has always been the behavior -- the ini parser returns truthy or falsey values, but they're not true Booleans. It would be wise to add type casting to make things more predictable, but functionally it doesn't make a difference in the current code. (It may begin to make more difference in future as we continue to get more strict about typing everything).

[demiankatz]

In any case, I have backported your fix to the release-11.0 branch since I am confident it is an improvement. I'm happy to discuss the documentation aspect further, and we can address that in a follow-up if we agree there is a need.