I've done quite a bit of cleaning up of this module.
Some of the changes in this patch:

This needs a review and perhaps some rework.

CommentFileSizeAuthor
#13 views_geojson-cleanup-2396781-13.patch26.83 KBbasvredeling
#11 views_geojson-cleanup-2396781-11.patch23.89 KBbasvredeling
#10 clean_up_the-2396781-10.patch27.8 KBbasvredeling
#8 clean_up_the-2396781-8.patch27.99 KBbasvredeling
#6 clean_up_the-2396781-6.patch27.99 KBbasvredeling
#5 clean_up_the-2396781-5.patch25.05 KBbasvredeling
#3 clean_up_the-2396781-3.patch30.33 KBbasvredeling
#1 major_update_of_views_geojson.patch23.66 KBbasvredeling

Comments

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new major_update_of_views_geojson.patch23.66 KB
pol’s picture

pol
the/that
Location Brussels
commented
Status: Needs review » Needs work

I think your patch is incomplete :)

The function to beautify the json is missing.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new clean_up_the-2396781-3.patch30.33 KB

I don't really know what happened there. JSON beautification applies to the preview and "page" output only. Not to feeds. Hence it was probably temporarily taken out. Anyhoo, this is the latest patch.

pol’s picture

pol
the/that
Location Brussels
commented

Erm I think there's a problem with your patch... Check the .module file, the hooks views_geojson_views_api,views_geojson_ctools_plugin_api are missing, the function _views_geojson_encode_formatted is missing too.
Plus, there's a class in that file.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new clean_up_the-2396781-5.patch25.05 KB

Yes, sorry, I see what you mean. I accidentally copied the contents of the views_plugin bbox filter file to the module file.
This patch should be correct.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new clean_up_the-2396781-6.patch27.99 KB

If I include the new views_geojson.nicejson.inc that is.

pol’s picture

pol
the/that
Location Brussels
commented

Those changes seems to be ok in general, but it includes a new alter function already discussed #2332221: alter json content.

Also the file with the nicejson functions should be named like this: views_geojson.helpers.inc.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new clean_up_the-2396781-8.patch27.99 KB

I'm not sure what you mean by: #2332221: alter json content
The patch there is not included in my update. Could you clarify?

I've renamed the include though. Here's the updated patch with helpers.inc

pol’s picture

pol
the/that
Location Brussels
commented

Hello,

This bit of code worries me:

+  // Let modules modify the data.
+  foreach (module_implements('views_geojson_render_fields_alter') as $module) {
+    $function = $module . '_views_geojson_render_fields_alter';
+    $function($feature, $view, $row, $index);

It has performance implications and I'm not sure that I want to have it in.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new clean_up_the-2396781-10.patch27.8 KB

Right, right. I see. The alter hook is in because of geocluster suggested patch. I thought it would do little harm to performance if no _views_geojson_render_fields_alter hooks are implemented. But I agree it's a different issue. I'll remove it from the patch and discuss it in #1799870: Add hook views_geojson_feature_alter. Also there is this issue awaiting review #2370471: Make patching Views GeoJSON obsolete. I'll test that too.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new views_geojson-cleanup-2396781-11.patch23.89 KB

Merged cleanup code from #1641870: Bounding Box support with Search API views into this new patch.

pol’s picture

pol
the/that
Location Brussels
commented

Hello Bas,

Thanks for your patch.
The function _views_geojson_encode_formatted() is still missing.

Could you update it so I can commit it ?

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new views_geojson-cleanup-2396781-13.patch26.83 KB

I really need to evaluate the way I create my patches. This is the second time the .helpers.inc is not included in the patch.
Anyhoo, here goes. I believe this new one is correct.

pol’s picture

pol
the/that
Location Brussels
commented

I suggest you to use PHPStorm for a better visual experience with Git !

I'll commit your patch as soon as I'm back home.

Thanks again !

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Reviewed & tested by the community

Actually, I do use PHPStorm. But the patches it outputs are not digested by the Drupal test bots. So I reverted back to command line.
Changed the status since you promised to commit in #14
Thanks very much!

  • Pol committed cebdc2d on 7.x-1.x authored by basvredeling 
    Issue #2396781 by basvredeling: Clean up the views_geojson module
pol’s picture

pol
the/that
Location Brussels
commented
Status: Reviewed & tested by the community » Fixed

It tooks time but it's in now.

Thanks !

Tips for the next time, split your patch in different sub-issue so it's easier and faster to get it in.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented

Thank you. I'll try to split it up into separate issues next time.

Status: Fixed » Closed (fixed)

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

monstrfolk’s picture

monstrfolk commented

Confused on the status of bbox support with search api.

@basvredeling

Wrapping options from #2395407: Bbox argument: wrapping doesn't apply to all coordinate systems

Patch contains

if ((empty($this->argument)) || ($this->view->base_field == 'search_api_id')) {
  return;
}

Does not look like bbox support works.

basvredeling’s picture

basvredeling
Primary language Dutch
Location Amsterdam
commented

@monstrfolk Search api support was never in. Let's create a new issue for it: #2747493: Search API bbox support. The current status of search api support has also been mentioned here: https://www.drupal.org/node/1839462#comment-9985073