Posted by marvil07 on October 5, 2009 at 12:34am
3 followers
Jump to:
| Project: | Version Control API |
| Version: | 6.x-2.x-dev |
| Component: | API module |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | co-maintainer, gsoc, gsoc2009-marvil07 |
Issue Summary
GSoC2009 is finished, and I wonder if I could commit the changes I made on my github branch for the project to cvs.
I also want to help to commit some patches I wrote while I was writing those changes to the actual 6.x-1.0 branch.
@sdboyer, I know you're now reviewing my work, but I could start closing issues(at least in the actual stable) if I get access to cvs.
Comments
#1
upps.. some patches I wrote
#2
yeah, probably better to have the conversation in public than in irc :) sorry about all the delays, and i'm now in boston, then portland, until beginning next week (which means no time for work on this, yet again). short answer is YES we'll be committing them to a new branch, and really, we don't need to wait on me finishing my review to do it.
again, the delays are ridiculous, and entirely my fault, and i'm sorry. first thing when i get back sunday night.
#3
Love the delays! Apologies again.
I've committed your changes to HEAD, and you've got CVS access now. We still do have a ways to go, and I'm a little troubled by some of the interface usage I saw in the most recent commits. But we've also come a long, long way :) Here're my comments on the work:
===========
== Comments, itemized per commit
73ef1976be3ccf2473753ec4fd45a8fc6ba6eafa - Storage of the backend as a property of the repository object seems a little sloppy...sorta like it's something that could, maybe should be better solved somewhere else? I mean, the child class implementations should always only ever be associated with a single backend...
946952c92bf7a73769ff74bde66d3f6b0de2ace0 - queries need to be updated to DBTNG
7ea30b87236e56529ed46c0b4642e466c154ca88 - Things like getting the repo cache should be done in the implementation of the DrupalWebTestCase::setUp() method
c6a78ca8bacef1fae86eefa46e619d08f69e9cda - This kind of logic is sloppy. If at all possible, we should ensure that the data is _always_ serialized or unserialized by the time it gets into this method. (I'd prefer unserialized).
6a23c181bfe534c6d098e44d25f5c55596be6068 - The right intermediate fix, but doing OO properly means completely getting away from this style of communicating information to the repo objects.
--7bdb761fae1f0c31ae537d63fdca74863828592d - It's a fairly pedantic point, but the _update() method introduced here is not in keeping with the same pattern I established for fully leveraging DBTNG...kind of. multi-table UPDATE is far from easy or really even worth it, so passing a query object around isn't helpful, but we should at least be consistent on how we do the naming. And no more prefixing internal methods with underscores!
08473c98d0b81f59616097519837667578668413 - Similar to last comment re: naming style for child class query behaviors. Really, we should just let them override and then call the parent, probably; fewer methods, loosens the coupling.
db6c81d9bda5568a254f4564889ed70762db9b6c - I wouldn't be opposed to some simple wrapper helper functions to make some of this less ugly. f.e., take:
$vcs_account = new $repository->backend->classes['account']($username, $uid, $repository);
and turn it into:
$vcs_account = versioncontrol_get_account($repository, $username, $uid);
and versioncontrol_get_account() function could probably be just a one or two-line wrapper.
1e3ed2cfaeea2f9bf7b5d6deeaa663bc4f0a8b7d - Again, use real visibility on methods, and don't prepend private-tending methods with an underscore!
873a67d9af5eca3d65aa78d0c0bb2574c0d9849d - This commit, and some of the subsequent ones, are interesting. Really seem like places to consider employing some of the classes in the SplFileInfo family.
6c0504b4d0c362ba0884af12dd174fce643aa9cc - Weird to have the verbal construction in the name of the interface itself (VersioncontrolItemExportDirectory). Interfaces should be nouns, not verbs; save the verbs for methods.
e141ef9d8f894f28d189c702adc3d38343397a44 - Still a lot to do with dbtng in here.
d10cad7bdde16207ce67e83a845895a7d876d63e - What is the VersioncontrolRepositoryUrlHandler doing for us?
54920724a2b423afd0750e175ade4f9d6c14cf1f - OK, I'm starting to wonder if I'm really missing something about why you're making interfaces out of things like VersioncontrolRepositoryGetItem. What's the architectural significance of that interface? Do we have any VCSes that DON'T implement it? Unless I'm really missing something, it seems to me just like splitting off an interface for the purpose of doing it, nothing more. Judging from the corresponding changes to the VersioncontrolRepository::getItem() method, though, this really doesn't seem like a proper use of an interface.
== General Comments
- Good job of doing nice, incremental, descriptive commits. big woot.
- I still REALLY hate caching loaded repositories based on constraints. It's just UGH.
- At the moment, our backend classes are really little more than arrays. There's no substantial reason for us to have made them objects at all. The drupal OO philosohy I adhere to says, "unless there's a clear reason to make it an object, don't." That applies even if our explicit goal is to OO-ify something in drupal-land.
- Pursuant to this...well, it's a big change, but we might consider offloading object loading logic into the backend classes. If we do that, then we could, I think, avoid some of the complications with the buildSelf()-type approach that I'd advocated previously, making for cleaner objects - we could be guaranteed that if they're built, they're fully (or at least sufficiently) populated.
- ...of course, it might also be better to put this on the repo. though we could start getting into problems at scale, i suppose...although it might work if we generally follow the VersioncontrolRepositoryCache pattern i laid out.
- We're actually going to want to employ the entity controller system (once we port to D7, anyway). They really are a pretty well-formed pattern for the type of thing our Versioncontrol***Cache classes are doing.
- How much are we _really_ gaining from using ArrayAccess? Seems like just a backwards-compat thing, and I'd rather not (ever) support that, per the typical Drupal approach.
== Before we branch and update on d.o...
#4
(reminder that I'm in Vancouver this weekend for the contrib upgrade sprint, so I won't have time to respond to anything until Sunday, at the earliest)
#5
first...
- thanks for reviewing my work!
- woooooo finally my work on d.o!
Before starting the replies, I think _a lot_ of directions are lost commiting the work as a single _big_ commit(8563 lines! as standard git format-patch). So I suppose a CHANGELOG file or something like that is now _needed_: #606660: Document API changes from 6.x-1.x to 6.x-2.x.
So, is the work going to happen on HEAD instead of 6.x-2x ? (I suppose)
I was just before closing many issues because oop solve many little problems for 6.x-1.x, but using HEAD for development and not having a branch for 6.x-1.x, should I think that it is not more maintained? If not we can close all those issues because HEAD have solved it, but if not, we need a branch and need to apply those changes.
So, I think this is a big reply, but I'm glad to talk about it finally! :D
Yep, a repository child must _only_ implement one backend. Actually I can not imagine a different case, but if you have some suggestion about another possible use of repo children, it'd help me to understand your point.
:o I though we were actually not sure to depend on DBTNG(#528966: dbtng integration). So, I would apply the changes on oop-dbtng branch from github to HEAD if we are going to do so.
Yep, actually that commit do that:
$this->repoCache = VersioncontrolRepositoryCache::getInstance();From other methods the repo is obtained from that cache retrieved on
setUp()in this way:$repository = $this->repoCache->getRepository($repo_id);Yep, I remeber I want to do that, but I was not enough sure that it happens on all cases, so lets make this one an issue to take time to review : #606664: make sure we have an unserialized array at repository build
:o ok, but I think I do not understand you completely. AFAIR In that commit I was trying to fix the interaction between the form and the repo object, not between objects. So if it still an observation, maybe you could open an issue about it to get in deep about the topic.
I think I'm not understanding all the point, (so maybe you want to create an issue where we can discuss this in detail? )
In the other hand, quoting the comment on a6ab3dc:
distinct underscored methods
finally finish following sdboyer initial suggestions
(http://groups.drupal.org/node/23015#comment-79517):
- There are prolly other ones, but one coding convention we have is to
get rid of the prepended underscore on function names when they become
methods, and just make em protected. f.e.
VersioncontrolItem::_badItemWarning.
Now we have methods underscored _only_ when it is a "vc api call to
backend implementation through an interface, vc api controls the flow,
the backend only provides the right way to do the stuff"
We _still have_ underscored functions with original meaning("file private
function")
I think that you see underscored methods as a bad pratice, so there's no problem to change it for other thing that let identify the group of methods.
In the other hand, making this instead of let overwrite is intentional because I wanted to preserve the flow, for example in
VersioncontrolRepository::insert():public final function insert() {
if (isset($this->repo_id)) {
// This is a new repository, it's not supposed to have a repo_id yet.
unset($this->repo_id);
}
drupal_write_record('versioncontrol_repositories', $this);
// drupal_write_record() has now added the 'repo_id' to the $repository array.
$this->_insert();
// Everything's done, let the world know about it!
module_invoke_all('versioncontrol_repository', 'insert', $this);
watchdog('special',
'Version Control API: added repository @repository',
array('@repository' => $this->name),
WATCHDOG_NOTICE, l('view', 'admin/project/versioncontrol-repositories')
);
return $this;
}
the flow is almost everywhere for this type of interaction in this way:
- update db
- let the child interact with the repo object(probably to insert data on own tables, but with the actuall data field in repo table now it's probably no more needed except we need some specific quick access or we're storing big size values/data per repo)
- call a
module_invoke_all()to let interaction with modules that _are not_ backends- log the action
If we let overwrite the method we lose control about what's going to happen, and actually I do not see any other use of
_{insert,update,delete}()than interacting with own tables; so this pseudo-mini-action let the needed interaction. But agai, if you have other uses maybe we can change it.:D, yes I also think thata lines look __really ugly__, so good idea. And I think it's also related with your last comments: "backend as array"?
In this case, like other "underscored methods", I use it for forcing a "good" workflow, letting a fallback; but maybe is a good idea to rename the underscored method to a with a new "identifier"/name instead of _
So if you consider it necesary please open an issue to detail thoughs.
:o, so I'm opening an issue for this(I really need to get informed more about SplFileInfo :p): #606676: inherit from SplFileInfo?
:o right, so maybe
VersioncontrolItemDirectoryExporteris better.. so I think we're opening lots of issues :p : #606678: remove verbs from interfacesYep, but like I said above, it depends on #528966: dbtng integration
Let manage "easier"the urls. actually this commit shows what it's doing. Mainly use methods instead of direct queries. This started as the jpetso propose to have pluggable url backend for repos: #354509: Make repository URL backends pluggable but after analyze it I discarded it.
:o. I just tried to base my interfaces on your comments #521792: thinking on interfaces(aka define them), just quoting some relevant parts:
marvil:
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
sdboyer:
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.
Maybe I misunderstood you :-(
Point by point:
- :D thanks! Like I said above, a changelog joining the main changes should be on a changelog to avoid losing them in the one commit(some people don't like it :p) for kind of 3 months work.
- :o, so I think you can ake an issue about it to avoid forget it
- Backend class can be an array now I suppose. At start I though backend class was going to hadle the optional functionality, but then each entity as a class make this non-real, so IIRC we can change it with no so much effort.
- I think I'm not understanding it, but sound me like moving **cache classes code to backend?
- ^
- :o, so apropos that, are we going to start a D7 branch, I think it would be a better idea to wait until we think we have a ready-to-use oop versioncontrolapi(aka finish 6.x-2.x)
- Yep, that should be one more item on OOP_TODO: remove ArrayAccess dependency everywhere(aka finish oop migration).
#6
btw I forgot to mention that I made 2 commits these days to github, so I think that both was not in your last pull; so I made those minor changes directly on HEAD :-D
#7
Hi!
I'm planning to start working on this again from Dec 13, so it would be really great if we have some decisions taken on that date :-)
It's important because there are architectural and maintainer decisions to take here, so I definitely appreciate more opinions before I do it "wrong".
#8
changing status
#9
I'm marking it as critical because I can not do so much without Sam approve for now :-( and because lot of issues depend on this
#10
BTW, there's been movement on coding standards since I think we last talked - http://drupal.org/coding-standards and http://drupal.org/node/608152 . Yes, more Crell (and me) :) I see why we were using them - indicating a public method that is really only intended to be called by the backend - but I'd still rather we use something other than underscores. I like your idea:
The other crucial thing will be to document them in the docblocks in some way that's consistent. Maybe with an appropriate @ grouping or something.
Re: interfaces: I'm actually not sure what all more to say that isn't in the section of what I wrote that you pasted in. VersioncontrolGetParallelItems is maybe a good one, since not all backends necessarily implement it; however, VersioncontrolRepositoryGetItem doesn't make sense as an interface because all the backends must implement it (...right?), so separating that method from the class has no architectural significance.
We definitely, DEFINITELY want to depend on DBTNG. We'll be able to cut out SO much unnecessary logic by using it instead of the nasty querybuilders that we've got right now. That does mean refactoring & some rethinking, and is probably the biggest remaining todo, as it does require more rearchitecting.
#11
Oh and I agree, no D7 until the OOP branch is stable.
D7 really probably should move some of this stuff over to being Entities.
#12
#13
So, finally I think I created the last 4 issues needed to close this one :-)
#723944: make more readable class creations
#723950: backend class back to an array
#723958: Remove ArrayAccess dependency everywhere
#723962: Code standards review
#14
Automatically closed -- issue fixed for 2 weeks with no activity.