Unify entity C(R)UD

Tagging. We need this for a solid API, I'd be embarassed to finish phase 2 without it.

Been working on this, probably halfway there.

Collapsing this with #724616: provide a way to delete an entities from the database, as it really makes most sense to just deal with all the CRUD at once.

OK, here's an initial patch. What's in here is really kinda straightforward, but it took a while because working through it raised a LOT of things to think about. Some notes:

• There's no batched insert in this patch. Batch inserts require quite a bit of backflipping to make sure that we've attached new ids to the right objects, as well as making sure that any secondary, dependent inserts get accomplished as well. So, while it'd probably be very nice to have, I'm going to hold off on doing it until performance tests actually indicate it to be a bottleneck in need of addressing.
• There's also no batched delete in the patch. Batch deletes are less complicated than batch inserts (because we don't have to realign serial-generated IDs later after the fact), but I'm still applying the same reasoning as with inserts.
• ensure() methods are gone (or if they managed to stick around, they're certainly deprecated). They need to be replaced, as it's potentially useful for most of our entities, possibly all (the question is on VersioncontrolRepository). This patch shouldn't be committed until they're added.
• More minor, kinda - seems I totally forgot VersioncontrolOperation in this patch. Not sure how that happened, I know I made a lot of those changes...

There are some remaining schema issues that really need to be cleared up - versioncontrol_source_items is the easy example (do we keep it? or do we just kill it and stick the relevant data into versioncontrol_item_revisions?). Representing operations vs. entities, which is what #879600: Meta: introduce an activity stream separate from commit logs is turning into, is more complicated. Obviously, how the schema works is going to affect CRUD. To the extent that it's possible, though, I think we can keep this issue focused on creating the interface/skelestructure, committing it with the schema we have, then changing the internal logic behind the interface as we make schema changes.

Yeah, glad you like that change. I figure it's a pattern we can use elsewhere, too - if we know it's something the backend should be working with, just stick 'backend' at the beginning of the method name.

Updated patch. Adds two big things:

• Moves VersioncontrolOperation over to the new, unified system. We skip the {versioncontrol_operation_labels} table entirely and use 'commit' as the named entity type in the hook invocations, as I'm expecting that this class will shortly be moved over as a dedicated handler for commits (see #879600: Meta: introduce an activity stream separate from commit logs). This also rips out a lot of logic for managing attached/nested objects (like, the items on an operation), though, which will ultimately need to be restored.
• An $options array as the sole parameter to be passed to every one of the CRUD methods, so that we still have a unified interface but the caller can instruct the object to take different logical paths.

The $options parameter is tied to the nested objects issue. It might end up not being necessary, but from where I'm sitting right now there are too many possibilities for how CUD should behave for me to be comfortable with a single grand-unified method. These possibilities have nothing to do with how each backend operates individually, so inheritance can't help us.

I'm gonna try to get another patch round in on this tonight.

Maybe we can also use this issue to review all the classes data members, IIRC I have seen some inconsistencies on data members like repo_id instead of a repository object and only use repo_id on the drupal_write_record and related stuff.

Not a bad time to do this, probably. I've sorta been doing it as I go along. I've tended to avoid tasks like this because they end up spiraling into other API-fixing tasks, but as we're really getting quite near the end of the API work, it might actually be feasible. But I'm gonna keep it in the "I'll do it if there's time" category.

If we are using an $options variable we should pass it at backend<crud_op>() method and versioncontrol_entity_<entity>_<crud_op>() hook.

Agreed. It already is passed to the backend*() methods, but should also be passed to the hook invocation.

I know this logic is obscure, and it needs a replacement, but for now is needed. I do not see how the operation nested objects are inserted, you also mentioned this to me on IRC, so I am just writting it here to avoid forgetting it and to put it in context.

Yeah, this is one of those nested cases...however, even if we do retain the source items table (which I continue to be on the fence about) the logic here is still too much...I think. More importantly though, that logic should be on the Item, not the Operation/Commit. In any case, though, this logic has been removed and that needs to be dealt with (schema change, some other CRUD additions), somewhere.

It seems like this patch also tried to unify the way we are returning/throwing exceptions, and it seems like it is going to be like DBTNG, returning the object to be able to chain method calling.

Yep, to the extent that it makes sense, I'd like to present a fluent/chainable API.

Maybe is the right time also to unify where we should add watchdog messages. By now it seems like it is going to be nowhere at crud. Is that what we want?

Yes, I stripped out all the watchdog calls - I don't really see a reason to have them in there. Then again, maybe I'm misinterpreting what watchdog is really supposed to track, but I tend to think of it as only having "WARN" and "NOTICE"-type loglevels, not "INFO" or "DEBUG" ones. We can re-add them if that's a legitimate use case for watchdog. Given that it's yet another insert to do, though, it does merit justification.

Definitely we need to resurrect some of this, repo deleting needs to remove all related stuff. So, again nested crud operations for repo this time are needed.

It's more nested deleting, yes, but that's actually one place that I left some some code in there - look at the beginning of delete(), it loads all contained entities and deletes them. That was prior to me starting work on the nested stuff, though, so it does need refactoring - I just mention it because, in that particular case, I pulled out all the old code because those foreach()s at the beginning should actually accomplish the same thing.

OK, added the framework for nesting and implemented a bunch of the necessary nested logic. Also updated to be consistent with #975864: Merge {versioncontrol_item_revisions} and {versioncontrol_source_items} tables. We're pretty close to complete, here, but this DEFINITELY needs a sanity check as I'm going cross-eyed on it. There's a really nasty potential situation with VersioncontrolItem::insert() due to the way that source items now must work, but other than that, I think we're in pretty good shape.

We have some simple fields as data members directly, but for repository we have a special case that loads also the repository based on repo_id. It would be great to have "autoload related entities" instead of just adding foreign keys on the target entity. I remmeber I have implemented some of it for the logic you made at controllers load. Well, maybe that deserve another issue :-)

