Skip to content

Add default for ShardPreferences->showCheckboxes #5016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maccabeelevine wants to merge 3 commits into from
Closed

Add default for ShardPreferences->showCheckboxes #5016

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5545e2
Add default for ShardPreferences->showCheckboxes
maccabeelevine Jan 16, 2026
c21d523
Default to null
maccabeelevine Jan 20, 2026
944d119
Merge branch 'dev' into shard-showcheckboxes-default
demiankatz Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion module/VuFind/src/VuFind/Search/Solr/Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public function __construct(ConfigManagerInterface $configManager)
$this->defaultSelectedShards = array_keys($this->shards);
}
// Apply checkbox visibility setting if applicable:
if (null !== ($visibleCheckboxes = $this->searchSettings['ShardPreferences']['showCheckboxes'])) {
if (null !== ($visibleCheckboxes = $this->searchSettings['ShardPreferences']['showCheckboxes'] ?? false)) {
Copy link
Copy Markdown
Member Author

@maccabeelevine maccabeelevine Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@demiankatz demiankatz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member Author

@maccabeelevine maccabeelevine Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@demiankatz demiankatz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Copy Markdown
Member

@demiankatz demiankatz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@maccabeelevine maccabeelevine Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@demiankatz demiankatz Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

$this->visibleShardCheckboxes = $visibleCheckboxes;
}
}
Expand Down