First of a lot of style patches.
Bare me, this is just a test if I can handle this ;)

fixed some style issues, reported on the http://qa.drupal.org/head-style page

some points I need more help with

1)

  public function rewind() {
    // Nothing to do: our DatabaseStatement can't be rewound.
  }

This gives the next warning:

Use PHP's master function, not an alias. (List of PHP aliases)

I think this is a false positive, and that means I can't do anything about it

2)

Function summaries should be one line only. (Drupal Docs)

I fixed the style issues so that it will pass. But maybe these sentences aren't correct now.

Yes a summary can exceed 80 characters, but it need to be on one line,
you can add more info but you need to sepperate it with an empty line.

3)

global variables should start with a single underscore followed by the module and another underscore

function _db_check_install_needed() {
  global $databases;
  if (empty($databases) && !drupal_installation_attempted()) {
    include_once DRUPAL_ROOT . '/includes/install.inc';
    install_goto('install.php');
  }
}

problem with the $databases var, dunno how to fix this

I'll follow up this patch and can make follow up patches if I missed something

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

FileSize
6.13 KB

Damnit to much info in header and no patch atached

jhodgdon’s picture

Status: Needs review » Needs work

Looks mostly good. A few points:
a)

-   * @return array
+   * @return Array

Actually, we don't normally have either array or Array or anything else on the line with @return.

b)

 /**
- * Exception to throw when popTransaction() is called when no transaction is active.
+ * Exception to throw when popTransaction() is called when no transaction is
+ * active.
  */

The first line of any doxygen doc header must be 1 sentence of less than 80 characters, ending with a period. You need to rewrite it so it fits into 80 characters.

This one is also a problem:

 /**
- * Execute a query string against the active database and save the result set
- * to a temp table.
+ * Execute a query string against the active database and save the result set to a temp table.

Again, it has to be less than 80 characers.

c) This sort of thing appears in several places:

  * @code
- * class DatabaseStatement_oracle extends PDOStatement implements DatabaseStatementInterface {}
+ * class DatabaseStatement_oracle extends PDOStatement implements
+ * DatabaseStatementInterface {}
  * @endcode

I personally would not recommend wrapping this line inside a @code block in a doc header. If the Coder module is complaining about wrapping within @code, maybe Coder needs fixing, and maybe the style guidelines need fixing? That should be up for discussion, but probably on a separate issue.

d)

/**
- * Prints a themed maintenance page with the 'Site offline' text, adding the
- * provided error message in the case of 'display_errors' set to on. Ends the
- * page request; no return.
+ * Prints a themed maintenance page with the 'Site offline' text.
+ * 
+ * Adding the provided error message in the case of 'display_errors' set to on.
+ * Ends the page request; no return.
  */

That sentence starting with "Adding..." is not a sentence. Should start with "Adds" instead, and then it would be fine.

aspilicious’s picture

copy paste of what I said in irc so everybody can follow

[20:32] jghodgon
[20:32] you are wrong in several points
[20:33] "Yes a summary can exceed 80 characters, but it need to be on one line,"
[20:33] thats what drupal is saying
[20:33] first line can exceed 80 characters
[20:34] and "If a parameter or return value is an array, object, or interface, it is recommended to specify that data type in the @param or @return directive.:"
[20:34] ==> example: @param Array $options
[20:34] see the Array data type
[20:35] Found those rules due to this link you alrdy know: http://drupal.org/node/1354

So I think a is correct and second part of b also, don't know why I have to wrap the first part of b

jhodgdon’s picture

OK...
(a) is OK then. Sorry, I didn't know that had been added to the standards document, because it was just something someone decided to add rather than discussing it on an issue as they should have.

(b) is still wrong. The standard is that the first line should be 80 characters or less if possible. The standards page states that if it cannot possibly go longer than 80 characters, it still needs to be on one line and still needs to end with a . and still needs to have a blank line after it.
http://drupal.org/node/1354

