Update module should verify downloaded tarballs and propagate errors correctly

Hm. Yes, yes it should. How did you manage to do this though? This is what I get:

Oh! I get it. This is a bug with update.module validation, then.

This doesn't feel like a release-blocker, though.

No patch, so marking "active" again.

- uninstall doesn't uninstall from the filesystem, only from the database.

- I guess the UpdaterTheme and UpdaterModule classes can try to have some logic about parsing .info files in the directories that it unpacks to see if the .info files are going to be allowed by the rest of core. But what happens if a tarball happens to include some modules that are compatible and others that are not?

It seems that the problem is that something is trying to enable the new theme automatically. That seems like the real bug...

If a tarball contains some modules that are compatible and some that are not, then it would probably be very difficult to detect the incompatibility in advance. But I think that case is pretty rare especially if the tarball in question came from drupal.org. Is it possible/acceptable to parse the actual filename of the tarball for things that could indicate an incompatibility (for example if the filename contains '6.x' or '5.x') then just display some message like "The archive you are about to upload may be incompatible with this version of drupal"? Then the user can choose to ignore the message or find the compatible version. If they ignore the message and the package truly is incompatible then they are just not able to enable the module/theme which is the current behaviour, but at least they had some advanced warning.

Perhaps the filename could be parsed and warning displayed with javascript similar to the password strength indicator. If the user has javascript disabled, then it degrades to the current behaviour which is not so bad as long as nothing is trying to automatically enable the theme/module as may be the case for shuklasp and the fusion theme.

I could be way off here, but I just wanted to throw the idea out there since discussion about update manager is in the critical queue now. Also, I'm willing to help with these issues in any way I can. I'm really new to contributing to drupal, but I like to learn, so if there is any way I can help without stepping on toes or getting in the way, I'd love to.

-Josh

Adding fancy doo-dads like compatibility meters isn't on the table for D7 anymore, because we're in UI freeze.

I'm not sure that a simple filename check is enough. I might download a module from an issue somewhere that's still in the process of being written, and it'll be called "foo.tar.gz/zip", not following Drupal.org's naming conventions.

But having some help on Update Manager would definitely be extremely welcome! :D

@shuklasp Re: #12: Unpacking to a temp folder is already how the Update manager works. If this is going to be fixed in D7, we'd need to add code that inspects that temp folder and tries to check compatibility first. This can all still happen inside update.manager.inc itself at a full bootstrap, before we even try to get to the authorize.php step. There's some existing code to check that the tarball you're working with is valid before we try to hit authorize.php, we just need to add more logic to that.

@see update_manager_archive_verify()
@see hook_archive_verify()

;)

This copies a bit of drupal_system_listing into a new
update_verify_update_archive() (Implements hook_verify_upate_archive())
It is pretty straightforward and I tested it locally.
Screenshot coming up.

And the screenshot.

Progress! I'll be able to test this tonight.

Cool, this is a great start. Thanks! Needs work for the following reasons:

A) The comment doesn't match the code:

+ * Implements hook_archive_verify().
...
+function update_verify_update_archive($project, $archive_file, $directory) {

Which is it? hook_archive_verify() or hook_verify_update_archive()?

B) This seems wrong:

+    // If it has no info file, then we just behave liberally and accept it.

The point of this issue is to prevent people from trying to use the update manager to install/update things that core's going to freak out about. If there are .module files without corresponding .info files, core's not going to let you enable those. Seems like we should be throwing a different exception in this case about modules without .info files. I don't know what we win by preventing wrong .info files but still allowing missing .info files (which core can't install).

C) Do we want update.module to implement its own hook? Or should we put this code directly into the function in update.module that's invoking this hook for everyone else. I'm not sure how we handle this in other places in core, so I'm just raising it. I could sort of argue both sides of it. Con: nit's another layer of code complexity and the cost of another function call. Pro: at least there'd be an example in core of using this hook. I don't care that much, so long as there's precedent (or ideally a convention/standard) in core for how we handle this in other hook-invoking modules...

