Per http://drupal.org/node/1112854#comment-4882080 we need to add doxygen to every function in select.inc. This is trivial most of the time, just copy-paste, there's say

  /* Implementations of QueryConditionInterface for the WHERE clause. */

now remove this and instead
<?php
/**
* Implements QueryConditionInterface::conditions() for the WHERE clause.
*/
public function &conditions() {
return $this->query->conditions();
}
*/

like that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

karschsp’s picture

Assigned: Unassigned » karschsp
karschsp’s picture

Status: Active » Needs review
FileSize
5 KB

Patch attached.

adamdicarlo’s picture

Status: Needs review » Needs work
FileSize
11.01 KB

I don't think the first patch is exactly what's needed here.

Here's how I think it should go. I've only rolled the new comments for the SelectQueryExtender class, so this new patch is not done yet. I had to look up the implemented classes to see which class exactly was the origin of the method that's being implemented. I think I did it OK.

@chx is this pretty much what's needed?

If so, @karschsp, want to roll the next off of this? The SelectQuery class still needs to be done.

adamdicarlo’s picture

Status: Needs work » Needs review

Actually, this does need review before we should proceed.

catch’s picture

Do we really need this? In the class definition it specifies which interface it implements (or extends from another class that specifies this) that's enough to go on IMO.

adamdicarlo’s picture

@catch I had assumed the idea was to make the api.drupal.org docs easier.

But looking at http://api.drupal.org/api/drupal/includes--database--select.inc/class/Se..., it looks like the api site is automatically showing what each method is overriding...

So what could we gain from adding these comments? (Anything?)

chx’s picture

When I look at api.drupal.org I see a lot of empty lines this patch would fill in .

catch’s picture

I feel like api module should be able to figure out something from the class hierarchy (ideally a link to the respective method in QueryAlterableInterface).

What is more of a concern to me is there's no way to browse to QueryAlterableInterface on api.drupal.org

http://api.drupal.org/api/drupal/includes--database--select.inc/interfac...

vs.

interface SelectQueryInterface extends QueryConditionInterface, QueryAlterableInterface, QueryExtendableInterface, QueryPlaceholderInterface {
chx’s picture

well, certainly it'd be awesome if api module could figure out this but right now this is a very easy cop out which we can clean up once that happens.

catch’s picture

That's fine with me, I'm not keen on the existing:

-  /* Implementations of QueryPlaceholderInterface. */

so better to replace them with something standard too.

chx’s picture

@adamdicarlo @karschsp please continue. we have agreement this is what we need.

posulliv’s picture

Made an attempt at this. Does the attached patch look ok?

xjm’s picture

Component: database system » documentation

Crossposting #1316902: Clean up API docs for lib/Drupal/core/Database. Also, this is more a docs issue.

xjm’s picture

And, a core/ reroll for convenience.

jhodgdon’s picture

Status: Needs review » Needs work

The idea of this patch is correct. See
http://drupal.org/node/1354#classes
for standards.

But I don't know if the patch is correct. This is documentation in SelectQueryExtender, which is declared as:

class SelectQueryExtender implements SelectQueryInterface {

So how/why are methods here saying they are implementing methods from other interfaces besides SelectQueryInterface? Such as:

+  /**
+   * Implements QueryAlterableInterface::hasAnyTag().
+   */
   public function hasAnyTag() {

That can't really be right. If this class is implementing QueryAlterableInterface, then it should declare that. If not, the documentation is wrong.

chris.leversuch’s picture

If I've understood this correctly, SelectQueryExtender implements SelectQueryInterface which extends QueryAlterableInterface. QueryAlterableInterface is where hasAnyTag() is originally defined.

The current comments are just skipping SelectQueryInterface because it doesn't include its own implementations of the functions - technically SelectQueryExtender::hasAnyTag() is overriding QueryAlterableInterface::hasAnyTag(). I'd probably comment as 'Overrides SelectQueryInterface::hasAnyTag()' even though this doesn't actually exist because it could in the future - not sure if this would confuse api.d.o though.

jhodgdon’s picture

Status: Needs work » Closed (duplicate)

Actually, at this point this should be included in the big Docs cleanup instead of being its own issue.
#1317628: Clean up API docs for includes directory, files starting with N-Z

jhodgdon’s picture

Oh wait, this is part of the database one, not the includes N-Z:
#1316902: Clean up API docs for lib/Drupal/core/Database