Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Honestly, there's no reason _at all_ that the versioncontrol_account_status module should be in here. AFAICT, it was originally created as part of the 1:1 feature parity attempt with vcapi vis-a-vis cvslog & company. Given the huge variety of different possible ways you could set up an account approval workflow (as demonstrated by the very discussions we're having right now for phase 2 - #781264: Decide on an appropriate git analogue to current CVS account workflow), I see no reason to package one approach with the main api module itself.
Consequently, unless someone raises a legitimate objection, I'm going to rip that whole implementation out. Someone else is free to resuscitate it from CVS history and rebuild it in another project. Or I will, if the need arises.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | 0001-Kill-the-account-status-module-completely.patch | 70.16 KB | marvil07 |
Comments
Comment #1
marvil07 CreditAttribution: marvil07 commentedsounds reasonable
hint: do not forgot removing the directory from cvs
Comment #2
marvil07 CreditAttribution: marvil07 commentedavoid forgetting we must have another solution before removing this module
Comment #3
marvil07 CreditAttribution: marvil07 commentedThis issue is naturally too related with #983926: Remove account class.
This module intersect the user in the account creation form to force an "approval process" by an administrator. It relies on the "account entity" at main vcs api, so it account gets removed from versioncontrol we should need to move some of it to this module(as I started at the other issue).
The idea behind it AFAIK is:
- Provide a drupal UI to manage accounts, and process them inside an "approval process".
One thing to notice here is that versioncontrol_account_status module maintains a list of accounts(uid+repo_id) that have access to a repository, but it is assuming that the approval of the account is done once, so if we have cases like d.o where we have one repo per project, we should need to add an approval per project, which do not really make sense.
One use case for it can be: Using git-daemon(with receive-pack aka all can write) + hooks per repo + versioncontrol_account_status auth plugin(#223891: API change: make authentication management pluggable) which verify against approved accounts we can get a simple authentication mechanism controlled at drupal.
The other natural fit for it is for any CVCS, since many of that menthods uses authentication based on passwords, so accounts are real. It only needs to synchronize passwords(which is done for example at versioncontrol_cvs/xcvs/xcvs-generate-passwd.php)
I am still not really sure if this module must be removed or not :-(
Comment #4
sdboyer CreditAttribution: sdboyer commentedOK, thinking on this even yet MORE, I'm still inclined in the direction that says we strip it out of the api. There's nothing wrong, nothing at all, with backends implementing their own account registration system. But it gets backend-specific SO quickly - the text & options for the account registration page, where to store/what to do with the account information, how to use the account information etc., that I think the difficulties of having it in the API far outweigh the benefits. Especially because the benefits really just come down to avoid a little code duplication. Big deal.
Basically, I see it being 5-10x as much work to come up with good architecture for this in the API as it is to let each backend implement it. If we just stipulate that this system needs to work on top of the new pluggable auth system, then it shouldn't be any problem to push it out there. The most we should have in the API is maybe some kind of messaging layer for doing 'account status'...but we can open an issue for that later if we want it.
Therefore, RTBC. Your patch, marvil07, so you please pull that trigger :)
Comment #5
sdboyer CreditAttribution: sdboyer commentedDammit.
Thought about this more and poked at the code, and we do need SOMETHING. So let's hold off committing the changes until we've at least opened an issue outlining what ought to be done.
Comment #6
sdboyer CreditAttribution: sdboyer commentedOK, I'm going to officially back off of this. The more I think about it, the less I feel like I'm approaching a reasonable idea. Other ideas, proposals, brainstorming, whatever, welcome. marvil07, you should also feel free to just run with this, if you have an idea.
Comment #7
marvil07 CreditAttribution: marvil07 commentedI will be working on #983926: Remove account class as the step 1 of this(see #983926-8: Remove account class for details), postponing this until that gets in.
Comment #8
sdboyer CreditAttribution: sdboyer commentedAs part of #983926: Remove account class, I removed the account status module entirely. If and when we have time to do a good job of reimplementing that mess, we can have it back in - but we can't let it hold back a stable release and/or the git migration as a whole. And that's the point we've gotten to.