A) Will fix that. It is, as can be seen from the function name, hook_verify_update_archive(). The differing function names just confused the hell out me :)

B) I just copied that from drupal_system_listing(). The original comment is:

      // If it has no info file, then we just behave liberally and accept the
      // new resource on the list for merging.

which I shortened, because it didn't really apply here in that way. If I understand it correctly it allows modules to be overridden (e.g. placing a "translation" module in sites/all) without overriding the info file, but only the .module file (e.g. there's no sites/all/translation/translation.info, only a sites/all/translation/translation.module, but the module is still overridden). I'm not sure, though, and anyway it seems like a pretty weird edge case. Here's the link for convenience. http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_sy...
Throwing an additional exception sounds like a pretty good idea.

C) I don't really care and will do whatever gets this committed, but I personally like to separate hook invokations and hook implementations because I prefer the clear architectural separation.

I'll roll a patch later today or in the next couple days and will incorporate the above or any further feedback that has been given until then.

Also adding the "string freeze" tag, because this has to go in before RC, if at all.

Updated patch for A) and B)
Kept it as a hook_implementation for now.

Oops.

Just as an FYI to other testers: this patch and the one in #958046: Update.module automated installer should not permit download of Drupal core 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.

I tried this by downloading the Zen 6.x tarball and got:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: http://localhost:8888/d7-latest/authorize.php?batch=1&render=overlay&id=... StatusText: OK

I don't know whether that is related at all to this patch, though, or whether it is an unrelated Update Manager issue.

I tried again by downloading the Views 6.x tarball, and this time the patch worked as expected. Here is a screenshot of the error I received: https://skitch.com/evandonovan/rbwgb/modules-incompatible.

I think the other time might have been an issue unrelated to this one. Both times I was using the "install from a URL" functionality.

Well this patch needs to be reviewed. If you are not up for a code review.
Simply go admin/modules/install and enter the URL of a Drupal 6 module tarball in the "Install from URL" field (hint: small modules are obviously faster, so you might not want to go with Views)
- Without the patch you should reach the next page, where you have to enter your FTP credentials, etc.
- With the patch you should get an error message, saying that the module is not compatible with your version of core.

Tested, patch works as expected.
Code looks fine.

Small nitpick:

+      throw new Exception(t('One or more modules in the archive are missing an info file.'));
+      throw new Exception(t('One or more modules in the archive is not compatible with Drupal 7.x.'));

Shouldn't the first message then be "One or more modules in the archive is missing an info file."
Sounds more natural too, but then again, I'm not a native speaker.

Oops. Actually I think the plural "are" is correct here. I'm not a native speaker, either though. It should be consistent either way.

Generally speaking, if you've got singular and plural words connected by the word or, then usually the verb agrees with the word it's closest to.
--> singular or plural are
--> plural or singular is

You're right :)

Yes, both should be "are".

Do you think you need to re-work this patch, as well as the other patch, to use drupal_set_message() as per the comment in #958046-15: Update.module automated installer should not permit download of Drupal core?

These are both string freeze patches, though, so I think that today is the last day they could get in....

This issue and #958046: Update.module automated installer should not permit download of Drupal core are both trying to do similar things, the patches automatically conflict (since they're both trying to add an implementation of hook_verify_update_archive()) and they're both running into the same limitations of how that hook can't propagate errors. In the interest of getting all of this fixed and working before 7.0-rc1, I'm going to merge the two issues and fix the API all at once. Trying to handle this in 3 inter-dependent issues can't work, especially this close to string freeze and release. Stay tuned...

Thanks, dww!

I don't think this hook is going to work well trying to be based on Exceptions. Instead, it should just return an array of error messages. If the hook invocation results in an empty array of errors, we're happy. If the array has any content, we treat those as error messages to display. The different call sites handle these messages differently. In the install case, they're form errors (although EVIL, there's no clean way to set multiple errors on the same form element and have all of them appear -- see the hack and the @todo in the patch). In the update case, they're stashed in the batch results and displayed once everything is downloaded.