/**
 * Summary here; one sentence on one line (even if it exceeds 80 chars).

(c) is not addressed on http://drupal.org/node/1354

(d) still needs to be fixed.

jhodgdon’s picture

Also, on that same standards page, in the section on documenting functions and methods:

The first line of the block should contain a brief description of what the function does, limited to 80 characters, and beginning with a verb in the form "Does such and such" (third person, as in "This function does such and such", rather than second person imperative "Do such and such"). A longer description with usage notes should follow after a blank line, if more explanation is needed. Each parameter should be listed with a @param directive, with a description indented on the following line. After all the parameters, a @return directive should be used to document the return value if there is one. There is a blank line between the @param and @return directives.

So yes, our standard is that it should be 80 characters or less.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
21.39 KB

Srry for the delay, another try!

My english isn't perfect so I can use a review :)

jhodgdon’s picture

Status: Needs review » Needs work

Most of this is pretty good... A few questions/comments:

a) I am not sure why you indented all of these:

  * @code
- * SELECT nid, title FROM {node} WHERE uid=:uid
+ *   SELECT nid, title FROM {node} WHERE uid=:uid;
  * @endcode

b) I think the blank lines between the explanation and the @code need to be removed in this section:

@@ -86,13 +86,13 @@
  * value, not the query itself. Thus, the following is incorrect:
  *
  * @code
- * SELECT nid, title FROM {node} WHERE title LIKE :title%
+ *   SELECT nid, title FROM {node} WHERE title LIKE :title%;
  * @endcode
  *
  * It should instead read:
  *
  * @code
- * SELECT nid, title FROM {node} WHERE title LIKE :title
+ *   SELECT nid, title FROM {node} WHERE title LIKE :title;
  * @endcode

c) This should be wrapped at close to 80 characters. Or if you think it should start a new paragraph (I don't think so, but maybe), you need to leave a blank line:

- * object-oriented API for defining a query structurally. For example, rather than
+ * object-oriented API for defining a query structurally.
+ * For example, rather than

Also, that last line should probably end in :

d)

-   * @return array
+   *
+   * @return Array

This should have been left as "array" and not "Array".

e)

 /**
- * Exception to throw when popTransaction() is called when no transaction is active.
+ * Exception to throw when popTransaction() is called when no transaction is
+ * active.
  */

The first line must be 1 sentence of less than 80 characters. How about:
"Exception for when popTransaction() is called with no active transaction."?
Except that it should start with a verb... well, we haven't actually defined our doc standards for classes yet, so let's leave that question for later.

f)

 /**
- * Execute an arbitrary query string against the active database, restricted to
- * a specified range.
+ * Execute an arbitrary query string against the active database.
+ * 

Should start with verb in 3rd person: "Executes" not "Execute".

Same applies to the next few functions.

g)

+ * Retrieve the name of the currently active database driver.
+ *
+ * For example: "mysql" or "pgsql".

This should probably be all one sentence, or at least one paragraph? That second line there is not even a sentence.

h)

 /**
- * This maps a generic data type in combination with its data size
- * to the engine-specific data type.
+ * This maps a generic data type.
+ * 
+ * The mapping happens in combination with its data size to the engine-specific
+ * data type.
  */

Leave out "this" at the beginning. Function description should start with a verb.

aspilicious’s picture

d) is correct explained that earlier in this topic

a), b), c), f), h) ok I'll fix that

Need help with g)

What about e) ?
- Wrap it like I did, or leave it as it was

jhodgdon’s picture

g suggestion:
Retrieve the name of the currently active database driver; for example, "mysql" or "pgsql".

e: Don't wrap. Cut down to 80 characters or less. My suggestion was:
Exception for when popTransaction() is called with no active transaction.

aspilicious’s picture

I'll finish this patch tomorow!

thnx jhodgon

aspilicious’s picture

FileSize
20.88 KB

Retrieve the name of the currently active database driver; for example, "mysql" or "pgsql".

is to long so I changed it to:

Retrieves the name of the currently active database driver.

aspilicious’s picture

Status: Needs work » Needs review
jhodgdon’s picture

There are still a few things that need fixing here:

a)

  * value, not the query itself. Thus, the following is incorrect:
  *
  * @code
- * SELECT nid, title FROM {node} WHERE title LIKE :title%
+ * SELECT nid, title FROM {node} WHERE title LIKE :title%;
  * @endcode
- *
  * It should instead read:
- *
  * @code
- * SELECT nid, title FROM {node} WHERE title LIKE :title
+ * SELECT nid, title FROM {node} WHERE title LIKE :title;
  * @endcode
  *
  * and the value for :title should include a % as appropriate. Again, note the

I think the blank lines at top and bottom should still be removed. This is all one paragraph, and a blank line in doxygen makes a new paragraph.

b)

   * @return SelectQueryInterface
    *   A new SelectQuery object.

