Give an option to not show geocoding error messages

If it said "Error geocoding - No results", would that be better?

What do you want to see happen?

Agree! This could be shown for privileged users.

I agree. In my case I am using this module as an API and calling the geocoding functions from elsewhere.

In the case of an error like ZERO_RESULTS, I use a default value for the location in the backend and don't want any messages displayed on the screen for the error.

Maybe if it were a setting you could enable / disable the error messages?
A wrapper around drupal_set_message() that checks the value of a $debug variable or similar maybe?

I can manually remove the messages in my custom code but that isn't a good solution.

It should log it to watchdog though I think. Or maybe that could be an option too :)

Hello

I'm working on this issue form Barcelona Drupal Dev Days sprint.

I think the biggest problem is that each geocoder displays its own errors.

That implies each plugin will manage the error messages.

In my opinion, will be aesier if all the geocoding plugins send the message to the geocoder module. Then, geocoder module could check the variable by itself and decide if the error message should displayed or not and stored in watchdog or not.

What do you think?

Regards.

Hello

I'm working on this issue form Barcelona Drupal Dev Days sprint.

I think the biggest problem is that each geocoder displays its own errors.

That implies each plugin will manage the error messages.

In my opinion, will be aesier if all the geocoding plugins send the message to the geocoder module. Then, geocoder module could check the variable by itself and decide if the error message should displayed or not and stored in watchdog or not.

What do you think?

Regards.

The main module should not have to know about individual plugins and their messages. If you want to show or hide all geocoding messages or all geocoding error messages then that's cool with me. You'll need to specify the type of message in your message callback like drupal set message does so you can selectively purge/log them.

Okay, I'm going to change it to throw PHP warning. That way it can be suppressed as wanted in admin settings.

Okay,

This is now fixed. Please re-open if you have problems or concerns

I have the 2012-Jun-20 dev version of Geocoder installed; I cleared my cache and No Warning Messages are set to display under Development: Logging and Errors.

Although the PHP warnings from other modules on my site are not displayed, when logged in as an authenticated user I am still getting the "Error geocoding - ZERO_RESULTS" warning from Geocoder. Authenticated users have no Geocoder permissions. Is there something else I need to do to hide the ZERO_RESULTS message?

Thanks in advance for any help you can provide.

There are still some other places were errors might be being thrown. I've attached a patch where I've changed all the ones I think shouldn't be seen by the end user to watchdog error messages so they can be suppressed if necessary.

I've left the drupal_set_message() call on line 278 of geocoder.module in:

function geocoder_service_check_request($handler, $format, $check_ac = TRUE) {
  if (!geocoder_get_handler($handler)) {
    drupal_set_message(t('Could not find handler @handler', array('@handler' => $handler)), 'error');
    drupal_not_found();
    exit();
  }

As it seems this is intended for the end user.

Same with line 18 of plugins/geocoder_handler/exif.inc

function geocoder_exif($url, $options = array()) {
  // Invoke hook_file_download to check permissions
  // We are essentially duplicating the logic found in file.inc file_download()
  foreach (module_implements('file_download') as $module) {
    $function = $module . '_file_download';
    $result = $function($url);
    if ($result == -1) {
      drupal_set_message(t('You do not have permission to access this file'), 'error');
      return FALSE;
    }
  }

I've also changed the trigger_error calls to use watchdog. I'm not sure if Drupal has a policy when trigger_error or watchdog should be used so if anyone more knowledgeable would suggest changing it back I'd be happy with that.

I attempt to apply the patch against the latest dev version (2012-Jun-20):

Hunk #1 FAILED at 181.
Hunk #2 succeeded at 232 (offset -33 lines).
1 out of 2 hunks FAILED -- saving rejects to file geocoder.widget.inc.rej

Here is the content of the .rej file:

***************
*** 181,187 ****
   */
  function geocoder_widget_get_field_value($entity_type, $field_instance, $entity = NULL, $source_field_values = NULL) {
    if (!$source_field_values && !$entity) {
-     trigger_error('geocoder_widget_get_field_value: You must pass either $source_field_values OR $entity', E_WARNING);
      return FALSE;
    }
    
--- 181,187 ----
   */
  function geocoder_widget_get_field_value($entity_type, $field_instance, $entity = NULL, $source_field_values = NULL) {
    if (!$source_field_values && !$entity) {
+     watchdog(GEOCODER_WATCHDOG_TYPE, 'geocoder_widget_get_field_value: You must pass either $source_field_values OR $entity', array(), WATCHDOG_ERROR);
      return FALSE;
    }

This patch is against the current HEAD of the repository, not the dev release.

Opened #1660992: Current geocoder dev release doesn't contain latest code.

This patch needs to be re-rolled against latest dev now. Sorry for the confusion folks

No worries. Rerolled the patch and attached it below.

I've also made some more changes:

1. I've reverted one of the trigger_error() functions. It seems the first one has more to do with how the function will be called rather than geocoder itself so it would make more sense to associate it with a generic php error rather than geocoder. Once again I'm more than happy to see this change if anyone feels strongly about it.
2. I dropped the define statement as it doesn't seem to be commonly done in other modules.
3. Changed the watchdog type to all lowercase to match other modules.
4. I've wrapped code in try/catch blocks where possible so we can make use of watchdog_exception().
5. I've changed any lines in geocoder_google() which didn't comply with Drupal's coding standards (http://drupal.org/coding-standards/). There are quite a few more exceptions but these should probably be dealt with in a separate ticket.

Looks good. I've committed it.