Allow different back-ends for Views
| Project: | Views |
| Version: | 6.x-3.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | views 3.x roadmap |
(For details on this, visit http://groups.drupal.org/node/12838 and possibly also the links there.)
Wanting to present search results of the Apache Solr search engine with Views, I encountered the problem that Views is fixed on obtaining its data from the database.
The attached patch allows for the definition of a different, arbitrary back-end for base tables. All one has got to do in order to achieve this is create a new query class that at minimum needs to define a query() and (although this may change) a get_where_args() method and set $data[$base_table_name]['table']['base']['query class'] to the name of this class in hook_views_data().
At the moment it is not a really elegant solution, but it should at least work. Any suggestions to improve the whole thing are welcome.
#1
Sorry, just now realized the patch didn't get attached. Hope it's there this time.
#2
Updated patch attached. It now applies to the latest version and I also changed the design of the view_query->execute() method to directly set the view object fields, instead of returning an array. While this is against good OOP practice, I think in this case it makes the code a lot cleaner.
#3
the call
$this->query->get_where_args();
isn't needed any more in build().
Why do you remove the initial assignment?
$this->query = new stdClass();
I think this is is a mistake:
- // This reference deals with difficult PHP indirection.
+ // This reference deals with difficult PHP indirection.6/10/2008
in views_object_types, why do you remove - // statically cache this so t() doesn't run a bajillion times. ?
Then I tried this on my test site and now no (usual db) view returns any results. :(
#4
hm, I tried it again and I couldn't reproduce the mentioned problem on my test site. So probably it was a problem of my installation - sry for that.
I've edited and cleaned up the patch from stuff I think it's unneeded - as mentioned above. Furthermore I changed get_where_args() to actually return something - imho otherwise this is confusing. Furthermore your patch killed the live preview previously - the $view->build_info is actually needed. I've fixed that.
attached, is an updated patch. My views installation seems to keep working fine with it - drunkenmonkey, could you test it with your solr backend too pls?
Then I've seen that you restrict hook_views_query_alter to the views_query class only - is there a cause for that? Does it break something if we do not? Imho it should be possible to alter queries from other backends too.
#5
Thanks for testing again and for your work!
With your patch, everything seems to work fine here.
It should be able, you are right. The problem, however, is, that the modules implementing the hook expect the first argument to be an instance of views_query. If it isn't, this could result in a call to an non-existing function and, hence, in a fatal error.
So as long as implementations of the hook are not aware of the different classes, we cannot pass them objects from classes other than views_query.
#6
Re-rolled patch with latest views from cvs HEAD
#7
What's the status of this, apart from RTBC? Will it ever be committed?
If not, what are the issues?
The patch took me (and probably fago, too) quite a while and I would like at least to get feedback, why it isn't included.
#8
This may not be very helpful, but when I installed the module, and added the apachesolr argument or filter, it did not return the results I was expecting. However, I was in a rush so did not have too much time to try to figure out what was going on, etc.
I'll give it another spin, and give a better report.
Thanks
Audrey
#9
The commit status is that I'm requesting that this patch be maintained but I will probably commit it when I branch for Views 3. Note that Views 3 is not as drastic of a departure as Views 2 was, so don't think of it is quite as big of a jump. But once this goes in some things, at least in my mind, will be significantly different and I do not want to endanger the 2.x line with it.
That said, I do want this, but we are biding time with it.
#10
hm, I think maintenance shouldn't be the problem. However time might be.. I fear that views 3 is too far away for my needs :(
#11
I'm getting failure notices when patching. Does this mean it needs to be re-rolled?
#12
I've rerolled the patch against head. It looks like the switch from microtime to views_microtime is what changed in views and caused the failure reported above.
Can't wait to see this get committed.
#13
Ok, I have only one complaint about this patch:
+ if ($query_class == 'views_query')Let's avoid this. Call through to the query class and let it decide what to do. That way other query classes can use their version of query alter or whatever.
Otherwise, this is less intrusive than I thought it woudl be and I can get this in after all.
#14
i'm going to be super nitpicky since i doubt it'll stop earl from committing it if he's into it ;)
yeah the query class testing seems like a bad smell. the SQL signature injection should probably become an option on the query builder.
if we endup going with
+ // Only do this, if the default query class is used.+ if ($query_class == 'views_query') {
it should include a big TODO to relax the restriction in later versions of the API. or maybe we could ask the class if it's alterable?
i'd sort of like to see $external given a default value at the top of the function with a comment on it's purpose. I'd also like a little whitespace before
+ if (!empty($external)) {+ db_set_active();
+ }
so it doesn't get lost in the previous block of code.
while we're in here can we make these comments into proper sentences?
+ // dump information about what we already know into the globals+ global $pager_page_array, $pager_total, $pager_total_items;
+ // total rows in query
+ $pager_total_items[$view->pager['element']] = $view->total_rows;
+ // total pages
+ $pager_total[$view->pager['element']] = ceil($pager_total_items[$view->pager['element']] / $view->pager['items_per_page']);
#15
The attached patch should address the issues raised above. I've moved the invocation of hook_views_query_alter into the query class, given $external a default value (with an explanatory comment), and made the pager comments into sentences.
#16
Setting status to 'needs review'.
#17
I tested this patch, it applied fine. I was also able to build a normal view w/ nodes, as well as a view using an external source w/ a test module.
#18
Ok, this could probably go in as is, but I'm a little concerned with the organization of what we're doing here. If we extend to a time down there road when there is a query object for solr, sparql, flickr, delicious, twitter and some other esoteric thing where we're doing odd integration, there is absolutely no organization to the query objects.
The query class should probably be registered similar to handlers, and with a pointer (auto-generated, most likely) to the file that the query object is in. That way the query object won't have to be loaded if it's never used. The existing query class possibly even needs to be renamed to views_query_sql or something to specify which kind of query it is.
I guess we don't need a base query class, since the only truly required interface on the query class is execute() and alter() -- everything else is fairly specific to the class.
Now, these things may not be necessary right away. Renaming the query class is a bit of an undertaking. Registration, however, is pretty easy. We can even register it as a plugin so we don't need a separate hook or anything. And registration would allow some fairly nifty stuff with subclassing and allowing modules to actually replace the query object for existing things with something else.
I'm not going to set this CNW, though until I've thought about this some more.
#19
Very interesting stuff. For those interested Jeff from Development Seed used this patch to create a proof of concept for Flickr API. http://www.developmentseed.org/blog/2009/feb/05/extendr-flickr-and-views
#20
I'm really excited about the progress here! :))
But of course we need to make sure to get it done right. Registration as plugin sounds to be really useful and straight forward. More thoughts on this, merlin?
#21
hook_views_plugins already registers multiple plugin types, so 'query' could be its own type.
We would then need 'views_plugin_query' as a base type which describes the basic operations (->execute() and ->alter() I think) and all query plugins would then be registered that way. Otherwise everything else looks pretty similar to this patch, I think.
#22
Great, I should be able to dedicate a some time tomorrow to this.
#23
Attached is patch that makes the query object into a plugin. I'm providing a interface-like base query class "views_plugin_query", which extends "views_plugin". The default query building functionality has been moved form "query.inc" and into "views_plugin_query_default.inc".
Two unexpected things came up to take note of:
#24
The patch looks good to me. I tested it and all normal views (including creating, ..., them) seemed to work fine. I just couldn't bring a view with a new query class to work, but this is probably rather my fault than the patch's. I'll keep on trying it.
One more thing, regarding the inelegant class test, that Earl critisized. Maybe we should change this:
if (variable_get('views_sql_signature', FALSE) && $query_class == 'views_query') {$this->query->add_field(NULL, "'" . $this->name . ':' . $this->current_display . "'", 'view_name');
}
to something along these lines:
if (variable_get('views_sql_signature', FALSE)) {$this->query->add_signature();
}
Once this is committed and modules start implementing their own backends, this would be very difficult to do.
Another option would be to pass a $add_signature argument (or even a $options array argument, which could be very useful for later extensions) to the query() method.
#25
Thanks for the review! I also agree that the class test could still be better. I'll wait for Earl to weight in about moving the signature - perhaps we should let the query class handle it entirely and not have any of this logic in view.inc.
As far as getting this working with a new query class take a look at my flickr implementation which may be helpful.
#26
It really was very helpful, thanks!
Since your module requires a Flickr API Key, I couldn't test the patch with this query class, but I finally managed to bring my own to work.
So afaict the patch is OK, only the little issue above seems to remain. Good work, anyways!
#27
Oh, no, too hasty. I just discovered that if the query class hasn't got the method get_where_args(), it results in the following error:
Fatal error: Call to undefined method apachesolr_views_query::get_where_args() in .../views/includes/view.inc on line 628
Either this should be fixed and the logic in the corresponding views-method moved to the query class (the way I'd prefer it), or views_plugin_query should have the get_where_args() explicitly. I'll try to look into it tomorrow.
I think, all other methods not in views_plugin_query can really be omitted, so it's just the one.
#28
Regarding the issue mentioned above, I've got two approaches:
1) Introduce a new method, views_plugin_query->build_info() which gets called between alter() and execute() and takes care of filling the build_info with the last pieces of necessary information. I.e., in views_plugin_query_default it would look like this:
<?phpfunction alter(&$view) {
$view->build_info['query'] = $this->query();
$view->build_info['count_query'] = $this->query(TRUE);
$view->build_info['query_args'] = $this->get_where_args();
}
?>
2) Since in the above solution, making query() explicitly part of the interface is unecessary, the alternative would be to just change the definition of query() so it directly fills $view->build_info with the necessary information/queries. views_plugin_query_default->query() would then look like this:
<?phpfunction query(&$view) {
$view->build_info['query'] = $this->_query();
$view->build_info['count_query'] = $this->_query(TRUE);
$view->build_info['query_args'] = $this->get_where_args();
}
?>
With the original query() method renamed to "_query()".
Personally, I'd prefer the second variant, since imo it looks cleaner.
Two patches with speaking names are attached.
#29
Good catch - get_where_args() is certainly needed. I'd like to suggest a third alternative. Instead changing the internal workflow of view::build() we could just add a stub-ish get_where_args() metod to the views_plugin_query class.
Patch is attached.
#30
OK, this would probably be the easiest solution, you're right.
But since the method doesn't make much sense for most types of query class, I'm a bit worried about putting this in the base class, even as a dummy implementation, in terms of good style and clearness of the interface.
Well, good idea anyways. Let's just let Earl decide which approach to take.
#31
I think the best way to go what be solution 1 from #28. But perhaps just name the method build() roo - so $view->build() would invoke $query->build(), which writes into $view->build_info.
@#29:
"where" is sql specific, so I would take the solution above. Furthermore it allows the query plugin to pass even more stuff to execute() if necessary.
#32
@fago, sure - I'm totally fine with using solution 1 from #28. ...also I'm neutral on the method name change, but in the interest of moving this forward...
@drunken monkey - do you want to re-roll that patch with the method name change and set this to 'needs review'?
#33
OK.
#34
Sorry, was sloppy, here is the right one.
#35
Upon attempting to apply the patch in #34 I found *several* cases in views_plugin_query_default::execute() that had not properly turned '$this' into '$view' causing total failures. These need to be cleaned up.
#36
Sorry about that, I didn't notice. A refined patch is attached, with no '$this' at all in that method. So at least this method should be OK, I hope it didn't happen elsewhere too.
What strikes me as odd, though, is that I didn't notice any errors while testing the patch...
#37
Should we increase the views api version number? See http://drupal.org/node/254565#comment-1276500
But if we would do so, views would only use APIs written for >2.0, so probably that's a bad idea. So probably best modules depending on this feature should check whether the right views version is available and only return their views integration infos, if it's ok? Thoughts?
#38
Attached is an updated patch that includes a recent change to query.inc http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/views/inclu... and corrects a typo in #36
#39
more @API:
>But if we would do so, views would only use APIs written for >2.0, so probably that's a bad idea
We could also bump the API to 2.4 or whatever and change views to accept all 2.0-2.4 APIs. So elder versions of views would just ignore new views integration relying on this patch. What do you think about that merlin?
#40
I just tested this patch with latest views 2.x dev and extendr.
Patch does neither break existing views nor existing default views: I tested 2 default views and 4 views with a before/after comparison of patch applied and caches cleared.
I rerolled to reflect two tiny changes to query.inc in views_plugin_query_default.inc (query.inc 1.40 and 1.41).
This is great work and opens a whole new world of possibilities, I'm hoping we can use this in production soon.
#41
The patch in #40 looks ready to go to me. Thanks Alex.
#42
Subscribe. migrate module would like to select files in a directory tree. It would be like a UI for file_scan_directory.
#43
We'll be using in production soon and are starting some testing. Mostly for Solr purposes. Great stuff!
#44
@sirkitree just fyi, I spoke to Earl briefly about this patch at Drupalcon DC and it looks like the plan is to commit this after the next point release of views (2.4).
#45
subscribe -- we're running this now in conjunction with the solr patch: http://drupal.org/node/254565
#46
subscribe
#47
2.4 is out. Please reroll this. AS soon as I feel confident I don't need to make an emergency release post 2.4, I'll get this sucker committed.
#48
Great. You'll have it mid-day tomorrow.
#49
Attached is a re-rolled patch, which incorporates the recent changes to query.inc
#50
Reviewed and tested by this community of 1:
#51
This patch is working great with this module: http://drupal.org/project/apachesolr_views
#52
I have branched a 3.x dev branch and made this the first commit. There are other largish things and it seems like this is a good time to start work on Views 3 for real.
Great job, folks, this is a fantastic feature and I'm glad you all stuck it through even though I've been very slow.
#53
ahem.
#54
Automatically closed -- issue fixed for 2 weeks with no activity.
#55
I have re-rolled this against 2.6. I can't move to 3.0 and I wan the security improvements and the caching of 2.6. I provide it here for those looking.
#56
m3avrck pointed out the major difference in size of the two patches. This is because i didn't 'remove' the plugins/query.inc. Attached has that file removed.
#57
And I over-wrote a the query caching... here is the latest re-roll.
#58
i was not able to get the patch work against views 2.6.
#59
i was not able to get the patch work against views 2.6. it is giving couldn't open file \dev\null error (i am using tortoise on windows) once the patch is done the query doesn't have any content it, is there any other patch i have to apply before i apply backend.patch the one in 57 to 2.6. (i am a newbie here, please forgive me for asking)
adv thanks
#60
This patch will not be committed to the 2.x branch. It is the *reason* for the 3.x branch. I appreciate that it is here for people who want to run a patched 2.x, though at that point you're really running the 3.x code so I'm not sure what the point is.
#61
adding tag to find this later
#62
I've been reading through this thread, and I could really use it, and pardon me if I'm being blind, but I'm not seeing how to implement this with Solr. I've downloaded 6.x-3.x-dev, but I'm not understanding what to do next. I would look at the Flickr Extendr module, but the link to it above doesn't seem to have anything there any more. Can anyone point me in the right direction?
Thanks.
Update: Never mind, I figured it out.
#63
FYI: The link to extendr module above is broken. Here's the correct link: http://devseed.svn.cvsdude.com/sandbox/drupal-6/extendr/