Is it SelectQueryInterface or SelectQuery?

c) This one is also wrong:

   * @return TruncateQuery
    *   A new DeleteQuery object.

d)

+   * @return Array
    *   The query log for the specified logging key and connection.

Again, this should be a lower-case 'array', not upper-case 'Array'.

e) This is not something you introduced, but it sure doesn't look like _db_error_page() actually ends the page request? Wouldn't it need to call exit() or something to do that? Hmmm...

Actually, it looks like it used to do a bunch of stuff in D6 that it no longer does in D7:
http://api.drupal.org/api/function/_db_error_page/6
http://api.drupal.org/api/function/_db_error_page/7

So the doc should now just say for D7 that it sets up the title and theme for an error page. It doesn't end the page request, and doesn't print the errors.

How about:

/**
 * Initializes a themed site maintenance page.
 */
 function _db_error_page($error = '') {

Or better yet, just remove the function? I cannot see it being used anywhere in Drupal core (I grepped through the code for the string error_page, just in case someone was calling it by building up the function name from a string or something, and found nothing except this function...). Do we still need it?

jhodgdon’s picture

Status: Needs review » Needs work
aspilicious’s picture

FileSize
21.17 KB

D is correct!

EDIT: it WAS correct, they changed the doc style document... pfff
==> I fixed it like the doc style document says it has to be done

b) I chose SelectQuery (I'm not sure)

Here is the code:

  public function select($table, $alias = NULL, array $options = array()) {
    if (empty($this->selectClass)) {
      $this->selectClass = 'SelectQuery_' . $this->driver();
      if (!class_exists($this->selectClass)) {
        $this->selectClass = 'SelectQuery';
      }
    }
    $class = $this->selectClass;
    // new is documented as the highest precedence operator so this will
    // create a class named $class and pass the arguments into the constructor,
    // instead of calling a function named $class with the arguments listed and
    // then creating using the return value as the class name.
    return new $class($table, $alias, $this, $options);
  }

c) I fixed it

e) I just deleted the function for this patch

aspilicious’s picture

Status: Needs work » Needs review
Crell’s picture

The ->select() method will always return an object that conforms to the SelectQueryInterface interface. 99% of the time that is either SelectQuery or a subclass thereof (which has an identical interface). I can't actually think of a way you'd do otherwise without writing your own driver and doing all sorts of unnecessary things with the query builders.

I originally documented it to return SelectQuery in the prose because generally it does (I think at the time that docblock was written there were no select query subclasses yet), and that's where you'll go to find the implementation code. When we added return types, I went with SelectQueryInterface because that is, strictly speaking, the correct and code-enforced expectation.

I'm fine with changing the prose, although I think it reads a bit clunkier for it to say SelectQueryInterface.

jhodgdon’s picture

Then how about being a bit more verbose in all of those @return statements?

It just seems wrong to have

@return SelectQueryInterface
   A SelectQuery object.

It should probalby say more anyway.

Crell’s picture

I'm OK with that. Maybe something like:

An appropriate SelectQuery object for this database connection.  Note that it may be a driver-specific subclass of SelectQuery, depending on the driver.

And then a corresponding sentence for each of the factory methods on the connection object.

jhodgdon’s picture

That sounds reasonable to me.

By the way, that last patch has a slip of the finger in it:

 * value, not the query itself. Thus, the following is incorrect:
- *
+ 
  * @code

near the top.

aspilicious’s picture

"And then a corresponding sentence for each of the factory methods on the connection object."

Can you enter the full comment in here so I can patch it?

Status: Needs review » Needs work

The last submitted patch, database_inc_style_4.diff, failed testing.

jhodgdon’s picture

RE #21: I think that's actually a separate issue to add things to doc in other files, so I would leave it for now. If you want to look into it, you'd need to find the methods on the connection objects for the different databases that generate SelectQuery objects.

aspilicious’s picture

FileSize
21.32 KB

Maybe this one is good...
Maybe...

aspilicious’s picture

Status: Needs work » Needs review

status -> Needs review

aspilicious’s picture

This patch

- removes "function _db_error_page($error = '') {" as it is unused in core

- places newlines between doc blocks for example:

  * @param $table_expression
  *   An SQL expression, for example "simpletest%" (without the quotes).
  *   BEWARE: this is not prefixed, the caller should take care of that.
+ *
  * @return
  *   Array, both the keys and the values are the matching tables.

