Document class methods with class, factory and defining interfaces

Although I'm not opposing more comments, an IDE like Eclipse, and the PHPDoc, will outline what class and function you're looking at ala http://img206.imageshack.us/my.php?image=eclipsedi7.png .

This is in comments, but IMO is still a hack. I'm OK with improving documentation in the database layer, but this still feels like a hack for editors whose most advanced feature is "find". :-) There should be some better method.

Perhaps we should reach out to the broader PHP community and see if there's a common convention here?

I've never run into this because as RobLoach points out, any modern IDE will show a class hierarchy.

I'm also not real big on doing this with comments, because we don't similarly add things like:

if (arg(0) == 'node' && is_numeric(arg(1)) {
  lots and lots of code...
  lots and lots of code...
  lots and lots of code...
  lots and lots of code...
  lots and lots of code...
  lots and lots of code...
} // End check for node/X.

And lots of people find /that/ confusing as well.

I'm with Larry; we should explore what the standard nomenclature is for documenting these functions, if any, and use it.

Summary of conversation in #drupal:

chx was away from DBTNG for a couple of months and then went to work on #356074: Provide a sequences API. Larry says:

"Why in the world are you calling Database::getActiveConnection() from within a connection object? $this->insert(). That's all you need."

chx panics, because if he can't figure out DBTNG, then we have a serious problem.

The issue is when you grep for something like 'function execute(' or similar, you end up with 15+ results. This in itself is not a horrible thing, since if you grep for 'function setUp(' you get many more than 15 results. ;) The problem is that if you grep for '\->execute(' you get 100 billion results, because this function is called from everywhere, where '\->setUp(' is only called from a single place.

Now, when you're trying to debug why a certain execute() statement is failing, you're in for a wild ride trying to back-trace through the various inheritence/composition hierarchies to find out *which* of those 15+ execute() functions you are actually calling. The idea behind this patch is to leave some bread crumbs so that it's easy to tell from a quick grep which execute() function is being referred to.

chx pointed out that other languages such as Java won't have a standard for this, because they will assume their code was written in an IDE with a class hierarchy. I can see that this would be confusing without such tools. However, that said, we are catering to an incredibly small crowd here: one that debugs OO code with grep and a text editor. We've long had areas of Drupal (form.inc being a prominent example) that simply require busting out into an IDE to be able to understand wtf is going on in there; I'm not sure why we're special-casing this one, esp. when there are no pre-existing standards to support inline commenting of functions.

If we go this route, we need very clear guidelines to module developers on when they do and do not have to comment their function definitions this way. chx suggested something along the lines of:

"If there are more than one method called the same and that method is called from multiple places then you need these."

This would apply to Views 2 and DBTNG, but not SimpleTest, for example.

This still needs discussion.

