Currently, VersioncontrolRepository::getPlugin has the following logic:
if (empty($this->plugins[$plugin_slot])) {
// handle special case for two slots using the same plugin type
if ($plugin_slot == 'committer_mapper' || $plugin_slot == 'author_mapper') {
$variable = 'versioncontrol_repository_plugin_default_user_mapping_methods';
}
else {
$variable = 'versioncontrol_repository_plugin_default_' . $plugin_slot;
}
$plugin_name = variable_get($variable, '');
}
else {
$plugin_name = $this->plugins[$plugin_slot];
}
There are two fundamental problems with this logic:
- It does not function properly in multi-backend environments; a sane default plugin that is git-specific will not be a sane default for svn.
- Other such settings tend to be handled primarily by registering them on the
VersioncontrolBackend, then letting the backend object handle it. Makes for less code duplication. e.g. VersioncontrolBackend::verifyPluginInterface(). Moving it into a $conf var breaks the pattern.
Those are the really blocking problems; other less critical issues include the one-off code for the commit/author mapping using the same default, and...some other stuff I can't think of offhand.
In the long run, we'll probably want a cascading series of defaults so that the owner of the repo (e.g., versioncontrol_project) can specify defaults that don't interfere with other repo owners, but that's a >2.0 feature.
Comments
Comment #1
marvil07 commentedComment #2
marvil07 commentedTwo related commits:
Comment #3
marvil07 commentedAfter #1548006: Let backends define default plugins backend defaults are possible.
So, I would say: just remove $conf assignments. @sdboyer: are you OK with it?
AFAIK we do not really use that $conf variables on d.o, right?
Comment #4
sdboyer commentedyeah, i'm ok with that.
Comment #5
marvil07 commentedComment #6
marvil07 commentedHere the patch that remove the use of $conf to define defaults. It also re-arranges the code for easier reading.
Let's see what testbot thinks.
Comment #7
marvil07 commented@bot: on D7 :-)
Comment #8
marvil07 commentedComment #9
marvil07 commentedLast patch added to 7.x-1.x, pending on 6.x-2.x until the end of test running locally.
Comment #11
marvil07 commentedOk, added to D6 upstream. I also added a change record: http://drupal.org/node/1759486 to notify the removal.
There is still some more improvements suggested on the original issue, but I guess it's enough to close this. Please re-open if needed.
Comment #12
senpai commentedComment #14
sdboyer commentedwe really need to retain the global
$confvariable-level control. it should be on a per-backend + per-plugin basis.Comment #15
marvil07 commentedHere a patch for D7.
I completely forgot to change UI form when I removed the $conf logic on plugin retrieval. This patch re-adds the use of $conf per backend + per plugin slot(I think plugin slot makes more sense than plugin type).
@sdboyer: would you mind to take a look at this before I push it and backport it to D6?
Comment #16
marvil07 commentedtag to avoid forgetting
Comment #17
sdboyer commentedgetBackend() is a method, not a property.
other than that, quick perusal makes it seem like this is ok. what tests do we have that exercise this behavior?
Comment #18
marvil07 commentedThanks for the quick review, the bug is fixed.
I think we do not have tests about default plugin selection, but I think it's OK.
Comment #19
marvil07 commentedFollow up for tests on #1796224: Write a test for plugin selection.
Last patch added to 7.x-1.x and 6.x-2.x