- makes the @code style consistent in the entire file (I think there need to be more info about intending code in the code style doc)
- shortens summaries to 80 characters
- removes newline at end of docblock
- renames a return value and adds more comment to another

That's it I guess

Dries’s picture

Last patch looks good, but doesn't seem to apply to me. Might need a quick reroll.

jhodgdon’s picture

#24: database_inc_style_5.diff queued for re-testing.

aspilicious’s picture

FileSize
21.3 KB

Indeed last part needed reroll, hopefully last one :)

jhodgdon’s picture

Status: Needs review » Needs work

A few things need to be fixed with this latest patch:
a)

+   * @return array $queries

We don't put a parameter name like $queries on a @return.

b)

+ * In the vast majority of cases, you should not instantiate this class
+ * directly. Instead, call ->startTransaction(), from the appropriate connection
+ * object.

In order to (hopefully, if the API project ever gets around to doing the Right Thing with classes and interfaces) make a link here, can you write something like this (putting in the correct interface or class name):
Instead, call WhateverTheConnectionClassIs::startTransaction().

c)

- * Execute an arbitrary query string against the active database, restricted to
- * a specified range.
+ * Executes an arbitrary query string against the active database.
+ * 
+ * The query string has to be restricted to a specified range.

This is the doc for db_query_range().
It's actually pretty important that the range stuff make it into the one-line summary, as this is the entire point of the function...
How about:
Executes a query against the active database, restricted to a range.

d) Grammar:

+ * Executes a query string and save the result set to a temp table.

Should be "saves" here, and if "temporary" will fit on the line, that should be used rather than "temp".

e) Needs more in the one-line description:

 /**
- * This maps a generic data type in combination with its data size
- * to the engine-specific data type.
+ * Maps a generic data type.
+ * 
+ * The mapping happens in combination with its data size to the engine-specific
+ * data type.
  */
 function db_type_map() {

What does "maps a generic data type" mean? Where's it mapping from and to?

aspilicious’s picture

a)

Of course, I'll delete it but can you add a section in the style document that is saying that?
It's prety standard but I was confused.

b)

I think I found the base class

/**
 * Base Database API class.
 *
 * This class provides a Drupal-specific extension of the PDO database
 * abstraction class in PHP. Every database driver implementation must provide a
 * concrete implementation of it to support special handling required by that
 * database.
 *
 * @link http://php.net/manual/en/book.pdo.php
 */
abstract class DatabaseConnection extends PDO {

So I'm gonna change ->startTransaction() into DatabaseConnection::startTransaction()

c) Your sentece is good, going to use it

d)

Whatabout this? I don't know if it's saying exactly the same thing...

/**
* Maps a generic data type and its data size to an engine-specific data type.
*/

jhodgdon’s picture

Regarding (a), we use $variable for parameters, but not for @return... Return values are not $variables, are they? None of the examples on the doxygen standards page have a variable... So I am not sure what should be added to the style guide.

Regarding (d)...

Hmmm...

I really have no idea what the return value of that function is from either the current or the proposed function header doc (or in general). It would be nice if the function doc header would explain what the function returns.

It should probably have @see WhateverTheSchemaInterfaceClassIs::getFieldTypeMap() in there somewhere, too?

aspilicious’s picture

a) then its just me being dumb for a second :) never mind then

d)

I agree it isn't clear.

So you think adding @see DatabaseSchema::getFieldTypeMap() will do the trick?

I am not entirly sure DatabaseSchema is the interface/class we are looking for.
So it would be nice if some people with better understanding of databse can confirm this.

Crell’s picture

I'm really not sure what the standard should be for referring to a method of an object. @see ClassName::method() is the most common I've seen, but can also be confused with a static method call. I guess it works for now, but I just want to be on record saying "Bah!" :-)

As for getFieldTypeMap(), I apologize for not getting around to rewriting that whole mess. Let's try the following as a docblock for DatabaseSchema::getFieldTypeMap():

/**
 * Returns a mapping of Drupal schema field names to DB-native field types.
 *
 * Because different field types do not map 1:1 between databases, Drupal 
 * has its own normalized field type names. This function returns a driver-specific
 * mapping table from Drupal names to the native names for each database.
 *
 * @return array
 *   An array of Schema API field types to driver-specific field types.
 */

And actually, now that I look at the code, db_type_map() should not exist. That data is only for internal use, and internal methods should use internal methods, not wrapper functions. So let's do this:

