Skip to content
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.

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

[FR] Allow the `defaultSearchTermOptions` config setting to merge with the default array #2737

Closed
lindseydiloreto opened this issue Apr 12, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@lindseydiloreto
Copy link
Contributor

commented Apr 12, 2018

The default defaultSearchTermOptions value is this...

'defaultSearchTermOptions' => [
    'attribute' => null,
    'exact' => false,
    'exclude' => false,
    'subLeft' => false,
    'subRight' => true
],

The only one I want to change is subLeft. But if I add this to my config file...

'defaultSearchTermOptions' => ['subLeft' => true]

... it breaks the rest of the values in that array.

Can we have it so the config-specified settings get merged with the default settings? I'd rather not re-declare all five array keys when I only need to change a single one.

@lindseydiloreto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

Discussed further on Slack...

possible solution?

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

We can’t merge this one setting without merging all arrays, and that's not always going to be desired.

@lindseydiloreto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

So we have to recreate the entire 5 key array in our config settings, even if we're just changing a single value? That's unfortunate.

Other than "mass merging", is there any other way to address this shortcoming? In that Slack conversation, we discussed the possibility of defaultSearchTermOptions drawing from its default values to replace any null values.

That would be something that happens when the search is executed, not when the config array is initially compiled.

@angrybrad

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Not saying it's the best solution, but we addressed this with the allowedFileExtensions config setting by adding the extraAllowedFileExtensions config setting that gets merged in.

It's much more pronounced there because there are so many items in the allowedFileExtensions setting that you'd need to duplicate.

@lindseydiloreto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

Hmm, that solution may be extreme for defaultSearchTermOptions. It makes sense for file extensions because there are so damn many of them (and who knows what people will try to add).

It looks like defaultSearchTermOptions only comes into play once (okay twice) in a very narrow neighborhood...

https://github.com/craftcms/cms/blob/develop/src/services/Search.php#L155-L163

If possible, what I'm proposing is this...

  1. Load the config settings for defaultSearchTermOptions around line 155.
  2. Compare that to the Craft default version of the same array.
  3. Fill in any null values with the Craft values.

It seems like a minor kludge, but it would be pretty well contained in the filterElementIdsByQuery method.

Thoughts?

@lindseydiloreto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

Actually, let me adjust my previous statement... a perfect place for that null-value comparison would be in the SearchQuery constructor!

https://github.com/craftcms/cms/blob/develop/src/search/SearchQuery.php#L49-L52

brandonkelly added a commit that referenced this issue Apr 13, 2018

@brandonkelly

This comment has been minimized.

Copy link
Member

commented Apr 13, 2018

I just made sure the default corresponding property values on SearchQueryTerm matched the config defaults (there was only one inconsistency), and now the default value is just an empty array. So you can just set the things you want to change.

@lindseydiloreto

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

Brilliant, that should work perfectly! Thanks @brandonkelly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.