This probably still needs some UI help (e.g. we don't retain the URL they tried to use (not sure exactly why), so we might want a summary error message saying something like "The $URL contains an invalid release"), but this is a *LOT* closer...

I still need to test the update case, but that's harder, since we already need something valid installed, but an available update would have to be invalid. I'll probably have to cut a new broken release of http://drupal.org/project/update_test_module to test with.

Would it be possible to change the error message for when a module or theme is incompatible to have the module's name rather than the name of the .info file?

Something like $failures[$file->name] = t('!plugin is not compatible with Drupal !version.', array('!plugin' => $info['name'], '!version' => DRUPAL_CORE_COMPATIBILITY));.

In any case, I think "included in the archive" is a bit technical and might confuse people. At the least, it should be in commas, since it is a separate clause.

Based on IRC chat, here's a new patch.

This can't work: "!plugin is not compatible with Drupal !version"

It'd result in core printing something like "Views is not compatible with Drupal 7.x" which is sheer nonsense.

So, we went with this: "The version of %name included in the archive is not compatible with Drupal !version." See screenie.

I think this is great. Can't test tonight though - have to stop soon, but should be RTBC, if the technical architecture is good.

So, this patch didn't make it in before RC1, and represents string freeze breakage. However, I'm ecstatic about the amount of momentum we've managed to build around update.module, and I think the usability WTF here is severe enough that it's worth breaking string freeze to get this patch in once it's ready.

New patch with a slightly different approach to make the error messages less overwhelming and more useful:

- Group all the "incompatible with 7.x" modules/themes into a single message. The patch is using format_plural to avoid a : list of 1 thing. We could simplify the patch if we want. Note that we have to use "modules or themes" for this since we don't really have a great way of knowing which we're dealing with during this particular check (without more complicated logic in an already somewhat logic-heavy patch post rc1) given the outcome of #606646: Standardize UI language for "projects" and the generic term for a module or theme.

- Group all the "... is missing an info file" modules into a single message. In this case, we know we're only searching for modules.

Last night when I couldn't get back online, I hacked my locally cached copies of a few release tarballs to trigger the various error cases in the install case. I couldn't test update then (and haven't yet this morning), but this will at least show the logic about generating the errors is correct, and that displaying them works in the form_set_error() approach via the install UI. We still need to test if the update case propagates and displays the errors correctly, and I'll probably make a series of new releases over at http://drupal.org/project/update_test_module to facilitate that.

p.s. Forgot to mention that the error message also now includes the name of the archive file, since we don't preserve that across the form submission (this isn't just a validation step -- we can't validate the archive until you actually submit the form to trigger the download, etc) so that you at least see the filename you used, and it avoids the slightly jargony "in the archive" text.

For UX improvements of #38 over #35, compare these two:

#35

#38

Sorry for the inconsistent filenames -- the one in 35 should have been ".incompatible-modules" since it's showing what happens with an archive containing 4 incompatible modules...

Thanks, dww! I remember we talked about this, and I think this is a lot better.

dww: Isn't http://drupal.org/files/issues/936490-38.update_verify_update_archive.in... essentially the same as the "Views is incompatible with Drupal 7.x" message we wanted to avoid though?

We need to say something like "contains versions of the following modules or themes which are incompatible with Drupal 7.x"

Would it be too much info to put the version number next to the module name? I think that would be much more apparent where the incompatibility is.

Re: #42: Oh yeah, good point. ;) I think I meant for that text to read: "archive_filename contains a version of %name which is incompatible with Drupal !version"

Re: #43: Yeah, I think duplicating the version itself after every module would be overwhelming.

I would agree with that text, dww. Sorry for not catching that sooner...

Could we go the other route and and simplify the message by not even worrying about listing the individual modules and say:
"devel-6.x-1.22.tar.gz contains versions which are incompatible with Drupal 7.x"

Gotta read whole post. Sorry. + for 45

Not much change here just added version as per 44.

Thanks for the re-roll, haydeniv, looks good to me. But this is still overwhelmingly my patch, so I can't really RTBC it. Plus, this probably should get a UX review, so I'm tagging for Usability.

Edit: Wrong issue, sorry for the spam.

Something about the use of 'which' bothered me, had to look it up, but seems like 'that' should be used here:

That usually introduces essential information in what is called a "restrictive clause." Which introduces extra information in a "nonrestrictive clause." – http://www.businesswritingblog.com/business_writing/2006/01/that_or_whic...

Also, for the case where multiple items are incompatible, can we drop the "the following" bit?

foo contains versions of modules or themes that are not compatible with Drupal 7.x: bar, baz, bonk

Per #50
"which" to "that", removed "the following"

Minor improvements to the code comments, otherwise, all the strings are unchanged.

Note: before this is RTBC, we really need to test the upgrade case, too...

dww,

Are you talking upgrade from a 7.1 to 7.2 module or d6 to d7 site upgrade test?

I'm talking about using the "Update" tab on the modules page (/admin/modules/update) to install missing updates of D7 contrib modules using the Update manager. For example, upgrading from http://drupal.org/project/update_test_module version 7.x-1.0 to 7.x-1.2. However, that doesn't help us test this, since we know the newer releases of update_test_module (for now) are all valid.

Really, to test the various upgrade cases, I think I'm going to have to make about 4 new branches and 8 new releases of that module. :( I'll leave the 7.x-1* and 7.x-2.* series alone.

I'll make a new DRUPAL-7--3 branch with 7.x-3.0 release that's sane, then a 7.x-3.1 that includes a single .module file without a .info.

I'll make a new DRUPAL-7--4 branch with 7.x-4.0 release that's sane, then a 7.x-4.1 that includes multiple .module files without corresponding .infos.

I'll make a new DRUPAL-7--5 branch with 7.x-5.0 release that's sane, then a 7.x-5.1 that includes a single .info file that says it's only compatible with 6.x.

I'll make a new DRUPAL-7--6 branch with 7.x-6.0 release that's sane, then a 7.x-6.1 that includes multiple .info files that say they're only compatible with 6.x.

and actually, to make sure multiple errors show up properly...

I'll make a new DRUPAL-7--7 branch with 7.x-7.0 release that's sane, then a 7.x-7.1 that includes multiple .info files that say they're only compatible with 6.x and some extra .module files that don't have .info files.

Fun, huh? But, once all this exists, folks should be able to install any of the 7.x-[34567].0 releases, visit the update tab, select they want to update that module, and run into all the various possible errors trying to upgrade to the *.1 release from the branch they're currently on.

In the current state should the "Update" tab show 7.x-2.1 as an available update if I have 7.x-1.3 installed and enabled? Because right now it does not show that as an available update.

If you want I can take a stab tonight at creating the new branches and releases for update_test_module. That way you can focus on real code instead of making dummy modules. ;)

Ok per 54, test modules to test each of the use cases.
dww can you give these a once over and put them up on http://drupal.org/project/update_test_module
Actual tests to come soon with screenshots of course. ;)

