Updater classes use file_scan_directory() with a faulty regexp [#1012914]

While grepping the source for other occurrences of the bug from #936490: Update module should verify downloaded tarballs and propagate errors correctly I found some hits in modules/system/system.updater.inc. We're trying to figure out if the tarball we extracted is a module or a theme or what. We call file_scan_directory($directory, '/.*\.module/'). So, hypothetically, if we had a theme with a foo.module.tpl.php file or something, we'd think it's a module, and then we'd try to use the ModuleUpdater class on it instead of the ThemeUpdater class and Something Bad(tm) would probably happen. I have no energy or time to figure out exactly what that might be or to try to reproduce it, but I can guarantee that fixing this will only make things more stable, not less. ;)

Comment	File	Size	Author
#4	zen.zip	206.53 KB	aspilicious
#1	1012914-1.system-updater-file-scan-regexp.patch	1.02 KB	dww
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

dww

we/he/they

CreditAttribution: dww commented 2 January 2011 at 22:10

Status:	Active	» Needs review
Issue tags:		+Update manager

File	Size
1012914-1.system-updater-file-scan-regexp.patch	1.02 KB

Here's the patch. I tested this and I can still properly install modules and themes. Also forgot to tag this for Update manager...

Comment #2

Tor Arne Thune CreditAttribution: Tor Arne Thune commented 3 January 2011 at 08:26

I tried to install a module (zip version of wysiwyg) and a theme (zip version of zen) with this patch applied to a clean install of HEAD. It worked like before. Don't know how else I can test this.

Comment #3

dww

we/he/they

CreditAttribution: dww commented 3 January 2011 at 12:36

Status:

Needs review

» Reviewed & tested by the community

Right, the only way to see the bug would be to find a D7 theme that contains a file called *.module* and try to install it. Without this patch, it'll fail in weird ways. With this patch, it'll work. We *could* make a dummy theme project (update_test_theme) sort of like http://drupal.org/project/update_test_module but I don't know if it's worth it. From visual inspection, it's obvious this patch is a fix, and two of us have now confirmed update manager works as expected with the fix. Hence, RTBC.

Comment #4

aspilicious CreditAttribution: aspilicious commented 3 January 2011 at 13:30

Status:

Reviewed & tested by the community

» Needs work

File	Size
zen.zip	206.53 KB

Srry to put this back to needs work.

1) I just download a dev zip of zen.
2) I repacked it with a zen.module file.

Without patch:
I could install it as a module

With patch:
It still sthinks its a module (module alrdy installed)

3) With the patch I removed the .module file again
4) Try to install theme again
==> works, as it now knows it's a theme

Edited zen folder file attached :)

Conclusion:
----------
This doesn't brake anything, but does it fix something?

Comment #5

dww

we/he/they

CreditAttribution: dww commented 3 January 2011 at 19:03

Status:

Needs work

» Reviewed & tested by the community

If a theme actually contains a .module file, that's bogus and invalid and will confuse the update manager.

What I'm talking about here is a valid theme-related file that just so happens to have *.module* in the filename, something like node.module.tpl.php. It's sort of a stretch, but it could happen. ;)

I just ran a find command on the entire themes CVS repository, and didn't find anything that matches this. ;) There are a few themes that include .module files (naughty, naughty), but nothing like what I'm talking about here. So, at least in terms of existing contributed themes, there's nothing to trigger this bug.

However, I just think it's wrong to leave this broken. Other bugs came from not using file_scan_directory() correctly, so I'd like to avoid repeating the broken pattern lest anyone copy it and re-use it in a place that really matters.