1) Add the docblock above to the DatabaseSchema::getFieldTypeMap() method.

2) In DatabaseSchema_mysql::processField(), replace the call to db_type_map() with $this->getFieldTypeMap(). It's probably my fault that wasn't done long ago already.

3) In the docblock at the top of includes/database/schema.inc, replace the reference to db_type_map() with one to DatabaseSchema::getFieldTypeMap().

4) Remove db_type_map() entirely as it has no purpose.

aspilicious’s picture

FileSize
23.66 KB

Thnx crell and jhodgon for clearing this out!
Without you two this issue would be dead and forgotten.
This patch is evolving in more then just a clean up...

I also found small style problems with the other database files (specially @link vs @see problems), but thats for an other issue.
I did everything commented above + changed two @links to an @see. Cause I think they were used wrongly. Jhodgon correct me if I'm wrong.

I don't think this is going to be the final patch ;)

aspilicious’s picture

Status: Needs work » Needs review
jhodgdon’s picture

aspilicoius: Thanks for all the patching and repatching!

Crell: Looks like ClassName::methodName() is pretty standard for PHP -- http://us3.php.net/manual/en/pdo.query.php

Latest patch:

- * Execute a query string against the active database and save the result set
- * to a temp table.
+ * Executes a query string and save the result set to a temp table.

save -> saves.

 /**
  * Rename a table.

Rename -> Renames

aspilicious’s picture

FileSize
35.37 KB

I managed to make a 1000+ line patch....
I found a lot of third person verb issues

jhodgdon’s picture

Wow... Of course by editing those lines you have opened up more issues...

  /**
-   * Prepare and return a SELECT query object with the specified ID.
+   * Prepares and return a SELECT query object with the specified ID.

Needs to be "prepares and returns". This occurs in the next several hunks of the patch as well.

   /**
-   * Add database connection info for a given key/target.
+   * Adds database connection info for a given key/target.

info -> information

+ * Executes a query string and saves the result set to a temp table.

temp -> temporary

   /**
-   * Get the query string of that statement.
+   * Gets the query string of that statement.

What does 'that' refer to? Maybe should be this?

   /**
-   * Return a single field out of the current
+   * Returns a single field out of the current
    *

Needs an end of sentence.

+ * Restricts a dynamic table, column or constraint name to safe characters.

Needs a comma before "or" to conform to our doc standards for English style/usage.

  /**
  * Helper function to get duration lag from variable and set the session
  * variable that contains the lag.

You didn't modify this function header, but it doesn't conform (should be 80 characters, and I'm not sure how to rewrite it for 80?), and verb not in 3rd person...

jhodgdon’s picture

Status: Needs review » Needs work
aspilicious’s picture

Status: Needs work » Needs review
FileSize
35.39 KB
/**
 * Helper function to get duration lag from variable and set the session
 * variable that contains the lag.
 */

Sounds awfull lol, someone willing to give it a try?
I fixed the other issues.

Get ready for number 9

Whooeps I'm reading the file again and I'm seeing even more issues...

  /**
   * Decreases the depth of transaction nesting, committing or rolling back if
   * necessary.
   *
   * If we pop off the last transaction layer, then we either commit or roll
   * back the transaction as necessary.  If no transaction is active, we throw
   * an exception.

Help me with the first sentence

  /**
   * Returns an entire result set as an associative array keyed by the named
   * field.

and another one

Crell’s picture

Don't worry about transaction-related docblocks for now. Those are getting rewritten anyway in another issue that is rewriting the transaction code. :-)

Does the 80 character limit on the first line actually affect processing, or is it just stylistic? I would much rather us allow 85 character first-line summaries than waste time trying to bend the grammar to fit within that arbitrary limit.

jhodgdon’s picture

Suggestions for these three spots:

1.
Sets a session variable specifying the lag time for ignoring a slave server.

2.
Decreases the depth of transaction nesting.

This function first attempts to decrease the number of layers of transaction nesting by one. If there was no active transaction, the function throws an exception. If this was the last transaction layer, the function either rolls back or commits the transaction, depending on whether the transaction was marked for rollback or not.

3.
Returns the result set as an associative array keyed by the given field.

Hmmm... This is the original database.inc for this method:

  /**
   * Returns an entire result set as an associative array keyed by the named
   * field.
   *
   * If the given key appears multiple times, later records will overwrite
   * earlier ones.
   *
   * Note that this method will run the result set to the end.
   *

I think that last sentence about "note that..." can be omitted, as I don't think it adds anything to the documentation.

aspilicious’s picture

FileSize
37.06 KB

We are getting there...

jhodgdon’s picture

RE #42: I am not sure about that -- I think it did once, but I'm not sure about now. Good question for the API project.

RE the latest patch: Still has

-   * Prepare a query string and return the prepared statement.
+   * Prepares a query string and return the prepared statement.

return -> returns -- see #39 comments.

I stopped there and didn't look at the rest...

aspilicious’s picture

FileSize
37.06 KB

Oeeps... CTRL-F missed something apparantly

jhodgdon’s picture

Status: Needs review » Needs work

This still isn't quite right:

    /**
-   * Return a single field out of the current
+   * Returns a single field out of the current.
    *
    * @param $index
    *   The numeric index of the field to return. Defaults to the first field.
+   *
    * @return
    *   A single field from the next record.
    */

