The attached patch refactors the handling of exposed filters so that invokers of views_build_view can pass in exposed filter values, instead of relying on views to fetch them from the $_GET global. This patch bubbles down the changes to query building level as well, so that views_query.inc is no longer coupled to the exposed filter form implementation.

If no exposed filter values are passed to views_build_view, they are fetched from the $_GET global just as usual.

Some notes:

1. The $filters array passed into views_build_view is of the form

array(
 0 => array('op' => 'foo', 'filter' => 'bar),
 1 => ... etc
);

This reflects the old style of views exposed filter handling. Is this still necessary? Instead of stripping out all the names, etc, can we just return to normal Drupal form handling? I think it might be a little cleaner. Also, drupal no longer prepends 'edit' on everything, so it will a lot like the current implementation. (This will also help viewfield with exposed filter handling).

2. I've tried to be backward compatible. I doubt many other modules are calling _views_build_filters directly, but if they do, they're going to get a rude surprise. I'm guessing these changes are pretty safe.

CommentFileSizeAuthor
#15 views_20070413_backport_to_47.patch7.4 KBmfredrickson
#11 views_20070411_cleaner_refactor.patch9.17 KBmfredrickson
#8 views_20070404_refactor_only_important_files.patch9.95 KBmfredrickson
#6 views_20070403_refactor_filters.patch12.89 KBmfredrickson
views_20070308_refactoring_exposed_filters.patch6.24 KBmfredrickson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

subscribe ... a quick read through didn't sound any alarms. if it works, i think it is commit worthy. will try to test it, but am busy busy.

yched’s picture

yched CreditAttribution: yched commented

+1 on the feature - but this is not (yet) an actual review of the patch either

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented

Thanks for the +1s.

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented

I've rerolled this patch against 1.x-dev

In addition to the earlier comments, I've also made the following changes:

1. Removed the explicit "#name" setting the views_filters() function. Drupal no longer prepends edit[] to everything, so we don't need to explicity set the name anymore. This will help with my second change...
2. Refactored views_filters() into two functions: _views_build_filters_form() and views_filters(). The new function (_views_build_filters_form()) simply constructs a form array of all the ops and filter values and passes it back to views_filters(), which wraps it with the #action, #method, etc that views needs for standalone view pages.

These changes will faciliate other modules using views filters outside the context of views callback pages. (e.g. viewfield and my views node selector form element work).

The api remains unchanged for views_filters(), so you don't need to worry about changing anything.

I've tested this patch with required and optional filter, textbox values and multiselects, choosing one options, or many options, and with the pager. Everything looks good, though I would still appreciate a review.

Cheers,
-Mark

KarenS’s picture

KarenS CreditAttribution: KarenS commented

Did you mean to attach a patch when you said you re-rolled it??

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented
FileSize
views_20070403_refactor_filters.patch12.89 KB

I have to stop doing that.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

I'm having trouble getting the patch to apply, plus there is a lot of unnecessary stuff in it. It picked up diffs for all the version name in all the info files. Do you want to try to re-roll?

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented
FileSize
views_20070404_refactor_only_important_files.patch9.95 KB

Sure. Here is a patch that only applies to views.module, views_query.inc, and views_cache.inc.

Mojah’s picture

Mojah CreditAttribution: Mojah commented

Would this patch have to be applied in order to set an exposed filter according to the node being viewed? I have a view embedded into my page.tpl.php as ...

<?php
$myview = views_get_view('page_quotes');
$myview_args = array();
$myview->filter[0][value] = "frontpage";
print views_build_view('block', $myview, $myview_args, false, 4);
?>

Am I on the wrong train here? Thanks.

KarenS’s picture

KarenS CreditAttribution: KarenS commented

I think adding this capability is a nice addition and makes sense. I've tried it on a couple test sites, one a fresh install and one an existing site, and it seems to work fine.

I never could get the patch to apply, it was choking on the first and last chunk in views.module, neither of which actually make any changes, and I finally got it working by manually removing both of them from the patch.

So this is a +1 from me. If we can get a cleaner patch, and Earl agrees that the feature is acceptable, I would mark it ready to commit.

@Mojah, yes, you should be able to do that with this patch, but slightly differently than your example. It looks like it would be something like:

$myview = views_get_view('myview');
$myview_args = array();
$filters = array(
  0 => array(
    'op' => '=', 
    'filter' => 'frontpage'
));
$use_pager = FALSE;
$limit = 0;
$page = 0;
$offset = 0;
print views_build_view('page', $myview, $args, $use_pager, $limit, $page, $offset, $filters);
mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented
Version: 5.x-1.5 » 5.x-1.x-dev
FileSize
views_20070411_cleaner_refactor.patch9.17 KB

Ok. After a little manual massaging I have a patch that applies cleanly with all the appropriate changes.

Also, I'm changing the version to 1.x-dev, as this patch applies to the dev branch.

I agree with Karen, if we can get Earl's approval, this patch is RTBC.

Thanks to all the testers!

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

well, lets get earl's review then.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Reviewed & tested by the community » Patch (to be ported)

This one is, indeed, pretty clean and looks good.

Applied to 5.x branch. Would it be possible to backport this to 4.7.x as well? It's probably not necessary, so if you don't no big deal, either.

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented
Assigned: Unassigned » mfredrickson

I can back port it. No worries.

Thanks for reviewing and applying. :-)

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented
Version: 5.x-1.x-dev » 4.7.x-1.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
views_20070413_backport_to_47.patch7.4 KB

Attached is a backport to 4.7.

This patch is a little smaller than the 5.x version as I didn't split the form building code into two functions. Since in 4.7 you need to set the #name property on everything in the filter form the need for that particular re-factoring went away.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Reviewed & tested by the community » Fixed

Applied. I'm trusting you a bit, trying to get through as many issues as possible in fairly limited time.

mfredrickson’s picture

mfredrickson CreditAttribution: mfredrickson commented

Thanks Earl. :-)

Anonymous’s picture

(not verified) CreditAttribution: commented
Status: Fixed » Closed (fixed)
.-_-.’s picture

.-_-. CreditAttribution: .-_-. commented

did this patch get commited?