Allow the query builder to define options

Is this issue still a planned part of this patch?

http://drupal.org/node/548104

Thanks!

If I was a better coder, I would. :-) I'll be happy to help test, though!

Subscribing, Solr Views needs this to go forward here and remove some HEAVILY crufted code. Hoping to come back to this real soon.

subscribing

I took a quick look at this last night and this original patch was going to allow the query plugin to be configurable on a View level not a Display level. I am reworking it for a Display level: http://skitch.com/supermanscott/n43ty/query-builder

Is there a good reason why it wasn't on the display level ?

He is a 'working' patch though it doesn't export properly and I have a couple issues with it.

1.) The options form function has to fetch the plugin and init it. Its really strange, it shouldn't have to do that it should be handled in the views_plugin_display::init()
2.) The option definition is similar to access and cache. And thats probably wrong in this case. And the reason its not exporting properly.
3.) I don't like this wording: "t('Disable node access')". As someone who as written a couple db_rewrite hooks, it isn't always node access. The option should actually say what it does and I further hope that this becomes a Filter in D7 or a separate UI element for query tags.
4.) Distinct and group by should be default query plugin options. While Solr does have a rudimentary group by-like system, its will have distinctly different behaviors and I think that will hold true for other query plugins.
5.) like to move the summary_title() from views_plugin_query_default to views_plugin_query.

I think the big thing to review, is the decision to have the display object init the query as opposed to the View object itself and the ramifications that can hold.

Ok here is a working clean patch. It addresses all the points above expect Distinct and group by. First off, group by isn't an issue, there is a whole system around that, views_query::get_aggregation_info() so it is not a worry.

Distinct is a bit of a different beast and that probably is best handled in a separate issue.

This is a reroll with a little cleanup. Overriding isn't working.

It is also not setting the 'options' array based on the $query::option_definition and its not setting the right query type. Probably don't need to maintain the query type as its determined from the base_table anyways.

Here is what the solr form is looking like: http://skitch.com/supermanscott/n5n5i/solr-query-form

posted a patch for Solr Views #750234: Configurable boosts per View display..

Right now I think the problem resides with how option_definition is called. Sometimes it is called after views_plugin_display::init() and other times it hasn't been. This causes issues with the default value for the query type. So while normal Views are fine, once exported it isn't as the query type has been lost.

The other issue is that the options can't be overridden it seems. I think this is a logical issue not necessarily an architectural issue.

Ideas? Thoughts? Am I way off base?

Ok, I got this one. Here is an updated, working patch.

It is also not setting the 'options' array based on the $query::option_definition and its not setting the right query type. Probably don't need to maintain the query type as its determined from the base_table anyways.

Actually had to do this so that way export_plugin could figure out the option_definition(). So i made sure to $display->set_option() that in the views_plugin_display::init. The Solr patch is updated as well.

I believe I got all the ->init_query() calls, but please grep yourself to make sure.

Ahh this puzzled me for awhile as well. That makes sense, that is the major api change.

It should probably

  /**
   * Constructor; Create the basic query object and fill with default values.
   */
  function init($base_table = 'twitter_api', $base_field, $options) {
    module_load_include('inc', 'twitter');

    $this->options = $options;
  }

And then there is a switch on the api method which should be instead

switch ($this->options['api_method'])

And the url should use the $options array as well.

But it seems that the form work and it saved everything for you and you were able to export it? If so, would you consider it RTBC ?

Yes, that is what the default plugin does in this patch. Getting bogged down in details for another module, seems like this patch worked though!

First, I doubt this applies anymore. Second, it should model view::init_style(), which will delay building the query till its needed.

Thanks for dereine for the review.

Committed to 6.x-3.x!

Doesn't quite apply to 7.x so marking for porting.

I've created http://drupal.org/node/882800 as a followup to move the DISTINCT setting.

subscribing