-
Notifications
You must be signed in to change notification settings - Fork 256
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
Add field configuration for :include_in_request. If true, the field will... #783
Conversation
I want to discuss here what you wrote me in #781 as an usage example: before_filter only: :index do
blacklight_config.add_facet_fields_to_solr_request!
end
before_filter only: :facet do
blacklight_config.add_facet_fields_to_solr_request! params[:id]
end The problem in this example is that it's not part of the configuration anymore, it's more like a runtime decision. Sounds a bit weird to change the configuration in runtime. What do you say? |
@cbeer if you're OK with using the before_filter example you wrote me, I'll be happy to rewrite #781 without using user_params. I can do it quite easily. |
I'm still having trouble keeping up. If you need special local behavior for something not ordinarily included in Blacklight, and you can do it with a If you guys are suggesting including default Blacklight CatalogController setup such that it uses a Including a method off of Again, while I see the undesirability of the current architecture you are talking about -- there are unfortunately many undesirable parts of the BL architecture, it's important the cure not be worse than the disease as far as maintainability, and complexity of code and cognitive load in understanding it is a big part of maintainability. This part of BL may be too hard to understand now, it's important not to make it worse. |
I'm not suggesting the default include this behavior, just that it's an option if @eranhirs actually wants/needs that behavior. We've supported run-time configuration of blacklight for some time now. This patch doesn't introduce any new methods, and just refines an existing one. (For the purposes of this patch, the |
yes, agree that for local behavior, before_filters are appropriate. Not quite sure what we're talking about, but I don't need to be. |
@cbeer I agree, this is a great PR irrelevant whether I will use it in |
return unless blacklight_config.add_field_configuration_to_solr_request | ||
|
||
blacklight_config.show_fields.each do |field_name, field| | ||
blacklight_config.show_fields.select { |field_name,field| field.solr_params || field.include_in_request || (field.include_in_request.nil? && blacklight_config.add_field_configuration_to_solr_request) }.each do |field_name, field| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the inner block be a method? It could be resused on line 351 too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Updated.
I like it. |
…ill be explicitly added to the request made to solr. Otherwise, Blacklight assumes the appropriate configuration is made in the solr config.
Add field configuration for :include_in_request. If true, the field will...
Why not leave the old I don't really understand the use case for wanting to include some of them but not all of them; I trust there is one, but the use case for wanting to include all of them remains and I suspect is common? |
@jrochkind that's exactly what this commit does. EDIT: I originally had some code that iterated over e.g. all the facet fields when it was called to set |
@jrochkind There is a method |
Okay, cool. Why not leave the backwards compatible way to do this though, instead of requiring code to be changed to do the same thing using a new method? |
@jrochkind what's backwards incompatible about it? I went to great pains to keep it backwards-compatible. |
ah, okay, I guess I misunderstood. I thought |
Supported, yes, but deprecated. I don't think applications are changing that configuration directly, though.. we want them to use |
Somewhere -- I can't find where -- there's a whitelist of things that are passed along. I _think_ it related to [this discussion](projectblacklight/blacklight#783) which references doing something in a :before filter. Everything in @params that is _not_ whitelisted in the config somehow is thrown away. Everything in extra_controller_params is passed through unmolested. So, because we're sending lots of stuff not configured in the normal blacklight way (i.e., pretty much everything in the foci) we'll just merge @params into extra_controller_params and call it a day. At this point I do _not_ understand why everything isn't passed through; I'm assuming it's for security, which is less of an issue for us since we're behind the firewall. I also can't think of anything too nasty one can do with GET parameters if you can't pick the target handler.
... be explicitly added to the request made to solr. Otherwise, Blacklight assumes the appropriate configuration is made in the solr config. Attempts were made to keep this change backwards compatible with existing practice.
The configuration looks like:
Same goes for
add_index_field
/add_show_field
andadd_field_configuration_to_solr_request
.