Gnode provides a new entity type that is designed specifically for the collection and storage of data that is intended to be displayed on maps. http://drupal.org/sandbox/ccardea/1146278

The impetus for this project grew out of my inability to find suitable replacements in Drupal 7 for functionality that I had been using on Drupal 6 sites. Specifically I was looking to replace the geocoding and proximity searching that had been provided by Openlayers Geocoder and OpenLayers Proximity modules, since those projects were not being ported to Drupal 7. Both of those projects had relied on the Openlayers CCK field, which became obsolete with the movement of the CCK project into Drupal core. There were a couple of projects (geolocation and geofield) that created fields and geocoders that are compatible with Drupal 7, but the proximity searching feature was not available. The OpenLayers modules had provided a nice package that worked together well, and that has been lacking in Drupal 7.

I considered the possibiliity of trying to upgrade the Openlayers modules to Drupal 7, but ultimately decided that implementing the idea of a geo-entity has some advantages:

* The database storage can be optimized for geo-searching.
* Data storage can be shared by different mapping front ends.
* Operations on the data can be shared by different mapping front ends.

The gnode project provides a new way for mapping software to work with Drupal. Instead of implementing fields, maps and geocoders implement form elements. The gnode project provides the pages that the form elements are displayed on, and will provide the means to configure the pages with different form elements. A live demo can be found at http://gnode.ccardea.com

CommentFileSizeAuthor
#7 gnode-1176672-7.patch17.38 KBtim.plunkett

Comments

alexreinhart’s picture

alexreinhart commented
Status: Needs review » Needs work

I've taken a look through your code; some comments:

  • You use PHP 5 features, so you may want to note in your .info that PHP 5 is required.
  • In several places, I think you can use module_invoke_all instead of your own hand-rolled foreach() to call other module hooks.
  • Does gnode_bundle_load need to check that a result was, in fact, found before calling $result->fetch(), or is the error-handling hoisted to the caller? Same goes for a few other functions.
  • You have a spare semicolon on line 389 of gnode.module.
  • gnode_config_form in gnode.config.inc forgets a t() around a form #title element.

And... that's it. You have some squeaky clean code. Just set this back to "needs review" when you're done and I'll take another look through.

ccardea’s picture

ccardea commented

Alex,

Thank you for taking the time to review my project. I appreciate both the substantive and detailed nature of the review.

Re: using module_invoke_all.

I'm aware of the module_invoke and module_invoke_all functions. The reason I chose to use my own foreach for all of my entity hooks is because that is how it is done in the attachLoad function of the Drupal default entity controller. See example below from entity.inc. I know that there have been problems with the module_invoke functions because they cannot pass a reference as a parameter, and some other issues too. I don't know if this entered into the decision not to use module_invoke in entity.inc, but if its all the same to you, I'd prefer to stay with this technique for consistency.

// Call hook_entity_load().
foreach (module_implements('entity_load') as $module) {
$function = $module . '_entity_load';
$function($queried_entities, $this->entityType);
}

Re: bundle_load function

The fetch functions return null or false if there are no results, so the answer is that no-result conditions are handled by the caller. Based on this comment I re-tested the module for cases where a database query returns no results. I added error handling code where it was needed.

Re: PHP 5
I checked into this, to get the right format to do this, but I found out that, according the developer docs,

"Modules should generally not specify a required version unless they specifically need a higher later version of PHP than is required by core." Drupal 7 now requires PHP 5.2.5, and recommends 5.3. I've avoided using code that is PHP 5.3 specific, so I think I'm OK without the declaration. As far as I know, I don't require anything higher than what Drupal core requires.

I removed the extra semicolon and added the t() to the #title in the config form, and have committed all of the changes.

Thank you again for the review.

ccardea’s picture

ccardea commented
Status: Needs work » Needs review
alexreinhart’s picture

alexreinhart commented
Status: Needs review » Needs work

Interesting about the PHP 5 stuff; another day, another interesting bit of the Drupal regulations.

A couple more comments:

  • In your changes, you add a drupal_set_message() call to gnode_bundle_form(), but you do not apply t() to the error message. It should be translatable.
  • In gnode.bundles.inc in gnode_bundle_form(), you have some lines like this:
    '#default_value' => $bundle ? t('@value', array('@value' => $bundle->gnode_type)) : ''
    Why do you need to pass the data through t() here? It just does check_plain() on the @value parameter, and you don't have any translatable text there.

Otherwise, looks good. I'll trust that whoever wrote attachLoad() knew what they were doing.

ccardea’s picture

ccardea commented

Thanks for pointing this out. I've made the changes you suggested. I found out that Form API runs the #default_value element through check_plain(), so there's no need even for that. Thanks again.

alexreinhart’s picture

alexreinhart commented
Status: Needs work » Reviewed & tested by the community

Looks good.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new gnode-1176672-7.patch17.38 KB

Minor coding issues, apply this patch and set back to RTBC, and I should be able to give you the vetted git status right away.

ccardea’s picture

ccardea commented
Status: Needs work » Reviewed & tested by the community

Thanks for the patch. Applied, committed, and pushed to repository.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed

For future commits, please read http://drupal.org/node/52287.

For example, that one could have been Issue #1176672 by tim.plunkett: Improve documentation formatting.

Thanks for your patience, and your help on other applications. See you around the queue!

ccardea’s picture

ccardea commented

Sorry, I should have given you credit. That was a lot of work that you did, and I appreciate it. Thanks for your help, and for taking care of my application.

Status: Fixed » Closed (fixed)

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