Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In working on a patch for cvs_deploy, http://drupal.org/node/184664, I realized that there's fallback code inside update.module for determining the project name for a given module or theme if it's not included in the .info file. There are only 2 possible cases where this is hit:
A) You've deployed from CVS and your .info files don't define 'project'.
B) You're developing a brand new module or theme that doesn't exist on d.o nor in CVS.
In the case of (A) this fallback code is doing more harm than good, and making cvs_deploy's job much harder to do a proper solution in #184664. :( We have no way to know if there's already a legitimate 'project' attribute or if we should do the more expensive operation of opening and parsing the CVS/Repository file to get the definitive result. This fallback code prevents people from correctly using update.module if they deploy modules via CVS into subdirectories of various module folders searched by core. For example, this doesn't work: sites/all/modules/contrib/foo. #184664 will fix this, but only if we remove this fallback code.
In the case of (B), this fallback code is a futile effort, there's definitely no available update data anyway, so we should just ignore the project entirely.
Patches attached for both update.module in core and update_status.module 5.x-2.* (DRUPAL-5--2 branch).
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | update_get_project_name_no_CVS_5x_1.patch | 1.34 KB | dww |
| update_get_project_name_no_CVS_5x.patch | 1.42 KB | dww | |
| update_get_project_name_no_CVS_6x.patch | 1.33 KB | dww | |
Comments
Comment #1
dwwPatch for core is fine, but the patch for update_status in contrib is slightly off. We need to invoke hook_system_info_alter() first to give cvs_deploy a chance, then we can give up. ;)
Comment #2
webchickI'm very much +1 for this change, as I desperately want 184664 to happen. It also removes a bunch of code that looks like a bit of a sub-optimal hack anyway.
Tested with the following modules:
- Archive 6.x-0.9-dev tarball (simple, single module)
- Devel 6.x-1.0 tarball (has several additional modules in the module's root directory and, as an extra bonus, a security release in 6.x-1.0)
- Image 6.x-dev tarball (has sub-modules in its "contrib" directory)
- MyTestModule x.x, a module I just invented that has a blank .module file and .info file with only the bare necessities (name, description, core) in order to simulate the custom module/contrib still in development module.
I enabled Archive, Devel, Devel Generate, Image, Image Attach, and MyTestModule.
Before the patch, this shows (as expected):
After the patch, the results are the same /except/ mytestmodule is not in the list, at all, as described.
Now, I tried with CVS checkouts of the HEAD versions of each module, rather than tarballs:
Before patch:
With the patch, none of these are displayed.
Finally, I tested with these same checkouts, and cvs_deploy module. Without the patch:
(in other words, looks as I'd expect)
Without the patch, none of these are displayed. This: http://drupal.org/files/issues/cvs_deploy_project_from_cvs_5x.patch didn't apply against the HEAD version so I couldn't confirm whether cvs deploy is just awaiting an update to deal with this situation, or whether it's just carrying on the expected behaviour...
---
The behaviour being exhibited seems like a bug to me. While I'd fully expect MyTestModule to be dropped from the list (along with any other custom/in-development modules), it was rather shocking to see CVS checkouts removed entirely as well, as I've seen many websites with hybrid checkouts like this (sometimes by necessity, if you're dependent on a module by a contrib developers who hasn't quite started using the release system yet). I'd prefer everything be displayed with an "Unknown" next to it (file_scan_directory(.module$)?) than entries missing altogether...
If this is in fact desired behaviour/the only way it can work, then I guess we need to make it clear somewhere that cvs deploy is going to be absolutely required in the event that someone's managing their site via CVS in order to update status to do a bit of good. Currently, the only project it does any good with in all situations is Drupal core itself.
I'm marking needs work, but I might be wrong. Will wait for dww to confirm.
Comment #3
dwwA few quick answers:
- update_status can't do *anything* right with CVS checkouts unless you use cvs_deploy, since there's neither a 'project' attribute nor a 'version'. cvs_deploy has been and will always be required if you want update_status to work with a CVS checkout -- that's why i wrote cvs_deploy in the first place. ;)
- no, the HEAD version of cvs_deploy has not been patched to populate the 'project' field yet. i was awaiting decision on this patch, since i don't want to spend my time on that particular approach if for some reason this patch isn't going to land. You can test the 5.x versions of everything via http://drupal.org/node/184664#comment-696076 if you want to see where cvs_deploy is heading.
- if there's no 'project' in the .info, and no cvs_deploy to get it via other means, update.module will be horribly confused about your code, since everything is organized by 'project'. :( i'd rather it just excluded modules it can't possibly tell you anything useful about than print weird things that basically all say "i have no idea WTF this is, sorry".
does that answer your concerns? would you rather i patched cvs_deploy HEAD, too, so you can fully test this?
Comment #4
webchickPatching cvs_deploy would help, yeah. I just want to confirm that there's some way for people to see something on those cvs checkouts somehow. :)
Comment #5
dwwOk, new patches for both DRUPAL-5 and HEAD cvs_deploy are now at http://drupal.org/node/184664#comment-698781.
Comment #6
dwwnote for testing: the bug 184644 fixes isn't projects that have N modules in subdirs. To see the bug, you want to put your foo directory in sites/all/modules/contrib/foo.
Comment #7
Gábor HojtsyI agree that it is a good idea to remove that fallback code. Looks like some cross-testing wit cvs_deploy is still going on, so I am awaiting results. Sorry I don't have the time to test now.
Comment #8
dwwAnother idea occurred to me: while ripping this out, we could also rip out the fall back code that tries to group modules as parts of core. Again, if there are release tarballs, we've got "project = drupal" in the .info file from the packaging script. if not, we're in cvs_deploy land, and we can just definitively decide if it's core or not based on what repo it's checked out from (basically, we do the same thing to parse the CVS/Repository file from #184644, but we slightly alter the logic to look at the 1st and 3rd elements of the path, not just the 3rd). thoughts?
Comment #9
Gábor HojtsyWell, this issue solves possible issues with checkouts, while your new ideas would just remove some code duplication. In the interest of keeping what works for D6, not to risk more bugs and concentrate on solving problems, I'd not remove that.
Comment #10
dwwOk, fair enough. We can leave that code in, since it's not really hurting anything (other than some minor bloat).
Comment #11
webchickOk thanks a lot, dww!!
I re-ran my second two tests. with cvs deploy module and the referenced patch applied.
Now the only one that's hidden form view is mytestmodule, which is by design. The rest show up, and it's even smart enough to tell me that SimpleTest HEAD has no releases, and that Drupal 6.0-rc2 has security fixes. :)
So in other words, this patch does no harm to the vast majority of our end users who use tarballs. And it /should/ actually help those who deploy with CVS to see instantly that something is up, and to pursue a solution from there.
Marking RTBC.
Comment #12
dwwBased on webchick's RTBC, Gabor's positive comments above, and my own further testing, I committed my patch in #1 to DRUPAL-5--2 of update_status: http://drupal.org/cvs?commit=96872 @Gabor: feel free to re-use that commit message if you don't want to compose your own. ;)
Comment #13
Gábor HojtsyThanks, committed.
Comment #14
dwwYay, thanks! Just committed the fixes to cvs_deploy over at http://drupal.org/node/184664.
What a happy issue queue: http://drupal.org/project/issues/cvs_deploy ;)
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #15.0
(not verified) CreditAttribution: commentedfixing a minor (but confusing) typo