As part of #965890: [meta] Revisit VersioncontrolAccount and bring it up to speed, and in much the same spirit as #223891: API change: make authentication management pluggable, the process by which raw VCS user data is mapped to a Drupal user needs to be made completely pluggable. There's two layers to this pluggability:

  1. Since the raw data is different from one VCS to the next, vcapi itself can at most define some shell logic (abstract methods, whatever), that the backends then fill out.
  2. The backends must, in implementing that vcapi-defined logic, be pluggable in that they should be able to send their raw data to some plugin that then responds with the uid/a user object/a VersioncontrolAccount object (or a FALSE or something), and we can then update corresponding operations with the uid.

We could probably have some simple plugins that live in vcapi itself - e.g., mapping based on {users}.mail.

CommentFileSizeAuthor
#10 979040-10-incremental.patch20.72 KBsdboyer
#10 979040-10-mainline.patch17.74 KBsdboyer
#9 make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v4.patch12.19 KBmarvil07
#8 make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v3.patch12.6 KBmarvil07
#6 make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v2.patch11.99 KBmarvil07
#5 make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v1.patch9.46 KBmarvil07
#3 make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v0.patch7.06 KBmarvil07

Comments

sdboyer’s picture

sdboyer commented
Issue tags: +git phase 2, +git sprint 5

tagging

marvil07’s picture

marvil07 commented
Issue tags: +versioncontrol account refactor
marvil07’s picture

marvil07 commented
Status: Active » Needs review
StatusFileSize
new make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v0.patch7.06 KB

Here the first patch version mainly using all sdboyer ideas :-)

  • Schema change to add plugins field to repo table.
  • Base for user_mapping_methods as a ctools plugin: base ctools implementation and form addition.
  • Add a "plain mail" user_mapping_method that uses {users}.mail to map users

What needs to be done now: Actually call the plugin callbacks('map author' and 'map committer'). I did not added them because the mapping now is done while account class is executing its CRUD methods and IMHO this should be easier if we first try to get rid of account class. I'm not really sure where should be the right place to call the mapping callbacks.

So I will be opening another issue about that removal.

Another TODO here is fix the hook_update_N() I implemented to add default values at the update fro plugins field.

marvil07’s picture

marvil07 commented

The related issue I was going to open is on #983926: Remove account class

marvil07’s picture

marvil07 commented
StatusFileSize
new make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v1.patch9.46 KB

Minor updates to fix tests after applying the patch.

marvil07’s picture

marvil07 commented
StatusFileSize
new make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v2.patch11.99 KB
  • minor: Mention current possible plugin keys.
  • Add 'map_user' option to VersioncontrolOperation used on insert/update to call determineUsers(), which do the mapping.
sdboyer’s picture

sdboyer commented
Status: Needs review » Needs work

Looks like this is coming along nicely. Very much like the code I'd been working on locally :) Some comments:

First big note is that there's no reason to use crappy procedural ctools plugins - oo plugins ftw :) They're a bit more of a pain to set up, but point you to examples, or just do it myself.

Second big thing is that we need to assume multiple plugins will be defined to attempt mapping. It's a good idea in general, but if #898816: Consider using real names and/or e-mail addresses for the author/committer metadata? goes the way I think it should, also completely necessary for d.o. That's gonna be fairly complicated, of course - we have to start worrying about plugin weighting and a UI to control the ordering - but still needs doing.

Only other thing that immediately jumped out at me was this:

$plugin = versioncontrol_get_user_mapping_method($this->repository->plugins['user_mapping_method']);

From the first line of public function determineUsers() - I'd rather VersioncontrolRepository provide a method for doing this, pulling the plugin out manually is a little awkward. Additionally, we can reduce the number of calls into ctools by caching the plugin locally on the repository object and masking it all behind a single method. Much cleaner.

marvil07’s picture

marvil07 commented
StatusFileSize
new make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v3.patch12.6 KB
  • minor: Cache for instantiated repository plugins.