The problem with the guideline chx suggests (as quoted in #5) is that "more than one method call with the same name" would include every single time we use inheritance or interfaces. Multiple methods with the same name that do different things is the fundamental underlying concept behind polymorphism, which is the cornerstone of OOP design IMO. We'd be providing documentation on how the language syntax works everywhere we use it. We don't do that for any other part of PHP, so I don't see why we'd single out methods to turn into a language tutorial.

I'm not sure I follow chx in #6, either. db_select() will dynamically create an object, not a class. :-) And that object can be debugged just like any other variable if you're using a RT debugger.

To be fair, fully chained methods mean that you never have a named variable for the object for you to examine. However, the same is true if you nest function calls inside each other, something we do often. For instance, consider http://drupal.org/node/111127#comment-1215783 :

array_keys(array_diff_key(array_flip($nids), $cached_nodes));

You can't separately debug the result of any of those intermediary steps, even though that's arguably easier to read later and saves a byte or two. The solution for debugging a fluent method chain is the same as it is here: Break the code up into separate steps (that are not called fluently), debug them normally (for whatever is normal for you) and fix whatever you're doing wrong, and then merge them back together. I do that all the time, in procedural code no less.

Re #8, terminology difference. It constructs the class name dynamically, not the class itself. That's no more or less obtuse than our extensive use of call_user_func_array(). (It's pretty much the exact same concept, just done in OOP syntax.) Re #9, same thing. :-) We do have the same "name" appearing multiple times. It's just in the form of hooks rather than method names. Re #10, OK, point. We'd be providing documentation on how the language syntax works in a great many places we use it. :-)

The PHPDoc answer seems to probably be to use @see:

http://manual.phpdoc.org/HTMLframesConverter/default/phpDocumentor/tutor...

I'm not sure what that does to inheritance of documentation blocks.

chx pointed out that other languages such as Java won't have a standard for this, because they will assume their code was written in an IDE with a class hierarchy.

Please don't assume this. I write OO code and I do it without an IDE. It's not a magical tool that leads to understanding, nor is it required to gain understanding.

It seems to me that the real reason for this is because api.drupal.org does not yet support classes and methods. If it did I think this would be moot because we could just go to the documentation...

I use vim for everything, and I've got no plans to move to an IDE at the moment - and I like to be able to follow code flow around with dpm() and reading the source - even with call_user_func_array() you can always debug_backtrace() if you hit a wall. Don't have a strong preference between @see and inline comments yet.

"Comments should be on a separate line immediately before the code line or block they reference." (http://drupal.org/coding-standards)

I.e. exactly this syntax is incompatible for coder_format.

Something like:

// @see SelectQuery->execute(), db_select->execute()
public function execute() {

would be compatible, and equally findable.

However, my next best question in IRC was not (even) answered: If we already have trouble to find the proper execute method in a set of class definitions, what trouble will we face when issue 1234567 decides that the arguments for SelectQuery->execute() need to be changed? Poor developer who has to dig through core and find out which ->execute() of his grep belongs to the class that needs to be altered...

I'm in the doesn't use an IDE camp as well. You kids with your fancy 200USD IDE's or 6000USD laptops that can run eclipse with something resembling usable performance, should think about developer accessibility. I don't think that is a valid argument against adding a comment to note which class is being used... either in the function header or inline. I for one really like inline commenting...

dopry, there was just a very long discussion in #Drupal where we concluded that this would not, in fact, offer anything useful that would justify the added maintenance cost. Making api.module suck less with regards to OOP and teaching people how to fish is a better approach. It's not about fancy-schmancy IDEs vs. cheap-skates, nor, really, is it about OOP vs. procedural.

Relevant API module issue: #300031: Rework PHP parser

Bah! :P

subscribe,
+1 ;)

The method signature doesn't change for the subclasses. If db_select() was able to specify its return type, then every IDE worth mentioning would pick that up. To wit:

/**
 * Description here
 *
 * @param string $table
 *   The base table from which to select.
 * @return SelectQueryInterface
 *   A new query object.
 */
db_select($table, ...) {

}

And now any worthwhile IDE can auto-complete properly. Simple. It's called "using PHPDoc instead of our bastardized Doxygen as a documentation format", and is something I've been arguing for now for over a year. :-)

I'd still much rather us pour energy into #300031: Rework PHP parser first, then come back and decide if we need this as well.

Er. Crell? I don't understand #25. Why can't you specify return types in Doxygen now?

Edit: Nevermind, I get it. Specifying the return type, not just @return like we currently do.

Add tag.

I still hold there are more standard ways of doing this that work better anyway.

I am still against this for reasons already stated. Either way, though, "critical" means "release blocker" and this is absolutely not a release blocker.

Lots of existing documentor systems know how to link from a class to its interface in the online docs so that you can read the documentation in all its glory. That is not an issue.

My position remains the same: Adding proprietary documentation techniques is a non-starter, especially when there are 2 decades of established, perfectly good OO documentation practices already out there and widely in use in the PHP world. Find me some existing documentation mechanism that will improve the documentation and I'm all ears. But trying to invent our own proprietary documentation techniques, especially just to account for weak editors or weaknesses in our own proprietary documentation parser, I will continue to stand firmly against. We've reinvented enough wheels in Drupal already.

There's only one implementation of ->condition() that has anything of value in it; the one in DatabaseCondition. SelectQuery just passes through to it using composition.

Are you talking about debugging via nano or about api.module? You're going back and forth. Those are not the same question. Pick one.

I use vim for both development and emergency live debugging, and I'd also like to be able to continue to grep core to find things.

The implication on this thread is that only very experienced Drupal developers who are also luddites are using text editors rather than IDEs to develop. This seems completely wrong to me - people coming from having a spattering of HTML/CSS like I did are either going to be using either something like dreamweaver, gedit, kate, notepad++ and other tools like that. People who double up as their own sysadmin or come from that background will also be familiar with vim or nano. Also don't forget that a lot of people starting on Drupal development don't even have a local development environment and are FTPing up to shared hosts or VPS. Requiring a full featured IDE to develop isn't only an annoyance to people like me or chx, but is another barrier to entry to people from non-programming backgrounds. Yes it's true that many people should hopefully never have to touch dbtng internals, but you never know.

catch, I honestly have to call hyperbole there. HTML ninjas just writing a little PHP here and there in Dreamweaver are not going to be doing heavy work inside the guts of Drupal. They're not going to be doing deep intricate debugging inside of the database layer. If they want to start doing that, and they want to keep using Dreamweaver, well, we can't solve the problem of people using the wrong tools. Their own tools are a bigger barrier to entry than anything we are doing.

If someone wants to understand how to debug complex OO, they're going to need to know OO concepts and how to read OO-based documentation (really, no harder than procedural docblocks) long before they get to the point where they'd need a fancier editor. You're asking us to go out of our way to let someone with a bicycle pump fix the transmission on a Ferrari.

And yes, I was a nano fan for more than half of my programming lifetime, including OO code.

My position in #38 still stands. Inventing new, proprietary documentation hacks is a non-starter. Documentation tools for this and even more complex OO already exist, even in PHP.

FWIW, I'm with catch and chx. I use a plain, simple, and - most importantly - fast text editor for all of my Drupal development work. And I already tried to understand DBTNG internals, but I had no (and still don't have a) clue. Therefore, I effectively was not able to contribute a single line to includes/database*

