Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nicl’s picture

Assigned: nicl » Unassigned

I haven't had time for this in the end so if anyone else feels like having a look feel free.

xjm’s picture

Assigned: Unassigned » nicl
xjm’s picture

xjm’s picture

Assigned: nicl » Unassigned

Apparently you can ninja-assign an issue with a crosspost. ;)

chris.leversuch’s picture

Assigned: Unassigned » chris.leversuch

I'll try this one.

jhodgdon’s picture

This issue has a start of a patch for select.inc (and I've marked it as a duplicate):
#1255524: select.inc needs more doxygen

Crell’s picture

Eep!

Um, not to dissuade anyone, but please see: #1321540: Convert DBTNG to namespaces; separate Drupal bits.

I'm moving around ALL of the code in the /database directory. That's absolutely going to collide with anything here, and make one or both of us manually reroll a ton of code. Let's please not do that. :-(

Can we mark this postponed on that issue?

xjm’s picture

Yeah, having read and tickled the 600k monster that is Crell's patch in #1321540: Convert DBTNG to namespaces; separate Drupal bits, I'd definitely suggest postponing this particular cleanup until after that issue goes in.

chris.leversuch’s picture

Status: Active » Postponed

Luckily I haven't had much time for this patch so I haven't done too much. I'll stop until the other patch is in.

jhodgdon’s picture

Issue tags: +Novice

tagging

NROTC_Webmaster’s picture

Status: Postponed » Active

Changing this to active since #1321540: Convert DBTNG to namespaces; separate Drupal bits is closed.

jhodgdon’s picture

Assigned: chris.leversuch » Unassigned

I'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!

TravisCarden’s picture

The core/includes/database directory no longer exists in d8. Does anything need to be done in this issue before closing it and removing it from the meta issue?

Crell’s picture

Title: Clean up API docs for includes/database » Clean up API docs for lib/Core/Database

Retitling. :-)

larowlan’s picture

Assigned: Unassigned » larowlan
larowlan’s picture

Title: Clean up API docs for lib/Core/Database » Clean up API docs for lib/Drupal/core/Database/Query/Select
Status: Active » Needs review
FileSize
14.71 KB

This is cleanup of select class.
Since all of the database classes have since been split into separate files in line with PSR-0, I propose we add follow-up tasks for the other classes.

jhodgdon’s picture

Regarding follow-up tasks, that is not how we have been doing this API cleanup effort -- and we really don't want to have a separate issue for every file in Drupal Core to clean it up (pretty much every file in Drupal Core needed some cleanup!). That would be very tedious. So what we've been doing is one issue per module, one per theme, and then we split up the other parts of core to similar-sized groups of files.

So: we really do want one patch that cleans up all the database stuff, eventually.

I took a quick look at this patch, and it looks like you're not yet familiar with our documentation standards:
http://drupal.org/node/1354#functions
Check out the formatting we require for function/method docs and try again. :)

You should also read the meta-issue this is a part of:
#1310084: [meta] API documentation cleanup sprint
to find out what specifically we are cleaning up, before tackling any other classes in this issue.

Thanks!

jhodgdon’s picture

Title: Clean up API docs for lib/Drupal/core/Database/Query/Select » Clean up API docs for lib/Drupal/core/Database
Status: Needs review » Needs work

Forgot status change, and changing title back to what this issue is supposed to encompass

larowlan’s picture

Most of the methods in the patch have been documented in their respective defining interface. Should I be duplicating these docblocks? Seems like overkill.

jhodgdon’s picture

No. You did fine on the "Implements name/of/interface" doc blocks, aside from a lot of them missing . at the end. Those are mostly like:
http://drupal.org/node/1354#hookimpl
I thought we had a standard written down somewhere for the class overrides/implements too, not sure where that is... but blocks like this are fine:

+  /**
+   * Implements Drupal\Core\Database\Query\AlterableInterface::addTag().
+   */
   public function addTag($tag) {
larowlan’s picture

Status: Needs work » Needs review
FileSize
20.25 KB
26.08 KB

Cleans up Query/Select.php and Query/SelectExtender.php
What's next?

jhodgdon’s picture

Status: Needs review » Needs work

What's next? This issue covers all of the directory core/lib/Drupal/Core/Database. Check that directory and see what else needs to be done.

But first, let's get this much right. A few things to fix:

a)

+  /**
+   * Constructor
+   *
+   * Creates a new select query.
+   *

See http://drupal.org/node/1354#functions -- read those guidelines and follow them.

b)

+    /**
+     * Implements Drupal\Core\Database\Query\AlterableInterface::addTag().
+     */
   public function addTag($tag) {

Fix indentation -- docs comment should be indented same as "public" (2 spaces). I think the rest of the methods in this file are fine. Just do this one the same.

c)

+  /**
+   * Implements Drupal\Core\Database\Query\ConditionInterface::condition() for
+   * the HAVING clause.
+   */
   public function havingCondition($field, $value = NULL, $operator = NULL) {

Needs a one-line summary... but this isn't accurate anyway. You can only use the "Implements..." wording if something is an implementation of an interface method, and it looks like this isn't one. This applies to quite a few more methods in the patch too.

d) There are several other methods in the patch, that, like (a), need to be fixed up to follow our function guidelines, such as:

+  /**
+   * Return the query as a string.
+   *
+   * @return string
+   *  The compiled query
+   */
   public function __toString() {

+  /**
+   * Clone the Select query object.
+   *
+   * Clones the query being sure to clone all object properties as well.
+   */
   public function __clone() {
Albert Volkman’s picture

Re-roll of #19 against current head. Still needs fixes from comments in #22.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
955 bytes
26.01 KB

This patch covers a & b from #22.

Crell’s picture

It looks like we'll be dropping the "Implements Foo\Bar\Baz" style comments shortly: #1906474: [policy adopted] Stop documenting methods with "Overrides …"

I know, the timing is terrible. :-(

Albert Volkman’s picture

C'est la vie. Here we go for the majority of the comments. However, I'm not sure about what to do about the 'for the HAVING clause' lines. I don't think those would be able to figure out their proper parents (havingConditions vs conditions methods). Also, for-

Implements Drupal\Core\Database\Query\SelectInterface::join().

for method 'innerJoin'. What should be done there?

larowlan’s picture

Assigned: larowlan » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

The param lines in the Select::__construct method do not have . at the end of their descriptions.

+   * @param string $table
+   *   The base table for the select query

etc.

Also $options is *connection* options, not query options.

So... Regarding all the inheritdoc lines... You can only use inheritdoc if the class actually is implementing or overriding methods on an interface or class that it inherits from.

In this case, Select inherits (directly or indirectly) from:
Query
SelectInterface
ConditionInterface
AlterableInterface
ExtendableInterface
PlaceholderInterface
and maybe some more that those inherit from. So all of the @inheritdoc things are probably fine, and the lines like

  /**
+   * Implements Drupal\Core\Database\Query\ConditionInterface::condition() for
+   * the HAVING clause.
+   */
   public function havingCondition($field, $value = NULL, $operator = NULL) {

should just be using inheritdoc as well, since those are methods on one of the interfaces. Right?

Albert Volkman’s picture

I don't believe these docs are correct. Those methods do not exist in the interface-

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

jhodgdon’s picture

Check the interfaces more carefully -- this class inherits a LOT of interfaces (some via the base class). I think inheritdoc is right.

Albert Volkman’s picture

Ah, I see that now. Thanks for pointing it out @jhodgdon! I concur that inheritdoc is correct.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!