This needs another word -- "the current" what? I guess it's the current record... But then the @return says it returns a single field from the "next" record. So which is it?

When this is fixed, I am good with RTBC for this patch (finally). It looks great. Thanks for all of your hard work!!!!

aspilicious’s picture

Database people have to say what it has to be cause it's an interface header so I can't see the implementation.
And I guess I have to change a test, where can I find the test that is failing?

jhodgdon’s picture

The failing test is "site-wide contact form". I don't think so... Probably just a testing glitch.

Regarding the DatabaseStatementInterface::fetchField method, it looks like there's a DatabaseStatementBase class that defines it as:

  public function fetchField($index = 0) {
    // Call PDOStatement::fetchColumn to fetch the field.
    return $this->fetchColumn($index);
  }

I checked the doc for PDOStatement::fetchColumn http://us2.php.net/manual/en/pdostatement.fetchcolumn.php

So I think the doc header for this one should say:

Returns a single field from the next record of a result set.

aspilicious’s picture

FileSize
37.08 KB

We will see what (maybe) someone else say but I think this sounds good.

aspilicious’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Yay! aspilicious deserves 1000+ thanks for all of this careful attention to detail, rerolls, etc.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

Transactions docs changes going in #669794: Use savepoints for nested transactions

sun’s picture

Status: Fixed » Needs review
FileSize
16.53 KB

Yay, nice patch :)

+++ includes/database/schema.inc	8 Mar 2010 22:51:10 -0000
@@ -266,9 +266,15 @@
   /**
-   * This maps a generic data type in combination with its data size
-   * to the engine-specific data type.
-   */
+  * Returns a mapping of Drupal schema field names to DB-native field types.
+  *

Unfortunately, a small hiccup here.

While being there, also fixed some other formatting issues.

Powered by Dreditor.

aspilicious’s picture

Looks good

jhodgdon’s picture

Status: Needs review » Needs work

Not quite:

+   * Returns a DatabaseSchema object for manipulating the schema of this database.

We never want doc lines to wrap past 80 characters, and this one does.

And

    * @param $a1
    *   An option depending of the fetch mode specified by $mode:
-   *    - for PDO::FETCH_COLUMN, it is the index of the column to fetch,
-   *    - for PDO::FETCH_CLASS, it is the name of the class to create, and
-   *    - for PDO::FETCH_INTO, it is the object to add the data to.
+   *   - for PDO::FETCH_COLUMN, it is the index of the column to fetch,
+   *   - for PDO::FETCH_CLASS, it is the name of the class to create, and
+   *   - for PDO::FETCH_INTO, it is the object to add the data to.

This doesn't follow our list standards, really.

   * @param $a2
+   *   In case of when mode is PDO::FETCH_CLASS, the optional arguments to
+   *   pass to the constructor.

Bad grammar/style.

And does the API module support @throws currently? If not, we need to file an issue on the API project.

sun’s picture

Status: Needs work » Needs review
FileSize
16.48 KB

1) Fixed. However, see #502190-9: Hook implementation headers out of compliance with standards

2) Not invented here. Note that this does not describe array keys. Further, you noticed that those lines belong to a method that is commented out?

3) Basically see 2).

4) AFAIK, @throws is not supported yet. But there's a complete rewrite of API module on the way, if I'm not mistaken.

Dries’s picture

Status: Needs review » Fixed

Committed the patch in #58 but we can definitely work on follow-up patches for items (2) and (3). Marking as 'fixed' for now.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.