Lemme tell ya, the thought of introducing an "autoload related entities"-type system was pounding through my skull when working on this. It's one of those rough areas that we don't handle consistently, but really _really_ need to. Since we've essentially now got a poor-man's ORM, we need to make sure that the objects we have loaded always & only get loaded a single time, and get properly updated throughout the course of execution. The fact that we don't have a unified system for managing the autoload of related entities is a huge problem, and something that does need to be addressed, probably first at the controller level and then within the entities themselves.

...but, while it's a bit messy, I think we'll be able to survive, for now, without doing it. Unless we run into a case where it's really just murdering us, I'm ok with deferring it until after we do the public unveil and we've got some more wiggle room.

IMHO VersioncontrolItem::sourceItem should have the same casing that all other data members. Any reason for use camelCase here?

The only, _only_ time I haven't used camelCase (when adding new stuff) is when it's a property that maps to something in the db. Since we don't have a layer that translates from db fields into proper camelCase for properties on our classes, I suffer through that. But - and this does need fixing - if it's not a db property, then it should follow coding standards and use camelCase.

at VersioncontrolItem::getSourceItem() we should handle the source_item_revision_id=0(is a new item) instead of return FALSE ?

Hoo man...the reason I don't deal with that case is because VersioncontrolItem::getSourceItem() is totally irrelevant for new items. That method is strictly about populating the property based on known values that are db-derived. It's not about doing any of the calculations or discovery that might go into figuring out what the item might be. So if I'm following your question correctly, then no, the only time that we can't get an item will be if the item we're currently on HAS no source item - that is, is the first revision of a given item in the repo.

But...maybe I'm overthinking how complicated that all is.

another change I see is the no-more-controlled environment(so backend can see exactly where it should interact), I mean no more final methods as part of a-controlled-crud, not really sure about it, since the backend() method makes sense only in that context(if we use public maybe the recommended way to implement it is just call the parent on the child method).

I think you probably remember me arguing against using final way back when y'all first introduced it :)

The architectural thought pattern I'm following here goes like this:

1. We need to provide a public-facing method for external code to call. This method should encompass as much backend behavior as possible, without doing backflips. The real benefit of this is reducing code duplication.
2. We should provide additional sub-methods, called from the main method at structured points in the logic flow, that are expected to be overridden for backends to run their decorating/additional logic.
3. This second point is especially true in cases, like update/insert/delete, where the logic sequence is highly sensitive and not easily overridden.
4. In the event that a backend cannot do what it needs to with the specialized sub-methods, that's fine - it can simply override the parent's main method and alter the logic as needed.

The crucial thing to remember is that the API and backends are colluding to present a simple interface to third-party code - they, together, are API producers, and third-party code is the API consumer. Thus, it should be our goal to make API<->backend interaction as flexible as possible, so that it can produce the simplest, most intelligible API for its consumer. Controlling the environment with things like final are the antithesis of that.

So that's to respond to your text bit...now I'll look at the patch.

OK, basically all of VersioncontrolAccount's direct updates to any tables other than {versioncontrol_account} are wrong, because it assumes it can just pwn the mapping itself. But we agreed to ignore VersioncontrolAccount for the moment, so I'm gonna mostly skip those.

all crud methods must return the object for chained ops

Probably worthwhile, but I don't want to block this patch on that - it's an easy follow-up later :) I did, however, check, and we're consistent for now - all insert() and update() (and therefore all save()) methods return $this. delete() methods do not, as it doesn't really make sense to.

add docs to non-crud methods

Yeah...later. I want to doc sprint through the entire API once we've got it all shored up. This isn't core, which means I don't think this is a reason to hold up this huge patch :)

But...wait. non-crud methods? like, which ones are you referring to, especially given that this is the CRUD patch?

no-repeated data members between inheritance relations on our entities

Don't quite follow that.

unify hook names (IIRC I see a problem in account hook names), but it worths to review them all at once.
review the flow at each crud method, making sure we are always using the same pattern.

Reviewed, checked, and done. I'm gonna let this cool for a bit while I attack some other things, and probably commit it in a couple hours.

Aggregate and incremental patches are attached.

OK yeah, this sucker's goin in. http://drupal.org/cvs?commit=453330