I get a "Trying to get property of non-object xmlsitemap.generate.inc:169" when running drush elysia-cron on the command line in a normal cron run (when hook_cron runs in drush)

php 5.3.16
drupal version 7.15
drush version 5.4
xmlsitemap version 7.x-2.0-rc1

the offending line is the middle line:

    if ($url_options['alias']) {
      $link['loc'] = xmlsitemap_get_path_alias($link['loc'], $link['language']->language);
    }

I'm thinking the appropriate fix might be something like:

    if ($url_options['alias'] && isset($link['language']->language)) {
      $link['loc'] = xmlsitemap_get_path_alias($link['loc'], $link['language']->language);
    }

Something like that to make sure the code only executes if it's a valid object. It might be a larger issue but that should at least stop the PHP notices.

Comments

Anonymous’s picture

Anonymous (not verified) commented
Category: bug » support
Status: Active » Postponed (maintainer needs more info)

You need to find the module that is incorrectly creating a link object and file a bug report with that module. Bandaiding xmlsitemap to not create links when the link object is incorrectly formed serves no purpose but to hide a problem.

To find the module with the issue it is suggested that you start removing modules until you find the one that no longer presents the issue. Then turn back on each one removed one at a time testing along the way to ensure that none of them have an issue.

mike503’s picture

mike503 commented

Considering our site has something like 200 modules, that's not very realistic.

I don't consider sanity checking a "bandaid", I consider it proper coding. It is arrogant to assume your data will always come through exactly as you want. It should be checked, sanitized and/or normalized, depending on the case. Is it really that hard to add a couple lines of code around a statement to check if there are the right array indexes, or object properties?

Anonymous’s picture

Anonymous (not verified) commented

I don't consider sanity checking a "bandaid", I consider it proper coding.

You will be hard pressed to make this argument in Drupal. It is the attitude of core development and thus the attitude I take here.

It is arrogant to assume your data will always come through exactly as you want.

It isn't arrogant when it is status quo. You will need to find the module that is incorrectly giving the data. This is the Drupal way in order to have a speedy system without needing to assert requirements. It is expected that the data you give is sane and that if you give dirty data the result is unknown. When programming in Drupal I often will take my development version and insert code to help find where the issue is. But that inserted code does not remain for a production system. Module distributions must be production ready and not a development version. One option is to insert a debug_backtrace when the expected data type or data element isn't appropriate just to find the point in the module that is causing the problem. You can deploy the same technique here.

mike503’s picture

mike503 commented

It is the attitude of core development and thus the attitude I take here.

Might be why switching to Drupal 7 we began to see a lot of PHP notices being thrown... not many anymore. Mainly all from contrib; which is going to happen. You have to realize, you are writing a contrib module and interacting with other contrib modules. Not all of them are as "sane" as you believe in your world. Drupal core may be able to save some of this because they operate in a controlled codebase, not one extended by contrib modules (those violate core if they're causing notices/errors from core, and should be fixed then)

It isn't arrogant when it is status quo.

"If everyone else jumped off a bridge, would you do that too?"

This is the Drupal way in order to have a speedy system without needing to assert requirements. It is expected that the data you give is sane and that if you give dirty data the result is unknown.

"Speedy" is a relevant term here. Adding output/strings to the PHP errors/notices buffer (not to mention watchdog, webserver logs, PHP logs, whatever else) is probably on the same level or more expensive on the macro level than a simple isset() assertion call, which is not a function and has special opcodes as such.

For a function like this especially, since it is only triggered very infrequently (cron is the only time I see it) this concern over performance or something is meaningless. The amount of CPU time, disk space, I/O and everything else due to these messages far outweighs lightweight isset() calls.

I've tried to pitch my case, it's obvious we don't agree, I've even given you the basic code to fix this, but you're not interested and want to push blame somewhere else (how many hours of hunting will that take with hundreds of modules, multiple content types, fields, taxonomy definitions...) vs. a 2 second commit + maybe a few extra ms per call on a cron process ...

Anonymous’s picture

Anonymous (not verified) commented

You have a problem. The problem has yet to be proven to be xmlsitemap. Something somewhere else is causing your system to fail because of dirty data. When you put dirt in you get dirt out. It is your responsibility to find where that dirt is coming from, not xmlsitemap's to filter it out. What else might that dirt eventually cause?

To find the culprit do the following code:

if (!isset($link['language']->language)) {
  drupal_set_message('<pre>'.print_r(debug_backtrace(),TRUE).'</pre>');
}
else {
  if ($url_options['alias']) {
    $link['loc'] = xmlsitemap_get_path_alias($link['loc'], $link['language']->language);
  }
}

You can paste the backtrace into a comment here if you wish.

Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes

added the versions of php/drupal/etc.