As promised screenshots of tests. Now we just need the tarballs on d.o so we can test the "Update" feature.

One thing that I realized while testing these (which probably needs its own issue) there is no way to update a module that isn't on d.o so if I have a custom module I have to manually (old way) install it.

If you try to install the updated module using Install New Module it says that the module is already installed.

Okay, all the releases I mentioned in #54 now exist:

http://drupal.org/node/520938/release

(Unfortunately, the tarballs from haydeniv didn't end up being useful, but I definitely appreciate the effort!)

So, if anyone else wants to test this, here's how:

1. Start with a clean install of D7 core and make sure FTP or SSH access works to your server.
2. Apply patch #52 from this issue.
3. Visit /admin/modules/install and attempt to install http://ftp.drupal.org/files/projects/update_test_module-7.x-3.1.tar.gz which should fail with an error message that looks something like http://drupal.org/files/issues/Screenshot-3.1.png
4. Install http://ftp.drupal.org/files/projects/update_test_module-7.x-3.0.tar.gz which should work fine
5. Check for available updates (/admin/reports/updates/check?destination=admin/modules/update)
6. Visit the "Update" tab (/admin/modules/update), it should say 7.x-3.1 is available for update test module
7. Check the checkbox next to that project, and click the giant "Download these updates" button
8. It should fail with an error message like the one I'm attaching here.

Lather, rinse, and repeat for 7.x-[4567].0 and .1...

So far in my testing, #52 is passing with flying colors for the update cases. I've done branches 3 and 4. See attached.

If someone could test branches 5, 6 and 7, that'd be fantastic.

Thanks,
-Derek

Per my talk with dww in irc 5, 6, and 7 did not error out like we had hoped it would. D.O packaging script automatically adds the version information to the info files so the cases we wanted to test for are very unlikely to ever occur anyway. Per my custom tarballs above we see that the appropriate messages are displayed for the 5, 6, and 7 scenarios if someone tries to install the wrong version.
I don't feel comfortable RTBCing this as I'm not that familiar with core so if someone wants to give the patch a once over I think this is good to go.

The error messages seem fine, they are specific and can be scanned quickly.

This patch needs an api.php hunk to document the return value change of hook_verify_update_archive(). Other stuff I noticed while reviewing:

+++ modules/update/update.module	2 Dec 2010 20:34:22 -0000
@@ -641,6 +641,81 @@ function theme_update_last_check($variab
+ * @see _system_rebuild_modules()

This should be @see _system_rebuild_module_data().

+function update_verify_update_archive($project, $archive_file, $directory) {
...
+  if (!empty($missing_info)) {
+    $failures['no info file'] = format_plural(

The key of $failures doesn't really matter -- they are all just messages to set. So we should have hook_verify_update_archve() just return a flat numeric array of messages instead.

+  // Make sure this isn't a tarball of Drupal core.
+  if (
+    file_exists("$directory/$project/index.php")
+    && file_exists("$directory/$project/update.php")
+    && file_exists("$directory/$project/includes/bootstrap.inc")
+    && file_exists("$directory/$project/modules/node/node.module")
+    && file_exists("$directory/$project/modules/system/system.module")
+  ) {
+    return array(

What a funky way to check for Drupal core... but I can't think of anything more elegant.

+  // Look for any .module file that doesn't have a corresponding .info file.

I can't think of a reason this check is necessary... but it can't hurt either.

The patch overall looks pretty sane and close to RTBC. Honestly the first thing that comes to mind when "Update module should verify downloaded tarballs" is md5 checking, but we should attack that in #974676: Verify MD5 hashes when downloading release tarballs for the Update manager rather than here.

Thanks, carlos8f! Agreed on all counts (especially since I suggested a few of them). ;)

This should fix everything -- new patch and interdiff. No error messages were harmed in the writing of this patch -- all the previous UI reviews are still valid.

Nicely done. I think this patch is ready.

Just for good measure I just tested the updated patch on Ubuntu 10.10 PHP 5.3.3 and on my CPanel WHM V 11.28.52 with PHP 5.2.14. Works as expected.

I ran through the very nice test plan in #58, as well as Fusion 6.x (original bug report) and Drupal core (child issue that was marked as duplicate) on DreamHost (which actually works! wow!). All seemed to work as expected.

Code looks good, apart from that ooky workaround for form_set_error() that we can't do anything about. I also really wish there were .test hunks for this, but I can't envision a way to do that without d.o getting slammed 3,000 times a second, and conferred with carlos8f in IRC and he couldn't either.

While this does technically break string freeze, I can't really envision another way to fix this problem without doing so.

So, committed to HEAD. Thanks! Especially to haydeniv for going above and beyond with testing in this, his first core issue. :)

So this is marked as an API change. Could somebody make a summary of the API change please? Does it break backward compatibility?

@rfay: the API change only respects a new hook in D7, hook_verify_update_archive(). It now returns an array of error messages, whereas it used to return a non-null value in the case of an error. This is not really worth reporting since to dww's knowledge this hook is not implemented anywhere but update.module :)

This is another example of "If anyone was actually trying to use this hook provided by the update.module, we would have had bug reports about it not working". No such issues exist, so it's safe to assume no one is using it, and therefore that we don't need to report the change anywhere.

Thanks for keeping such a close eye on the "API change" issue queue, rfay!

Cheers,
-Derek

+ * Then, we make sure that every module included in the archive has an info
+ * file.
+ *
+ * Finally, we check that all the .info files claim the code is compatible
+ * with the current version of Drupal core.

This seems way too restrictive. If a project has 10 modules and 9 are fine but 1 is bad, why shouldn't I be allowed to download it onto my site via the Update Manager and use it?

It seems like we should only prevent this if the project contains no valid modules, right?

Here is a followup issue containing a report of a real-life case where a totally "innocent" project can't be installed via the Update Manager due to this restriction: #997802: Update manager doesn't allow you to install a project if it finds a single "broken" module in it

We don't need two issues for this, shuklasp. I already replied at #997802: Update manager doesn't allow you to install a project if it finds a single "broken" module in it. I'd rather just work there, but if we must, we can mark that duplicate and reopen this one. Having both active is a recipe for disaster with the conversation forked and people either repeating themselves or missing important points, etc.

Opened this bug report http://drupal.org/node/1007542 for the Entity module, as the following was reported during installation of the Entity module:

# Notice: Undefined index: name in update_verify_update_archive() (regel 706 van /modules/update/update.module). (11 times)
# entity-7.x-1.0-beta5.tar.gz contains versions of modules or themes that are not compatible with Drupal 7.x: , , , , , , , , , ,

Where is the problem in the Entity module, or in the update module? Should the bug report http://drupal.org/node/1007542 be transferred to the update module?

Replied there. It's an entity bug, not update manger (although arguably update manager could try more carefully to defend itself from broken .info files instead of throwing PHP warnings and printing confusing messages).

although arguably update manager could try more carefully to defend itself from broken .info files instead of throwing PHP warnings and printing confusing messages

@dww: what do you suggest? Open a new bug report to get the improved error messages from the update manager, or should this bug report be used? The error report would be much more useful if it would state which .info is the culprit e.g.

And why did the update manager report 11 times the message "Notice: Undefined index: name in update_verify_update_archive() (regel 706 van /modules/update/update.module)." as the name is only missing once?

The undefined index notice is coming from these lines:

    // Get the .info file for the module or theme this file belongs to.
    $info = drupal_parse_info_file($file->uri);

    // If the module or theme is incompatible with Drupal core, set an error.
    if (empty($info['core']) || $info['core'] != DRUPAL_CORE_COMPATIBILITY) {
      $incompatible[] = $info['name'];
    }

The $incompatible array is getting $info['name'] added to it, but in this case, a file with the extension .info was able to be parsed by drupal_parse_info_file() but didn't have the 'name' attribute in it. This is clearly an edge case, so I'm not sure how much we need to cater to it.

To really handle it well, I think, would require parsing the .info file earlier in the function, so that it would have something like:

    if (!file_exists($info_file) || _info_file_invalid($info_file)) {
      $missing_info[] = $file->filename;
    }

where _info_file_invalid() would be a helper function that would a) check if an .info file was parsable by drupal_parse_info_file, and b) if it had the 'core' and 'name' attributes.

Oh crap, because it is a bug in update manager. The .info file regexp is too permissive. So it's also finding all of these:

./entity.info.inc
./entity_metadata/entity_metadata.info.inc
./modules/book.info.inc
./modules/comment.info.inc
./modules/field.info.inc
./modules/node.info.inc
./modules/poll.info.inc
./modules/statistics.info.inc
./modules/system.info.inc
./modules/taxonomy.info.inc
./modules/user.info.inc

:(

One sec, I'll post a patch that should fix it.

Untested, but I think this is what we need. Also, even though it *looks* like I'm breaking the string freeze here with a "new" t('Unknown') in the patch, I choose that since update.module already contains exactly that string. So, no translations will be harmed in the committing of this patch, but we'll avoid the weird error message (and PHP warnings) if we hit a .info file that doesn't define name.

Tested by trying to install http://ftp.drupal.org/files/projects/entity-7.x-1.0-beta5.tar.gz via the update manager. Fails without the patch in weird ways. Works fine with the patch. Someone else should probably test to confirm. Due to how the packaging script works, I don't know how to get a tarball we can test the missing name attribute with, since it's inside the .info compatibility tests, but the packaging script *always* adds the core attribute based on the version of the release node itself. :/ The more I ponder that, the more I think that's a bad thing for the packaging script to be doing. I just have no idea how many modules are relying on the current behavior and not defining core themselves...

I left the additional info file there to ease upgrading from the previous split module directories. That way an updated with the old-removed files being left works, as the .info file gets overwritten with the new empty one. But as seems to create issues for the update-manager I'll remove it now.

I gave it a test and with the patch installing worked fine. However, trying it again doesn't properly detect it is already installed, as it gets confused with second info file:

    * Notice: Undefined index: name in Updater::getProjectTitle() (line 185 of /var/www/fago/web/drupal-7/includes/updater.inc).
    * Notice: Undefined index: name in Updater::getProjectTitle() (line 185 of /var/www/fago/web/drupal-7/includes/updater.inc).
    * Unable to determine entity name.

I've updated the patch, such that getProjectTitle() at least throws an exception for not being able to parse the info file.

In the meantime I've removed the empty info file from the entity API. Still installing it only works with the patch from #79 - and I guess this is the same for any other module containing *.info.inc files.

Peoples repeatedly run into this issue, see #1009616: 7.x-1.0-beta5 doesn't install with the update-manager - thus I think we should get this fixed rather soon.

I can confirm that the error also occurs when trying to install Drupal Commerce -dev. Recommend bumping this to critical, although doing so massacres kittens.

I'm not so sure about #79. Then if you have an empty .info file for any reason, you end up triggering #1012822: If anything throws an Exception while downloading/verifying a tarball, you're in a dead end which sucks. I'd rather just go with #77 at this point. Wish someone would just RTBC that. ;)

Bah. I see what we're aiming for with 79, and yeah, we need something there. However, I found some other bugs. Stay tuned for a new patch.

New patch(es) for consideration.

There's another place where we're using file_scan_directory() with an unsafe regexp that's going to find .info.inc files, too. That's inside Updater::findInfoFile(). So, both of these patches fix that.

The lack of useful info in this exception text is driving me crazy. I'm not saying patch A is perfect, but at least it tells you where the info file it failed to parse is. It also splits out "couldn't parse" from "doesn't define 'name'". Probably this won't fly, but I'm hoping it will. ;) These are obscure errors anyway.

Patch B is basically exactly like 79 except for the file_scan_directory() bug I mention above.

Tested A.
Works great I now get errors like http://drupal.org/node/1012822 (second post).
I'll test B to if needed.

Sorry, B was done in haste and I hadn't tried it. This should work.

Tested B also working well but with a less descriptive message.

Help me out, here? What's the string difference between A and B?

Ok for webchick:

With A I see: "The info file (tempory://update-extraction/entity/entity/entity.info) does not define a 'name' attribute.
With B I see: "Unable to parse info file."

Note: if you actually had a .info file with a syntax error, you'd see this:

"Unable to parse info file: tempory://update-extraction/fubar/fubar.info"

See also #1012914: Updater classes use file_scan_directory() with a faulty regexp

While this breaks string freeze, which is verboten at this point, I think the trade-off between cranky translators and cranky end users who have no hope or prayer of recovering from this situation favours the cranky end users in this equation.

So, Committed #84A to HEAD. Thanks!