But anyway, documentation. This reminds me. Perhaps someone will close comments on this issue soon.

I agree that we should mark this 'won't fix' for now and that we should see what happens. We can decide to reopen as necessary. Let's get a bit more data.

We've been over this. Non-starter. Creating our own hacks is not going to help us learn how to work with the real standards in this regard. There's a much more productive thread elsewhere. Let's talk real ideas there, not hacks.

#718596: Lacking standards for documenting classes, interfaces, methods

chx, you would be much better served by explaining what about the output on api.drupal.org doesn't help you understand where things come from, and how it could be improved so it would help. jhodgdon has been an absolute rockstar with improving that site lately, and I'm sure any reasonable suggestions could be implemented quickly. You marked this issue won't fix yourself in #18 in favour of View's doxygen site, and these days that lives on api.drupal.org as well: http://api.drupal.org/api/views

This is a feature request, plain and simple. Cataloguing as such.

I'm doing some pruning of the critical feature queue, to get a sense of what's really really important to focus on for the next 6 weeks and came across this. This seems like a task to me, since it doesn't change any actual lines of code, so why would it be subject to a Feb. 18th deadline? However, I don't agree with it counting towards thresholds or holding up a release, so setting to normal. Normal does not mean "not important" though.

chx: We are 52 comments down in this issue at this point... Can you summarize:
- What the problem is.
- Why it is important.
- What comments you want in the code to help you solve the problem.
- If some updates to api.drupal.org could help as an alternative to adding to the comments in the code.

In other words, we need an issue summary, and since you're saying you'll stop contributing to core unless it's resolved, you seem to be the best person to summarize the problem.