backdrop-contrib / search_api

Provides a generic API for modules offering search capabilities
GNU General Public License v2.0
0 stars 5 forks source link

Store values in state rather than config #59

Closed argiepiano closed 7 months ago

argiepiano commented 7 months ago

search_api_facetapi uses config to store search ids. See search_api_facets_search_ids and search_api_facetapi.settings.json. These values are likely to change often (probably on each search), and are not user-configurable. Storing them as config is unnecessary and probably costly. They should be moved to state.

I also see a bunch of other config keys that are not used anywhere: date_format_search_api_facetapi_. Those can probably be removed. It's hard to understand the rationale there.

laryn commented 7 months ago

@argiepiano Agreed.

I took a look at the other issue about unused config keys and it seems like this is where it came in: https://www.drupal.org/project/search_api/issues/2677900

For Backdrop we can probably delete all those config keys and include some date format config files in that submodule's config folder to provide those configurable date formats, and use those formats where needed in the module itself.

laryn commented 7 months ago

@argiepiano I've a PR for this one -- can you test?

argiepiano commented 7 months ago

For Backdrop we can probably delete all those config keys and include some date format config files in that submodule's config folder to provide those configurable date formats, and use those formats where needed in the module itself.

Yes, I noticed that D7 required defining variables to store the date type's specific format. This is done differently in Backdrop - I see that you have that already, by putting those new formats inside system.date.json. I had a small question about the need to lowercase those constant values.

The removal of search_api_facets_search_ids looks good also. I can't test right now but will try to get to this later.

laryn commented 7 months ago

Thanks @argiepiano -- I've responded in the PR to your question.

argiepiano commented 7 months ago

About to test now...

argiepiano commented 7 months ago

Tested, looks great. RTBC.

laryn commented 7 months ago

Thanks @argiepiano! Merged.