I'm going to try to make the plugin a oop plugin now.

marvil07’s picture

marvil07 commented
Status: Needs work » Needs review
StatusFileSize
new make-pluggable-the-process-of-mapping-of-raw-vcs-data-to-Drupal-users-v4.patch12.19 KB

Ok, here an attempt to convert it to oop ctool plugin :-)

sdboyer’s picture

sdboyer commented
Status: Needs review » Active
StatusFileSize
new 979040-10-mainline.patch17.74 KB
new 979040-10-incremental.patch20.72 KB

Fair bit of refactoring & tweaking. Here's the list:

commit a8c4ea83ebc4911a33d78ba9e96dda17a5b22de7
 Author: Sam Boyer <drupal@samboyer.org>
 Date:   Thu Dec 2 15:32:20 2010 -0800
 
     Refactor the plugin loading, and concomitant UI changes.
     
     Plugin caching is now more efficient (caches instantiated plugin object
     instead of plugin definition), actually works for multiple plugin types,
     and has good centralized error handling.
     
     There can now be a separate mapping plugin for authors and committers,
     with the possibility of tying committer plugin to author.
     
     Updated the repository editing UI to reflect these changes.
 
 commit 1eb2fa78caf14203987a800a052575f89106a703
 Author: Sam Boyer <drupal@samboyer.org>
 Date:   Thu Dec 2 15:35:09 2010 -0800
 
     Tweaks to VersioncontrolOperation.
     
     Change the key slightly to be more human readable (no need for an
     underscore here), and reintroduce a mapUsers() method that replaces
     determineUsers() and just triggers both of the other mapping methods.
 
 commit 97b692987451f1683bdd706021cfbab7b146c0d4
 Author: Sam Boyer <drupal@samboyer.org>
 Date:   Thu Dec 2 15:37:07 2010 -0800
 
     Removed unnecessary plugin getters.
 
 commit 1177308f7173c1d15e1a1315998076bca1a038f3
 Author: Sam Boyer <drupal@samboyer.org>
 Date:   Thu Dec 2 15:41:31 2010 -0800
 
     Move & rename the simple mail mapper plugin.
 
 commit d968619635942d709b3668ee99b7918e8602d6a5
 Author: Sam Boyer <drupal@samboyer.org>
 Date:   Thu Dec 2 15:57:30 2010 -0800
 
     Make the mapping process a little more robust - ensure the uid is set to
     0 if no mapping plugin is set.
 
 commit 4b358832a38a65fe0b71f720a27bb10f7419e34b
 Author: Sam Boyer <drupal@samboyer.org>
 Date:   Thu Dec 2 15:59:41 2010 -0800
 
     Introduce VersioncontrolUserMapperInterface and create the simple_mail
     mapper plugin.

I'm ready to commit this, and probably will presently since I can't seem to find marvil07 on IRC :)

Two patches are attached, incremental diff vs. #9 and diff vs. mainline.

marvil07’s picture

marvil07 commented
Status: Active » Reviewed & tested by the community

This looks more clear ;-)

sdboyer’s picture

sdboyer commented
Status: Reviewed & tested by the community » Fixed

Aaaand, committed. woohoo!

marvil07’s picture

marvil07 commented
Status: Fixed » Reviewed & tested by the community

for the record http://drupal.org/cvs?commit=458628

marvil07’s picture

marvil07 commented
Status: Reviewed & tested by the community » Fixed

:-/

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 5, -versioncontrol account refactor

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

Issue tags: +git phase 2, +git sprint 5, +versioncontrol account refactor

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

  • Commit 9dc7542 on repository-families, drush-vc-sync-unlock by sdboyer: 
    Issue #979040 by marvil07, sdboyer: Make the mapping of raw VCS user...

  • Commit 9dc7542 on repository-families by sdboyer: 
    Issue #979040 by marvil07, sdboyer: Make the mapping of raw VCS user...