notice: Undefined offset: 2 in .../cvs_deploy.module on line 153.
dman - October 17, 2009 - 03:57
| Project: | CVS deploy |
| Version: | 6.x-1.1 |
| Component: | Code |
| Category: | task |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | E_STRICT |
Description
Just a paranoid PHP E_ALL notice, but I got this a few times now I've been referred to use this module instead of the drush built-in that I was enjoying.
Turns out the issue is my third-party repository. I've got some custom modules of my own in a different CVS repo. They are neither under the 'drupal' namespace or the assumed 'contributions' folder structure.
Code here assumes it's either one or the other.
<?php
if (file_exists($cvs_dir .'/Repository')) {
$repo_file = trim(file_get_contents($cvs_dir .'/Repository'));
if ($repo_file) {
$parts = explode('/', $repo_file);
if ($parts[0] == 'drupal') {
$projects[$name] = $parts[0];
}
else {
$projects[$name] = $parts[2];
}
}
}
?>I found that this seems to do the job fine instead
<?php
else {
$projects[$name] = array_pop($parts);
}
?>It could be that cvs_depoly does not even want to support non-drupal.org repositories at all, so mark it 'by-design' if you want, but is seemed that this tweak at least makes it php-strict compliant anyway. Developers likely to be using cvs_deploy are also likely to be working on dev versions, so we've got notices turned on by default.
#1
title typo
#2
Hrm, well, cvs_deploy is mostly for helping update status/manager know about your code so it can still tell you if your CVS checkout needs a "cvs up"... If you're doing your own code in your own repo, I somehow doubt you're setting up releases and release history XML files and all the rest of it, so update status isn't going to be able to tell you anything useful, anyway... So, I'm not sure what good it does to attempt to support non-cvs.d.o repositories in any meaningful way.
That said, it'd be nicer if we didn't barf with a PHP notice. ;) So yeah, I'd be open to a patch that tightens this up a bit to more gracefully recover from this case.
However, your patch isn't the right solution. That only works for modules that only have .info files in their top-level directory. However, look at a release from, for example, http://drupal.org/project/date. There are modules with their own .info files in lots of subdirectories in there:
date/date/date.infodate/date_api.info
date/date_locale/date_locale.info
date/date_php4/date_php4.info
date/date_popup/date_popup.info
date/date_repeat/date_repeat.info
date/date_timezone/date_timezone.info
date/date_tools/date_tools.info
All of those should be in the "date" project, but your code is only going to think date_api (and by accident, date) belong to the "date" project, while all the others will belong to non-existent projects like "date_tools", etc...
See what I mean?
#3
Yeah, I know it's not going to be expected to support or even be aware of external repositories - I just want it to not complain when they are there. Eliminate the red notices.
Happily, I didn't see any "problem" with d.o sandbox checkouts either - but it looks like that would be assuming all my sandbox stuff is in a project called {username}. That's probably not intentional.
I considered the nested info file issue, so looked at the results from image package that does this - just in the modules page UI - which was where I was seeing the problem. It seemed to give accurate results (or at least identical results) on that, so I guessed it hadn't broken that. But as you explain, it's probably not quite right with regard to the containing package.
Should we instead be a little more careful and check for contributions/[modules|themes] there, and ignore the rest?
<?php$repo_file = trim(file_get_contents($cvs_dir .'/Repository'));
if ($repo_file) {
$parts = explode('/', $repo_file);
if ($parts[0] == 'drupal') {
$projects[$name] = $parts[0];
}
elseif ($parts[0] == 'contributions' && ($parts[1] == 'modules' || $parts[1] == 'themes')) {
$projects[$name] = $parts[2];
}
else {
// else an unknown (non d.o) repository, ignore
}
}
?>
edit - removed debug left in :-)
#4
Yeah, that's a little more like it. Not sure what the else case should be, but it probably doesn't matter. Fall back to pop()?
Anyway, if you want to submit that as a patch (ideally for HEAD, DRUPAL-6--1, and DRUPAL-5), I'd be happy to test and commit it.
Thanks!
-Derek
#5
Righty.
Here's D5 and D6. patches then.
Falling back to array_pop as suggested, although I don't know how much it will help. Leaves the extreme edge case of a custom repo including nested modules in a package slightly undefined, but seeing as that info isn't actually getting used - can't hurt I guess.
Slightly amended comment.
D5 had a different variable name in the code, but I caught that.
I imagine that the D6 will apply to HEAD, possibly with fuzz. It's an easy paste. Not 100% clear on the difference between D7 and HEAD this week.
#6
status