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

marvil07’s picture

marvil07’s picture

Assigned: Unassigned » sdboyer
Priority: Normal » Major

After #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?

sdboyer’s picture

yeah, i'm ok with that.

marvil07’s picture

Assigned: sdboyer » Unassigned
marvil07’s picture

Assigned: Unassigned » marvil07
Status: Active » Needs review
StatusFileSize
new3.77 KB

Here 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.

marvil07’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev

@bot: on D7 :-)

marvil07’s picture

marvil07’s picture

Version: 7.x-1.x-dev » 6.x-2.x-dev
Status: Needs review » Patch (to be ported)

Last patch added to 7.x-1.x, pending on 6.x-2.x until the end of test running locally.

marvil07’s picture

Assigned: marvil07 » Unassigned
Status: Patch (to be ported) » Fixed

Ok, 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.

senpai’s picture

Issue tags: +git, +drupal.org D7

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

sdboyer’s picture

Status: Closed (fixed) » Needs work

we really need to retain the global $conf variable-level control. it should be on a per-backend + per-plugin basis.

marvil07’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new11.55 KB

Here 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?

marvil07’s picture

Issue tags: +Needs backport to D6

tag to avoid forgetting

sdboyer’s picture

Issue tags: -Needs backport to D6
+++ b/includes/VersioncontrolRepository.php
@@ -853,6 +853,12 @@ abstract class VersioncontrolRepository implements VersioncontrolEntityInterface
+    $variable_name = "versioncontrol_repository_plugin_defaults_{$this->getBackend->type}_{$plugin_slot}";

getBackend() 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?

marvil07’s picture

Thanks 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.

marvil07’s picture

Status: Needs review » Fixed
Issue tags: -Needs backport to D6

Follow up for tests on #1796224: Write a test for plugin selection.

Last patch added to 7.x-1.x and 6.x-2.x

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit 54f92c7 on 7.x-1.x, repository-families, drush-vc-sync-unlock by marvil07:
    Issue #1131084: Make a little more verbose the exception for plugin...
  • Commit 2fe8c45 on 7.x-1.x, repository-families, drush-vc-sync-unlock by marvil07:
    Issue #1131084: Fix logic about plugins default loading.
    
    
  • Commit 50093f7 on 7.x-1.x, repository-families, drush-vc-sync-unlock by marvil07:
    Issue #1131084: Stop using $conf variables to define global default...
  • Commit cb44eb9 on 7.x-1.x, drush-vc-sync-unlock by marvil07:
    Issue #1131084: Let use $conf to set per-backend/per-plugin_slot plugin...

  • Commit 54f92c7 on 7.x-1.x, repository-families by marvil07:
    Issue #1131084: Make a little more verbose the exception for plugin...
  • Commit 2fe8c45 on 7.x-1.x, repository-families by marvil07:
    Issue #1131084: Fix logic about plugins default loading.
    
    
  • Commit 50093f7 on 7.x-1.x, repository-families by marvil07:
    Issue #1131084: Stop using $conf variables to define global default...
  • Commit cb44eb9 on 7.x-1.x by marvil07:
    Issue #1131084: Let use $conf to set per-backend/per-plugin_slot plugin...