mikey_p and sdboyer agree that writing a hook_update_N() to change plugins is a bad idea. So providing a variable per plugin can make this things easier as mikey_p suggested.

Comments

marvil07’s picture

Assigned: Unassigned » marvil07
Status: Active » Needs review
StatusFileSize
new8.11 KB

Ok, here a patch that:

  • Add global variables for each default repository plugin.
  • Stop hardcoding default repository plugins.
  • Remove "Use Author mapping plugin", to avoid handling a special form submit case at the repository getCommitterMapper().
  • Add a 'none' repository mapper plugin that do not map.
marvil07’s picture

Issue tags: +git sprint 9

Seems like versioncontrol_project can use this :-)

marvil07’s picture

Status: Needs review » Needs work

Adding an UI per backend per plugin, and hopefully a way to define default settings inside the plugin.

marvil07’s picture

Status: Needs work » Needs review
StatusFileSize
new16.11 KB

Ok, finally the new patch!

what's new:

  • Add UI for default plugin settings.
  • Provide a way to set a default base URL for webviewer plugin.
  • minor: fix default plugin values for repo edition UI
sdboyer’s picture

Status: Needs review » Needs work

Overall, I think this is great. We really do need this; even if it's mostly in development environments (as opposed to real prod sites) where we get into funny inconsistent states where plugins aren't set that should be, plugins are now sufficiently essential to the basic functionality of vcapi that it's really not prudent to allow hard failures to occur when a plugin can't be found.

Good call on the 'None' mapper, that's _clearly_ the sensible default.

I'm a bit hesitant about the approach of having these global defaults for the base URLs of the webviewers. It feels like not-the-best architecture (really something that I'd rather encapsulate on the plugin), but I don't have any better ideas in mind and it clearly works, so it's not a blocking concern.

db_query('UPDATE {versioncontrol_repositories} SET data = %s WHERE repo_id = %d', $row->data, $row->repo_id);

Gotta keep an eye on this, you're not quoting the string placeholder in both 6312 and 6314. I missed this when reviewing the patch containing 6311 and committed it unintentionally, and had to fix it - unquoted serialized strings always ruin queries.

Only other note is that I think it might be worth it to create an includes/plugins.inc file, and move some of our plugin code into there. If we add a ctools_include('plugins') at the top of such a file, then that could simply it, too - just switch most existing calls to ctools_include('plugins') over to ctools_include('plugins', 'versioncontrol').

sdboyer’s picture

Status: Needs work » Fixed

I skipped creating the separate plugin file (no time for that noise right now), but fixed the db problems (one was actually already committed), made some other minor tweaks, and committed. Sorry marvil07, we just really need this one in and can't wait.

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 9

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

  • Commit ce32d16 on repository-families, drush-vc-sync-unlock by sdboyer:
    Issue #1027356 by marvil07: Provide a way to set default plugins for...

  • Commit ce32d16 on repository-families by sdboyer:
    Issue #1027356 by marvil07: Provide a way to set default plugins for...