thinking on interfaces(aka define them) [#521792]

The original plan:

Following OOP aproach, the versioncontrol submodules, like commitlog, could be converted to interfaces, that the backend could implement.

The wiki plan:

Conversion to interfaces(July 17th - July 26th)
* make backend capabilities interfaces
* convert commitlog class interaction to an interface where it's applicable(commitlog have lots of code that will remain on module file)

Nor the first plan, neither the updated fit on what we have on vc api. So, I tried to list here all the points where we would want to take a look before think on interfaces.

backend capabilities array(VERSIONCONTROL_CAPABILITY_*)

// Able to retrieve a file or its revision number based on a global
// revision identifier.
VERSIONCONTROL_CAPABILITY_ATOMIC_COMMITS

on Item::fetchCommitOperations, let fetch by revision id instead of fetch by operation/item association
on commitlog theme_commitlog_item, let display the revision next to the path on backends with no atomic commits

// Able to cancel commits if the committer lacks permissions
// to commit to specific paths and/or branches.
VERSIONCONTROL_CAPABILITY_COMMIT_RESTRICTIONS

on commit_restrictions versioncontrol-repository-form alter, show allowed_paths textfield, deny_undefined_paths checkbox and forbidden_paths textfield; so then on repo hook_versioncontrol_repository_submit() values can be retrieved and set. Those values are used to implement hook_versioncontrol_write_access().

// Able to cancel branch or tag assignments if the committer lacks
// permissions to create/update/delete those.
VERSIONCONTROL_CAPABILITY_BRANCH_TAG_RESTRICTIONS

on commit_restrictions versioncontrol-repository-form alter, show valid_branch_tag_paths, valid_branches and valid_tags textfields and process them on hook_versioncontrol_repository_submit(), called also from hook_versioncontrol_write_access() like VERSIONCONTROL_CAPABILITY_COMMIT_RESTRICTIONS workflow.

// The version control system assigns revisions not only to files
// but also to directories.
VERSIONCONTROL_CAPABILITY_DIRECTORY_REVISIONS

on Item::getParentItem to either assign or not a revision to the new parent item object

backend flags array

// versioncontrol_insert_repository() will automatically insert
// array elements from $repository['[xxx]_specific'] into
// {versioncontrol_[xxx]_repositories} and versioncontrol_get_repositories()
// will automatically fetch it from there.
// (Deprecated, to be replaced by $repository['data'][$module].)
VERSIONCONTROL_FLAG_AUTOADD_REPOSITORIES,

on repository _amend_repositories(called from getter), update, insert and delete to manage the own vcs specific repository table; but this is going to disappear with the $repository['data'][$module] like it's mentioned

versioncontrol_backend_implements/_versioncontrol_call_backend

versioncontrol_backend_implements, from its definition: Determine if a given backend module implements a specific backend function.
The function looks like 'versioncontrol_'. $vcs .'_'. $function

import_accounts / export_accounts
- on hook_menu to either define or not admin/project/versioncontrol-accounts/[import|export|export/%versioncontrol_repository]
- on versioncontrol_admin_account_import_form to filter the backends that can be imported
- on versioncontrol_admin_account_export_page to verify is repository passed can export
- on versioncontrol_admin_account_export_form to filter the backends that can be exported
format_revision_identifier
- on versioncontrol_format_revision_identifier to return a simple $revision string or some formated-by-the-backend string(ex. 7-chars cut SHA1 instead of complete 64-char on git backend)
operation
- on operation::insert, actually after main values are on DB, to notify actual backend before all other modules
- on operation::delete, just before read operation delete, let backend delete its own stuff
alter_repositories
- on repository::_amend_repositories(called from getter), let add own backend stuff to the repos
repository
- on repository update, insert and delete, let the backend manage its own stuff to the repos
account_username_suggestion
- on account::usernameSuggestion to get, if possible a backend aligned username; now depends on repository, but maybe it can be changed to depends on the backend
is_account_username_valid
- on account::isUsernameValid to get, if possible, a backend desition about username validity
account
- on account update, insert and delete, to let the backend to manage its own stuff about accout data

On item class, getParallelItems(get_parallel_items), getDirectoryContents(get_directory_contents), exportFile(export_file), exportDirectory(export_directory), getFileAnnotation(get_file_annotation) there is a direct callback(no versioncontrol_backend_implements verification, so it should be before the call)

Propose

So, here my intial propose:

all VERSIONCONTROL_CAPABILITies are used in a way that anyone of them need a function/method behind, they only change how VC API(who _know how to deal all cases_) is gonna use the backend; so it makes no sense to convert it into interfaces
there is only one vc flag, VERSIONCONTROL_FLAG_AUTOADD_REPOSITORIES, and it's used to manage own backend repo table; so it makes no sense to convert to an interface
import and export vc callbacks could be repository class interfaces
format_revision_identifier, is_account_username_valid and account_username_suggestion vc callbacks can be converted on threee backend class interfaces
operation, account and repository vc callbacks can be converted in three operation, account and repository class interfaces; but here I think it's more natural to convert that callbacks on unimplemented methods on operation and repository classes.. in that way for example git backend would implement a VersioncontrolGitRepository::[insert|update|delete] if needed
alter_repositories vc callbak can also be an unimplemented method on repository class
not sure about direct callbacks on item class, one interface in the class per feature seems like too much, but they seems also independent; so guide me here please :-p

Comments

Comment #1

sdboyer commented 24 July 2009 at 20:58

Lemme first note that it was my mistake before to suggest that capabilities == interfaces. That's just too much of a generalization. It would be quite a stretch to make VERSIONCONTROL_CAPABILITY_ATOMIC_COMMITS, for example, handle all of its differences simply by adding methods. It's the wrong approach because, as you point out, the capabilities information is about changing logic in how the API runs, often outside the scope of any particular method we might be able to place on a backend-specific object.

There's another way in which interfaces are wrong, as you've pointed out. While commit atomicity is a clear case of something that cannot be elegantly expressed via interfaces, inheritance, or any other OO goodness, VERSIONCONTROL_FLAG_AUTOADD_REPOSITORIES is a great example of something that should (and with my recent changes, already has) go away, as the role it plays can be managed entirely by class inheritance. And still no need for interfaces.

While I can see a way that we might use interfaces to handle commit restrictions, it may well be more trouble than it's worth. So at this point, I think I'm going to back off my original advocacy for using interfaces all over the place, and suggest that we continue to proceed as we have thus far - continue porting stuff over to OO, and handle the potential to use interfaces on a case-by-case basis. So, wrt to that, I'll address your bullets one by one:

Already addressed this, and I agree.
Already addressed this, and it's pretty much already gone.
Mm, sure could. Though I think we'll only get benefit out of it if some of the backends implement it, and others don't...and I can't think offhand what the status of that would be.
I don't know what benefit we'd get from actually making interfaces out of these. See my comments on the last bullet, below.
Yeah, no need for interfaces here. Polymorphism can take care of it - the parent implements it, and the child overrides and adds/alters as needed, if needed. dbtng makes this easy even if we're doing queries, and I demonstrated how we can employ wrapper methods to make it work smoothly in in my changes demoing dbtng.
I personally am hoping we can completely get away from using alter hooks to have the backends communicate with the API. Alter hooks are, IMO, typically more intended for other coders to cut in to our code in ways that we hadn't anticipated. If we need to use them inside our own code to make it work, it's a sign that our architecture could be better. Most of this stuff can be handled with a little refactoring and good polymorphism, anyway.
I'm a fan of thin, granular interfaces. Sometimes you feel a little silly if an interface just has a single method, but my general rule is this: if it is worthwhile for code outside the object's scope to know whether or not a given object is capable of/implements a particular feature, then it might be worth making it an interface. More concretely, if the only time you would check the interface from external code is right before you call the interface's method (i.e., you're checking in order to avoid a php fatal error for calling an undefined method), then an interface might be overkill. Just use method_exists(). If, however, the presence/absence of that interface has bearing on the behavior of external code in some way that's larger than whether or not one of the interface's methods get called, it probably does deserve to be an interface.

Let's keep on chuggin!

Comment #2

marvil07 commented 28 July 2009 at 09:01

Status:

Active

» Needs review

wooo

following the bullet answers :-D (yep, I'm happy because I have one solution working) .. please take a look at my github oop branch ;-)

so, not anymore to say, capabilities stay
and now commited ;-), no more autoadd flag
import_accounts and export_accounts vc callbacks are only implemented now by cvs backend(and not by git, svn, hg), so it should be a good idea to make it an interface
format_revision_identifier, is_account_username_valid and account_username_suggestion are now a methods, in that way inheritance is the interaction way
repository, account and operation are now converted to use inheritance as its way of interaction with backends.
repocache::_amend_repositories(), the only one method which call alter_repositories hook is not used anymore, so if the backend want interaction it had to use repocache::_getRepository()
Based on that approach, which _really_ makes sense, I made the following desition:
- getParallelItems(get_parallel_items), getDirectoryContents(get_directory_contents), exportFile(export_file), exportDirectory(export_directory), getFileAnnotation(get_file_annotation) are now interfaces because each vcs may implement it differently and they are all optional.