In #1777934: Delete geo data when address becomes empty we introduced some logic to empty the geofield if there is supposedly nothing in it. This causes problems as we return false from geocoder_widget_get_field_value() if we are updating an item with no changes so as to not unnecessarily geocode again.

Comments added:

// if we have a value, super, let's assign it to our geofield
      if (($field_value = geocoder_widget_get_field_value($entity_type, $field_instance, $entity)) !== FALSE) {
        $entity->{$field_instance['field_name']} = $field_value;
      }
// otherwise, set it to empty.
      else {
        $entity->{$field_instance['field_name']} = array();
      }

geocoder_widget_get_field_value() is the real problem here as it is (perhaps) attempting to serve a purpose not on the tin, it tries to compare field values on the original and the new entity and returns FALSE if there is no difference to prevent geocoding again. With the new logic introduced in #1777934: Delete geo data when address becomes empty we wipe the geocode everytime a node is saved and the address is not changed.

    // For entities being updated, determine if another geocode is necessary
    if ($entity) {
      if (!empty($entity->original)) {
        //@@TODO: Deal with entity-properties (non-fields)
        //@@TODO: This isn't working with file fields. Should use some kind of lookup / map
        $field_original = field_get_items($entity_type, $entity->original, $field_name, isset($entity->original->language) ? $entity->original->language : NULL);
        if (!empty($field_original)) {
          $diff = geocoder_widget_array_recursive_diff($field_original, $source_field_values);
          if (empty($diff)) {
            return FALSE;
          }
        }
      }
    }

But wait you say, this isn't true, I just saved a node and it kept the geocoded data in my geofield. Alas, AFAIK the reason this issue isn't exposed in a normal form node update is the entity object is only populated with stuff about to get saved from the form, not all the null value properties that often exist on an addressfield. When we try to save an entity in rules and we aren't messing with the geocoder widgets source field the geocoded data is wiped.

Make sense?

What to do? I think we should simply do what is on the tin and return a value if there is one from geocoder_widget_get_field_value(). That maaay not cover all bases but here's a shot.

NOTE: This does not address the fact that a node update (via the form) will cause an unnecessary geocode on addressfield data even if it hasn't changed as it doesn't have all the null properties set which geocoder_widget_array_recursive_diff() sees in the original field.

CommentFileSizeAuthor
#19 geocoder-delete-geo-date-when-address-empty-1.2-1992762-19.patch462 bytesjp.stacey
#16 delete-geo-data-when-address-empty-1992762-16.patch604 bytesdbassendine
#4 delete-geo-data-when-address-empty-1992762-4.patch833 bytesjackbravo
fix-programmatic-entity-updates-with-geocoder-widget.patch665 bytesjpstrikesback
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jpstrikesback’s picture

jpstrikesback CreditAttribution: jpstrikesback commented
Status: Active » Needs review
jpstrikesback’s picture

jpstrikesback CreditAttribution: jpstrikesback commented
Title: Programmatic Saving of an entity empties a geofield due to regression introduced/exposed in issue #1777934 » Programmatic Update of an entity empties a geofield due to regression introduced/exposed in issue #1777934
jackbravo’s picture

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

This works for me as well.

To me, this problem became apparent while using the workbench_moderate module. If you enable moderation for a content type then the geofield value is not saved because of the way the module delays the saving of the actual published node.

The patch fixes the issue.

jackbravo’s picture

jackbravo CreditAttribution: jackbravo commented
Status: Reviewed & tested by the community » Needs review
FileSize
delete-geo-data-when-address-empty-1992762-4.patch833 bytes

Actually, the fix implemented in #1777934: Delete geo data when address becomes empty did not actually solved the bug, because it broke the functionality described in the function geocoder_widget_get_field_value where returning FALSE means doing nothing.

Here is an alternative solution to both issue #1777934: Delete geo data when address becomes empty and this one. What we do is remove the code that deletes the field data when returning FALSE (this issue) and instead returns an empty array when the geocoder source field is empty (solving that issue).

Patch attached.

Countzero’s picture

Countzero CreditAttribution: Countzero commented

Thanks a lot for this last patch. Before, I couldn't save any geocoding info on my user entities ; now I can.

At least, by saving manually. Still no luck with VBO and the 'Arbitrary PHP script' action (which DOES work and correctly saves the entity with some other data). Maybe I have to indicate something in the entity for the saving to actualy happen ?

Here the code I use :

 if ($entity = _nmrz_cp_to_dpt($entity)) { //  This function returns TRUE almost all the time
   entity_save('user', $entity);
 }
jpstrikesback’s picture

jpstrikesback CreditAttribution: jpstrikesback commented

@Countzero, did you happen to try the patch in the original post?

Countzero’s picture

Countzero CreditAttribution: Countzero commented

Just did, with a brand new module install, it has the exact same effect than the other : manual saving does work but still can't manage to save programatically. Tried with both stable and dev versions.

Will try to dig into this today, but I don't expect much as you guys seem to be quite advanced in the understanding of the problem.

jmdeleon’s picture

jmdeleon CreditAttribution: jmdeleon commented

Patch in #4 worked for me. Was able to publish nodes with geocoded info.

isaac77’s picture

isaac77 CreditAttribution: isaac77 commented

FWIW: I manually applied the fix in patch from #4 to 7.x-1.2. It seems to work well and fix the issue described at #1777934: Delete geo data when address becomes empty for me. Thanks!

ergophobe’s picture

ergophobe CreditAttribution: ergophobe commented

Patch in #4 works for me.

ergophobe’s picture

ergophobe CreditAttribution: ergophobe commented

Just to clarify, I was having the issue with being unable to unset a geocoded location as in #1777934: Delete geo data when address becomes empty. With this patch I successfully

- created a node with a geocoded location (using geofield populated with values geocoded from addressfield)
- resaved it with an unchanged location without losing the geocoded field
- deleted the location data from the addressfield and saved without leaving behind any geocoded info

Excellent solution

jmoughon’s picture

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

Patch in #4 works for me. Recommend committing.

Simon Georges’s picture

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

Thanks for the feedback, guys!
Committed.

Status: Fixed » Closed (fixed)

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

dbassendine’s picture

dbassendine CreditAttribution: dbassendine commented
FileSize
delete-geo-data-when-address-empty-1992762-16.patch604 bytes

Patch #4 did not apply cleanly against 1.2 so re-rolling. Not yet tested.

deanflory’s picture

deanflory CreditAttribution: deanflory commented

Isn't 1.2 a whole two years older than the current dev release? Shouldn't all patches be made against the current dev release?

Simon Georges’s picture

Simon Georges CreditAttribution: Simon Georges commented

Patch has been committed into -dev a month ago. I'm still trying to find enough time to stabilize a new release.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey commented
FileSize
geocoder-delete-geo-date-when-address-empty-1.2-1992762-19.patch462 bytes

@Simon a new release would be really appreciated. In the mean time we do need to reroll this patch against 1.2.

I think the patch in #16 alters the wrong `return FALSE;` compared to the patch committed to -dev. I've re-rolled 16 and attach to this comment. Tested here and it seems to work OK.

dbassendine’s picture

dbassendine CreditAttribution: dbassendine commented

Thanks J-P - I've tested this patch independently against the same project and can confirm that #19 works against 1.2, whereas #16 does not.

jackbravo’s picture

jackbravo CreditAttribution: jackbravo commented

No new rerolls are needed, the code is already commited =). If you want to use it either download the module using git or download the dev version.