various bbox argument handler fixes:

- use real_field instead of field when constructing the field name for the query filters.
- use ensure_table instead of add_table to avoid unnecessary joins.
- Universally use 'bbox' as default for arg_id instead of 'BBOX' as described in the issue
- added a fix to the patch that declares arg_id option_definition of views_plugin_argument_default_bboxquery and therefore makes it exportable. (see #1859954: Views GeoJSON arg_id isn't exported
- also a bigger cleanup to views_geojson_bbox_argument that removes all $_GET handling, which from my point of view was duplicated and should only live in views_plugin_argument_default_bboxquery

background / original issue description:

1) while working on #1839462: BBox argument handler doesn't work with GROUP BY for geocluster, i realized that i don't need the bbox handler to support having but made some small changes which solve some problems for me.
2) to prevent cluttering into too many issues, i have merged the changes from #1859938: Universally use 'bbox' as default for arg_id instead of 'BBOX' into this patch.

CommentFileSizeAuthor
#15 interdiff--1839554-14-15.txt859 bytesmpgeek
#15 1839554_views_geojson_bbox_argument_handler_fixes_15.patch6.42 KBmpgeek
#14 1839554_views_geojson_bbox_argument_handler_fixes_14.patch6.08 KBbatje
#12 1839554_views_geojson_bbox_argument_handler_fixes_11.patch6.03 KBdasjo
#2 1839554_views_geojson_bbox_argument_handler_fixes_2.patch6.33 KBdasjo
#1 1839554_views_geojson_bbox_argument_handler_fixes.patch1.32 KBdasjo

Comments

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented
Status: Active » Needs review
StatusFileSize
new 1839554_views_geojson_bbox_argument_handler_fixes.patch1.32 KB
dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented
StatusFileSize
new 1839554_views_geojson_bbox_argument_handler_fixes_2.patch6.33 KB

to prevent cluttering into too many issues, i have merged the changes from #1859938: Universally use 'bbox' as default for arg_id instead of 'BBOX' into this patch:

- Universally use 'bbox' as default for arg_id instead of 'BBOX' as described in the issue
- added a fix to the patch that declares arg_id option_definition of views_plugin_argument_default_bboxquery and therefore makes it exportable. (see #1859954: Views GeoJSON arg_id isn't exported
- also a bigger cleanup to views_geojson_bbox_argument that removes all $_GET handling, which from my point of view was duplicated and should only live in views_plugin_argument_default_bboxquery

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

In #1877902: Tile based argument I've done some rewrite too but moved all argument handling out of ::query

What I miss in BBOX is the no argument found handling it dumps ALL nodes which is contrary to 'limiting nodes'. Not sure that's a feature or a bug.

Another point is do we need Views Geo JSON provide for BBOX or ZXY? Is a seperate project not better? See comment #2 in #1877072: Allow BBOX in other display format.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

The function _get_fields($data_source) { .. } is never used so this patch also removed that.

Nice catch for the duplicate table joins ... we were wondering where those came from.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

XREF #1806048: BBOX filter not working

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

commit c4216d961b45062910644dd5eac7143b2ce7df76

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented

It seems the function get_argument() { is never called on my system.

I continue through #1894494: $this->argument can contain value TRUE.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Status: Fixed » Needs work

I've reverted this back.

Testing this with the views preview without the patch gives a query like

AND( (location.longitude >= '?bbox=4.6647254729841') AND (location.longitude <= '5.1062385270159')

which is only explainable when $this->argument = ?bbox=4.6647254729841...

But running this from within a map $this->argument === TRUE

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented

i don't really see what #7 and #8 have to do with this patch?

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Assigned: Unassigned » clemens.tolboom

You're probably right ... I'll retry the patch next week.

Trouble for me is the argument handling is almost untestable :(

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented
Status: Needs work » Needs review

i have rerolled the patch + updated the issue summary.

i'd be glad if i can help testing it. i'm using it in combination with http://drupal.org/project/leaflet_geojson + http://drupal.org/project/geocluster

dasjo’s picture

dasjo
he/him
Primary language German
Location Zurich
commented
StatusFileSize
new 1839554_views_geojson_bbox_argument_handler_fixes_11.patch6.03 KB

ups here's the patch

randallknutson’s picture

randallknutson commented

Patch worked for me. I'm using it in a recipe mentioned in #1641870: Bounding Box support with Search API views

batje’s picture

batje commented
StatusFileSize
new 1839554_views_geojson_bbox_argument_handler_fixes_14.patch6.08 KB

When I base a view on a search_api_views view, all the SQL magic in the arguments->query() function is not supposed to happen.

This version of the patch has a (ugly) check for that.

batje’s picture

batje commented
Issue summary: View changes

updated summary

mpgeek’s picture

mpgeek commented
Issue summary: View changes
StatusFileSize
new 1839554_views_geojson_bbox_argument_handler_fixes_15.patch6.42 KB
new interdiff--1839554-14-15.txt859 bytes

In #2170641: Strict warning in views_geojson_bbox_argument.inc there's a quick fix for the query() method in the argument handler class. The patch there conflicts with the patches here in this issue. Here is an updated patch and interdiff that includes the patch from #2170641: Strict warning in views_geojson_bbox_argument.inc.

pol’s picture

pol
Location Brussels
commented
Status: Needs review » Fixed

Committed, thanks all !

  • Pol committed dc54d8c on 7.x-1.x authored by batje 
    Issue #1839554 by dajso, batje, mpgeek: BBox argument handler fixes
clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
commented
Assigned: clemens.tolboom » Unassigned

(Never assign to myself)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.