Skip to content

Backport unpermitted params #2718

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

Merged
merged 2 commits into from
May 19, 2022
Merged

Conversation

cbeer
Copy link
Member

@cbeer cbeer commented May 11, 2022

Backport of #2717.

To maintain backwards compatibility, defaults blacklight_config.filter_search_state_fields to false, which means we only warn about the parameters instead of filtering them out. This gives plugins and downstream apps a chance to add either permitted_params for a custom FilterField, or add configuration to search_state_fields.

@barmintor
Copy link
Contributor

barmintor commented May 12, 2022

  • run blacklight_range_limit/master CI against branch
  • run blacklight_range_limit/v8.1.0 CI against branch
  • run geoblacklight/main CI against branch
  • run blacklight_oai_provider against branch
  • run spotlight/master against branch

@barmintor
Copy link
Contributor

geoblacklight/master outputs a lot of:

DEPRECATION WARNING: Blacklight 8 will filter out non-search parameter, including: bbox. (called from permit_search_params at /Users/ba2213/.rvm/gems/ruby-3.0.0/bundler/gems/blacklight-9644976c01ad/lib/blacklight/parameters.rb:77)
/Users/ba2213/.rvm/gems/ruby-3.0.0/bundler/gems/blacklight-9644976c01ad/app/views/catalog/_search_form.html.erb is a deprecated partial.

as intended.

@barmintor
Copy link
Contributor

blacklight_oai_provider outputs a lot of:

DEPRECATION WARNING: Blacklight 8 will filter out non-search parameter, including: from, metadataPrefix, until, and verb. (called from permit_search_params at /Users/ba2213/.rvm/gems/ruby-2.7.5/bundler/gems/blacklight-9644976c01ad/lib/blacklight/parameters.rb:77) (4 times); e.g.: 

as intended.

@barmintor
Copy link
Contributor

blacklight_range_limit/master has been updated to register the params; the last released version (v8.1.0) passes CI against the branch with:

DEPRECATION WARNING: Blacklight 8 will filter out non-search parameter, including: commit and range. (called from permit_search_params at /Users/ba2213/Github/projectblacklight/blacklight/lib/blacklight/parameters.rb:77) (1 times); e.g.:

@barmintor
Copy link
Contributor

spotlight/master a bit harder to test because there are 2 or 3 (depending on ruby 3 or 2.7) expected unrelated failures (see #2816). I am seeing a lot of:

DEPRECATION WARNING: Blacklight 8 will filter out non-search parameter, including: browse_category_id, exhibit_id, and id. (called from permit_search_params at /Users/ba2213/.rvm/gems/ruby-3.0.0/bundler/gems/blacklight-9644976c01ad/lib/blacklight/parameters.rb:77) (2 times); e.g.: 

@barmintor
Copy link
Contributor

By way of review I've also drafted a PR to blacklight_range_limit following this pattern: projectblacklight/blacklight_range_limit#186

Running locally with solr_wrapper pinned to 8.9.0 and using this branch of blacklight 7, it is passing.

@barmintor
Copy link
Contributor

I also note that range limit and spotlight are both getting deprecation warnings for id... should we just add that to Blacklight::Configuration::BASIC_SEARCH_PARAMETERS?

Copy link
Contributor

@barmintor barmintor left a comment

Choose a reason for hiding this comment

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

I've tested against the last release tags of the plugins I know about, and things are working as expected. I can see it's a bit of a drag to get the merge working on permitted params, but I think the approach here is strictly an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants