Update.module automated installer should not permit download of Drupal core

We should not update the drupal-code with the automatic module installer.

We should detected if what we are download if in fact one Drupal Module. (e.g. find and read the info file)

Retitling issue to better describe required fix.

This implements a check for bootstrap.inc in a new update_verify_update_archive() (similar to #936490: Update module should verify downloaded tarballs and propagate errors correctly).

Screenshot coming up.

And the screenshot.

The @see's where leftovers from #936490: Update module should verify downloaded tarballs and propagate errors correctly.

Wow, the test bot is picky these days :)
Now with a properly initialized variable.

Just tested this, and it stopped me from installing Drupal 7 beta 3 from tarball, as expected. Here is a screenshot of the error it throws when you try it: https://skitch.com/evandonovan/rbwni/updating-drupal-core.

The only possible improvement would be if the error it throws would like to the upgrade guide. Something like, "Updating of Drupal core is not supported. To learn how to update Drupal core, please read the upgrade guide."

Just as an FYI to other testers: this patch and the one in #936490: Update module should verify downloaded tarballs and propagate errors correctly implement the same hook, so you can't have them applied at the same time, and one will need to get re-rolled after the other is committed.

#8 sounds like a pretty awesome idea.
New string:

Automatic updating of Drupal core is not supported. See the upgrade guide for information on how to update Drupal core manually.

Just a simple string change. Patch should still be good.

Thanks! I would mark RTBC but not sure if there are other technical implications.

This implementation is just plain weird.

If the function returns (i.e. the return statement is executed), it will return an empty array. Also, the variable is set as $failure[...] instead of $failures[...] (extra s), but that doesn't really matter...

Just the if-construct and the exception should be enough. Also, document why we throw an exception instead of following the API docs for hook_verify_update_archive().

Well, the thing is that (at least that's what I thought) we want a dedicated error message for this to guide user's what the exact problem is. The API doc for hook_verify_update_archive mentions putting something in $failures, but then doesn't use it, it just uses the exception. Since there is a empty($failures) check, I had thought we need to place something in $failures, but as you correctly point out, my code doesn't actually do that, but still seems to work. I will investigate, whether we have to return anything at all or not.

Well, it seems this actually works, so why not?

Just to make it clear: update_manager_archive_verify() (which calls hook_verify_update_archive()) doesn't use the exception you throw. It just throws an exception when $failures is populated. However, when the exception in update_verify_update_archive() is thrown, the rest of the code in update_manager_archive_verify() is skipped (and so is other code until the exception is catched, which is where code execution is resumed).

Thinking about this some more, I'm not happy with throwing an exception in update_verify_update_archive(), as it violates the Liskov principle (which is really an OO concept, but the idea remains the same) here. The hook_verify_update_archive() API does not mention any exceptions thrown. This means code executing this hook shouldn't need to bother about exceptions, but if they don't: BANG!

The only solution which does not require an API change (or violation) is returning $something for which !empty($something), but then we have no way of setting an error message. I would suggest (for D8 if it's too late for now) changing hook_verify_update_archive() to return an error message to be set, instead of just anything not-empty, and using this error message in update_manager_archive_verify(). In that case there should be a TODO about it, either at update_verify_update_archive() or at update_manager_archive_verify().

Oh, and also needs work because the comment following the function declaration should be on a separate line.

Not entirely sure what all that means, but are you saying that there's no way to properly set an error message in this function? I think that we have to be able to set an error message, even if it's technically the incorrect way to do it, since the UX will be terrible if not.

We could also use drupal_set_message(), I suppose. Maybe we can get another review on the patch. I don't really care for the implementation. Except that, as you portray, using the hook in the documented way doesn't make much sense, as you don't get a custom error message.

@tstoeckler: Why don't we just use drupal_set_message() and call it a day then? :)

Sounds like there are issues with the hook that can't be fixed at this juncture, whereas this issue needs to be fixed.

As with many parts of the update manager, we put in some hooks in the hopes that they'd be helpful, but no one ever tried using them until now. So, it's entirely possible that the existing hook API is broken and needs to be fixed. I'd be in favor of that, instead of hacking around it and trying to propagate errors solely through drupal_set_message(). Error propagation already sucks a lot in the update manager. We shouldn't add to that suckage here if we can help it.

p.s. Is this really the best we can come up with to finger-print a tarball includes core?

+  if (file_exists("$directory/$project/includes/bootstrap.inc")) {

That seems awfully fragile. I might have an includes/bootstrap.inc in a contrib module. If we added another 4 or 5 tests (e.g. for index.php, update.php, modules/system/system.module, and modules/node/node.module) all of which had to be TRUE before we triggered the error, I'd be a lot happier.

FWIW, the wording of the message is perfectly fine.

In the interest of getting this done in time for rc1, I'm merging this issue into #936490: Update module should verify downloaded tarballs and propagate errors correctly. Let's continue over there. Thanks.

#3: 958046-update-manager-download-core-3.patch queued for re-testing.