Update XML parse errors make installation fail

I wonder why update.module is checking for updates already during install.php...

Anyway, sounds like this is really a bug report against update.fetch.inc's error handling and recovery ("update.module"), not something about update.php ("update system"). Moving to the right component so update_status-enabled folks (namely me, but hopefully others) can find this.

This also causes cron runs to die with fatal uncaught exception. We just need exception handling in update.fetch.inc. The actual line where the exception is thrown is $xml = new SimpleXMLElement($raw_xml);

Parse errors also trigger PHP warnings, so it may not be necessary to do any extra logging. I catch the exception with break; rather than return; so we move on and try to parse the next XML string.

Cool, that all sounds reasonable and like a good move in it's own right. However, I'd still like to understand why install.php is triggering an update status fetch, and see if we can just avoid that from happening entirely. Seems like it's needlessly slowing down the installer for something we can do when they finally visit their site.

Really seems like we've got two basically unrelated bugs here: a) install.php shouldn't trigger update status at all, b) update status needs exception handling. We should probably move mfb's nice patch to a new issue just for (b), and leave this issue to work on (a). In the new issue for (b), it'd also be nice to add a test for this, but that should probably wait until #591632: Refactor tests to allow testing contrib lands to avoid patch conflicts...

This seems quite intentional:

/**
 * Installation task; perform final steps and display a 'finished' page.
 *
 * @param $install_state
 *   An array of information about the current installation state.
 * @return
 *   A message informing the user that the installation is complete.
 */
function install_finished(&$install_state) {
...
  // Run cron to populate update status tables (if available) so that users
  // will be warned if they've installed an out of date Drupal version.
  // Will also trigger indexing of profile-supplied content or feeds.
  drupal_cron_run();
...
}

Ahh, right, added via #93509: Cron has not run ... hrm. I guess there are good reasons we need that. Alas.

Turns out adding a simpletest for this is basically impossible with our current testing infrastructure. Writing the test itself is no problem, especially now that #591632: Refactor tests to allow testing contrib landed. The problem is that our testing infrastructure provides no way for a test to declare any expected exceptions. As far as the test framework knows, *any* exception is a testing failure. :(

So, until that's resolved, there's really no way to have an automated test that sends in busted XML and makes sure that update.module recovers somewhat gracefully.

Given that, adding some exception handling at all is better than fatal errors on invalid XML, so I support committing this patch now. We can always clean it up in the future, add tests, etc.

Thanks, mfb!

That break statement is a little funny. Normally, as far as normally goes, you'd put more stuff in the try block and have an empty catch handler. Thoughts on that?

I don't know, generally you want your try as close to where you're error will be thrown as possible. The break is a little odd looking but the way the try catch is setup makes it clear what error we're catching and the behavior we want associated with that specific error. I do think it would be nice to have a comment telling us why we're breaking out and what the break is doing to the flow.

It's not documented what SimpleXMLElement::__construct returns if there is a parse error, just that an exception is thrown. So I feel like this is a safe way to handle any exception. I added a comment.

Looks good. There are too many tiny little conditions that could trigger this and it all depends on valid data from updates.drupal.org, so I don't think this is worth adding tons of tiny little tests.

Committed to HEAD, thanks.

Might be time to revisit #301005: Add "expected fail" functionality to simpletest so that we can test for things like this in the future?