Convert Version Control API to OOP

Nice, now we've got a basis for further discussion :)

- maybe we want to remove "versioncontrol_" prefix on each class methods

Definitely, and in addition to that, we also want camel-case naming for methods, and remove the "entity" name from the function. (e.g. VersionControlRepository::load(), VersionControlOperation::getItems(), etc.) The DBTNG API in core uses this kind of naming - which is derived from official PHP standards anyways - and we want to comply with that instead of C-style naming like in Views (e.g. views_field_handler::get_options()).

Another point to consider would be dropping the "get" prefix. For PHP, method names like getIterator() are standard, but in fact the "get" doesn't say anything, and there's no exact line to draw between functions with a "get" prefix vs. functions without. What's "get", what's "load", what's "format" and "theme"? Imho, they're all the same essentially, apart from implementation issues like "retrieves stuff from the database" or "retrieves stuff via the theme system" or similar.

If I would create Version Control API from scratch, I'd do it without "get" prefixes. That's also standard in Qt APIs, and Qt is of course teh r0xx0rz (as everyone knows). Input from Sam highly appreciated.

not really sure where should go:
- versioncontrol_user_accounts_title_callback(), looks like a helper for profile tabs, so should it be left on *.module file

Yep, helper function, needs to stay where it is.

loose some sense in OOP:
- versioncontrol_is_file_item() and versioncontrol_is_directory_item() loose sense if we have item and directory clasess, but not yet decided

(nitpicking: not "loose", but "lose")
As I stated in our previous email discussion, I'm not yet convinced that items should be split into two different subclasses. If we go with a single VersionControlItem class then we still need this function (isFile(), isDirectory()).

Further comments:

1. get_operations() and its specialized variants for commit, tag and branch operations belong to VersionControlOperation class as static methods. Also, _versioncontrol_construct_operation_query() and _versioncontrol_operation_constraint_info() are in fact private methods of that class.
2. Likewise, get_backend() and get_backends() are static methods of VersionControlBackend. Method naming is subject to discussion, my first stab would be forId() and multiple().
3. fetch_item_commit_operations() is VersionControlItem::fetchCommitOperations(). get_operation_items() is VersionControlOperation::items(). fetch_source_items(), fetch_successor_items(), get_common_directory_path() and get_item_paths() apply to a list of items, we might consider to have a VersionControlItemList instead of a single array, in order to have it perform lazy loading and stuff. The other possibility would be to keep items in memory as a static copy, but that needs further thought.
4. get_item_history(), fetch_item_revision_id(), get_item_selected_label() and preg_item_match() are all methods of VersionControlItem.
5. On that matter, fetch*() methods should be completely unnecessary now, as with objects we can do that on demand whenever the API user needs the information. Example: VersionControlItem::fetchCommitOperations() (see above) can be reduced to VersionControlItem::commitOperations(), and the item object keeps track whether the commit operations have already been fetched or not (is_null($this->commitOperations)).
6. As you might guess, ensure_label() and insert_label() are methods belonging to VersionControlLabel, and account_username_suggestion() as well as is_account_username_valid() belong to VersionControlAccount.
7. Still not solved: how to represent accounts that do not have an exact uid/repository/username combination.
8. The get_url_*() functions are actually the thing that the "repository url backend" provides. Let's stick all of them into a VersionControlRepositoryDefaultURLBackend class (implementing a VersionControlRepositoryURLBackend interface) which can be accessed by $repository->urls()->commitView($operation) or similar.
9. The functions from versioncontrol.backend.inc do *not* make up a VersionControlBackend class, but should rather go into the respective classes that they work on (e.g. VersionControlOperation::insert(), $repository->delete(), etc.).
10. Functions with ($repository, $item, ...) parameters are VersionControlItem methods, including get_parent_item(), get_parallel_items(), get_directory_contents(), export_file(), export_directory(), and get_file_annotation().
11. The "file-level revision" property can also apply to directories (think of SVN) so it's a property of a VersionControlItem object. And please, let's keep calling it "revision".
12. Regarding VersionControlOperation, "committer" is spelled with a double "t". This is a serious change from the current "author", and probably we should have both. Or maybe not? Stuff to think about.
13. While dia2code might be cool, I'm a strong proponent of doing important stuff manually. We're going to have file names like versioncontrol.operation.inc instead of VersionControlOperation.inc, and we're going to keep our doxygen comments. And most importantly, I want every change to be done consciously, with autogenerated code you don't think about everything that you modify.
14. The dia file is not that large, and certainly does not require to be compressed twice... just post it in its original version, drupal.org should have the necessary resources to handle a 200 kB file :P

That's all for now! Looking forward to the next revision :]

More appropriate issue title.

Also, this is a task, not a feature request.

Whew. Lotsa stuff to comment on. I WILL get to this in the next 36 hours - just commenting for now to say I've noted this is here :)

(if I omit responding to something, consider it a thumbs-up)

The coding standards page is out of date with respect to classes. Most people who've done OO in drupal disagree with merlinofchaos on that point, myself included :) Truth is, as much as I love Earl, views is increasingly becoming the island bastion of C-style in an ocean of CamelCase. Which is another way of saying that I can't even imagine the necessary chain of events that would get core back to C-style for classes. Moreover, there was an explicit discussion (#260220: OOP standards) about the appropriate formatting, and the direction we went was CamelCase, in large part because it's the PHP standard. It's also not quite true to say that DBTNG was the first thing that went into core that made heavy use of classes - it had been standard fare in Simpletest for months prior to DBTNG getting patched in.

And, here's one of those super-minor nits that I can totally imagine us arguing over forever: I think class names should be Versioncontrol, not VersionControl. I don't like camelcasing strictly at word boundaries because it's not especially meaningful - rather, we camelcase at conceptual boundaries, which are often (but not always) word boundaries. I.e., the same way we use underscores: the module name is versioncontrol, not version_control. I think the reasons why are obvious in C-style - so shouldn't the same principle apply with CamelCase? I'll stick with the form we're using for the remainder of this post, at least, though...

I'm actually gonna disagree with you (jpetso) on the removal of initial verbs from the method names. Your reasoning is sound, with the exception of the fact that it basically assumes read-only. Which all we need now, of course, but if we're ever to introduce support for any kind of writing into vcsapi, then we'd be immediately confronted by namespacing confusion, because we'd NEED that verb there to tell us the sort of action we're performing on any one of these things. Maybe we never get around to supporting writing, but in the interest of avoiding refactoring - and forcing any modules that eventually implement this API to refactor - I'd rather use more explicit method names cases where we can imagine an ambiguity arising, even if it doesn't exist now.

Now, onto jpetso's list:

1. Yup, those should be static. However, the query and constraint info classes should be protected, not private - in that very same discussion about OOP standards in drupal, Crell briefly says why.
2. This confuses me. If we're attaching backend retrieval to VersionControlBackend, then that makes it a factory - NOT a parent to be extended. But the properties that are listed on the diagram right now are all the properties a concrete backend subclass will need to define. This class needs to be one or the other; personally, I'd prefer to keep it the way it is in the diagram (extendable) then leave those backend retrieval functions as plain functions. I don't see what making a factory class from them would gain us over the functions themselves, so it seems like just unnecessary added complexity. But maybe I've missed something.
3. See my earlier comment about removal of verbs from method names. Maybe the key here is that some of these classes are themselves strictly read or write only? That's actually how svnlib works. As for static var storage, no! Not in OO :) Operations and Items are all going to have a single parent, which means if we're going to store them anywhere, we store em on VersionControlRepository.
4. Ya.
5. Initial verb issue aside, ya.
6. Ya.
7. Sorry, I wasn't aware of this problem before, and am not quite sure what it refers to. Clarification?
8. Ya.
9. Ya.
10. So it's probably gonna be worth having a discussion at some point about using SplFileInfo as a parent class for VersionControlItem...but that's kinda complex, so I'm just gonna throw it out there in the abstract for now.
11. Ya.
12. We DEFINITELY need both. Git has some structured support for differentiating between committer and author, and it's a perfectly reasonable conceptual distinction anyway, so yes.
13. I'm with marvil07 on this - for the initial conceptual stages, I like using diagramming wherever possible. But I'm also with, I guess, both of you on this that we're rapidly approaching the time where it needs to be written by hand :)

I believe I mentioned somewhere else, but the whole approach to versioncontrol_backend_implements() really needs to be overhauled. Not only is the current approach infeasible for OO (simply checking if a function exists won't help), but it's FAR better done via interfaces and/or static properties. It's a basic good OO rule that you want to put that sort of metadata somewhere that you can retrieve it and use it prior to object instantiation, so let's be good OO citizens on that one :)

Some of the points that are still open after sdboyer's comment.

ad CamelCasing: The module name "versioncontrol" is not "version_control" because that's a major pain in the neck to write over and over again, and it doesn't look cool. Neither of these reasons apply to VersionControl. And as you all know, a module that doesn't prefix its own C-style name to functions is evil. Of course, if it were for myself, I'd probably just prefix "Vc" to each class, like I did with "Tf" for transformations, and afaik there's no styleguide that tells us how to name classes. All in all, I just find "Versioncontrol" totally unstylish, it's neither fish nor fowl. But as you said, this is something that can be discussed to death with no end, so I'll just leave that issue to you (Sam) so that I won't need to argue about it because it makes little difference in practice. (Still... think of the coolness factor! DX!!1! erm.)

ad 3. I did not say "remove verbs from method names". I said "remove getter verbs from method names". Any setter verbs must of course be preserved, including methods that perform any "write" action on stuff. Like, $repository->setName($repository->name());. (Hopefully I addressed the same issue that bothered you. If not, please clarify.)

ad 7. Hm, I thought I mentioned that in one of these recent mails. Anyways.
An account, in its current incarnation, is uniquely identified by either the uid or the combination of repository and username, with a 1:1 mapping in each direction. However, that's not good enough as we want to support multiple accounts per user and repository, and for that, the uid to repository/username relation is a 1:n one. What I had in mind is some entity providing facilities like this:

$wae = VcWhateverAccountEntity::fromUid($uid);
$wae->accounts(VERSIONCONTROL_ACCOUNTS_NESTED); // keyed by 1) repository, and 2) username. (like for current versioncontrol_get_accounts())
$wae->accounts(VERSIONCONTROL_ACCOUNTS_FLAT); // simple array of accounts
$wae->accountsForRepository($repository); // flat
$wae->accountsForUsername($username); // flat
if ($wae->matches($repository, $username) /* boolean */) { blah(); }

$wae = VcWhateverAccountEntity::fromRepository($repository);
(...same thing in a similar way, only with accountsForUid() instead of accountsForRepository()...)

We could well use this wherever the exact combination of uid/repository/username is not known, including on some of the account forms (for user N and, er, *which* repo/username combination?) and in places where we want to refer to such "partial" accounts but don't want to fetch all information about them from the database. See occurrences of versioncontrol_get_accounts() to (hopefully) get a better idea of why we need this, I find it a bit hard to explain.

Of course, the above API is utter crap, which is the main reason why Version Control API 6.x-1.0 still doesn't support multiple accounts per user and repository - I just didn't know how to present the information in a way that's sufficiently accessible to the developer. Any good ideas on that topic will save my day.

ad 13. I just meant dia2code, not diagramming in itself. Code generated from stuff like UML is evil most of the time, while proper diagrams can totally convey useful information fast, and I appreciate the diagram here too.

ad versioncontrol_backend_implements(): While I think that this approach does work pretty well outside of OOP, it definitely needs to be replaced by interfaces. The challenge here is to find combinations of functions that will be implemented together, because one method doesn't make sense without another one. The split between database storage and on-the-fly-retrieval of information is an obvious starting point, although I believe the final interfaces should be more fine-grained to allow less-featured backends to work in some scenarious as well.