I've heard various rumblings in #drupal from people who don't like the current DBTNG code style for one reason or another. Since PDO doesn't have coding standards, we've had to make up our own. Complaints range from it being inconsistent (which wouldn't shock me, since it was decided sort of piecemeal between random conversations between Larry and myself) to not being easily formatted in tools like Coder Format and emacs (which is a big problem) to just being kind of "icky." Not having this nailed down is actively blocking issues such as porting core files to DBTNG.
So let's have it out! Here's the current syntax. Can you do better? And why?
<?php
$result = db_query("SELECT name FROM {test} WHERE age = :age", array(':age' => 25));
db_insert('test_task')
->fields(array('pid', 'task', 'priority'))
->values(array(
'pid' => $john,
'task' => 'eat',
'priority' => 3,
))
->values(array(
'pid' => $john,
'task' => 'sleep',
'priority' => 4,
))
->execute();
?>Bear in mind that whatever is decided here:
a) Needs to be documented in the Coding standards
b) For whatever we change, we need a patch to change it everywhere.
c) Above is much easier if we can get a Coder Format module working under D7 that can script the change.
So we need volunteers for those three things in order to proceed.
Comments
After I've figured out how
After I've figured out how to confiure emacs for this style, I am ok with it. However, there's an inconsistency as far as we always have a dangling closing bracket for FAPI arrays and here the bracket is together with the closing bracket for fields() or values(),
((X)Emacs config for this is C-c C-o arglist-cont-nonempty RET c-lineup-math RET)
I don't like the big gap at
I don't like the big gap at the bottom which often appears with the current syntax - looks like a syntax error usually, which isn't good. So I /think/ this would be easier on the eye in terms of easily scanning where things start and end.
<?phpif ($something) {
$result = db_query("SELECT name FROM {test} WHERE age = :age", array(':age' => 25));
db_insert('test_task')
->fields(array('pid', 'task', 'priority'))
->values(array(
'pid' => $john,
'task' => 'eat'#
'priority' => 3,
))
->values(array(
'pid' => $john,
'task' => 'sleep',
'priority' => 4,
))
->execute();
}
?>
With every other core code style, the text is indented, not an angle bracket, parenthesis or whatever, so having the pointy arrow indented here looks like double indentation to me.
Parenthesis indentation
@catch I assume you meant to have the first
))back two spaces:<?php->values(array(
'pid' => $john,
'task' => 'eat'#
'priority' => 3,
))
->values(array(
'pid' => $john,
'task' => 'sleep',
'priority' => 4,
))
?>
I just want to be clear on your syntax. :)
Senior Drupal Developer for Lullabot | www.davereid.net | @davereid
yep
Yeah that's what I meant - I've updated the original comment. Also added an if statement in there - since that's the sort of context in which I find the current syntax hard to read with HEAD - so here's what I don't like about it.
<?php
if ($something) {
$result = db_query("SELECT name FROM {test} WHERE age = :age", array(':age' => 25));
db_insert('test_task')
->fields(array('pid', 'task', 'priority'))
->values(array(
'pid' => $john,
'task' => 'eat'#
'priority' => 3,
))
->values(array(
'pid' => $john,
'task' => 'sleep',
'priority' => 4,
))
->execute();
// Big ugly confusing gap that makes it look like there's an unclosed angle bracket.
}
?>
Chained methods
Whatever we come up here must apply to all chained methods throughout Drupal. DBTNG is just the trigger.
For the coder_format script, the first attempt should probably be to try to detect chained methods, and apply a special newline/indentation for
->tokens until a closing;appears, i.e.<?phpregular_function_in_reality($regular_formattings_here)
->chained_method($regular_arguments_and_formattings_here)
->chained_method2();
// We hit the special ; token, proceed with regular (un-chained) formatting if we get f.e. $node->nid.
?>
Daniel F. Kudwien
unleashed mind
Daniel F. Kudwien
netzstrategen
The value of chainable
The value of chainable methods is lost, in my opinion, when you have huge multi-line statements. Just as is the same when you have huge if/else statements. It begins to get very confusing and hard to follow by matching up indents and functions.Angie's example is simple, but imagine it as it gets to a much more complex statement.
I realize this post is for a coding standard and API may be out of its scope, but API largely affects your options in coding standard. With that in mind, I have listed an examples of some of my thoughts. Some of them may not be possible with the current API or they may be worth making a small addition to the API like allowing arrays of data.
Using the current example:
Example #1 restyled with no API changes from Angie's example:
<?php$sql = "SELECT name
FROM {test}
WHERE age = :age";
$result = db_query($sql, array(':age' => 25));
$fields = array(
'pid',
'task',
'priority',
);
$values = array(
array(
'pid' => $john,
'task' => 'eat',
'priority' => 3,
),
array(
'pid' => $john,
'task' => 'sleep',
'priority' => 4,
),
);
db_insert('test_task')->fields($fields)->values($values[0])->values($values[1])->execute();
?>
The DBTNG docs give the below as an example for inserting multiple values.
Example #2 from docs:
<?php$values = array(
array(
'title' => 'Example',
'uid' => 1,
'created' => REQUEST_TIME,
),
array(
'title' => 'Example 2',
'uid' => 1,
'created' => REQUEST_TIME,
),
array(
'title' => 'Example 3',
'uid' => 2,
'created' => REQUEST_TIME,
),
);
$query = db_insert('node')->fields(array('title', 'uid', 'created'));
foreach ($values as $record) {
$query->values($record);
}
$query->execute();
?>
Running a foreach loop when you have the power of chainable methods is also a bit cumbersome. I would be better to simple allow multi-dim arrays to be used. In this manner we would get rid of one of the ->values calls or the need for a foreach loop.
Example #3 using data context from example #1 with a mult-dim array for values:
<?phpdb_insert('test_task')->fields($fields)->values($values)->execute();
?>
The docs also give a more compact example of using fields and values:
Example #4 from docs:
<?php$nid = db_insert('node')
->fields(array(
'title' => 'Example',
'uid' => 1,
'created' => $_SERVER['REQUEST_TIME'],
))
->execute();
?>
If we could combine the power of using this compact form with multi-dims we would get a much better chain.
Example #5 using data context from example #1
<?phpdb_insert('test_task')->fields($values)->execute();
?>
#2 is not a good example here
Example #2 from the DBTNG docs is deliberately contrived. Normally you shouldn't build the array just to iterate it like that. Presumably you'd be doing some sort of processing, such as iterating over the returned values from hook_menu(), processing them, and queuing each of those up in ->values() inside the loop. We may want a more demonstrative example in the handbook docs to help clarify that. (Suggestions welcome on what would be a better example.)
Logic of the current format
The current format for fluent methods was developed to mimic what I have seen/used for heavily-fluent jQuery chains. That is the existing fluent system that is most likely to be familiar to our current developer base:
$('.foo').thing1()
.css({
color: 'green',
size: '14pt'
})
.bind('click', function () {
// Do stuff here.
})
.thing2()
.thing3().thing4().end()
.thing5();
Using the same style for PHP fluent code made the most sense to me, and still makes the most sense.
Agreed
I agree, it makes sense to use a similar format for jQuery and PHP chaining. Indenting subsequent lines of a chain makes the code much more readable.
Generally jQuery developers are flexible with this structure. Up to three or so simple chained operations might be done on a single line. Multiple lines are used when the number or complexity of operations calls for them.
Yes, we end up with a four-space indent, but this should be no more confusing than what we have with e.g. a switch structure:
<?phpswitch ($variable) {
case 'first':
do_something();
break;
default:
do_nothing();
// Four-space indent.
}
?>
Probably confusing the first time or two you come across it, but easily learned.
+1 indentation
I'm in favour of the indentation. I see catch's point, but we're looking at the code in isolation. I don't think the gap would be an issue if we had surrounding code. Though maybe that's because I'm familiar with jquery.
<?php$result = db_query("SELECT name FROM {test} WHERE age = :age", array(':age' => 25));
db_insert('test_task')
->fields(array('pid', 'task', 'priority'))
->values(array(
'pid' => $john,
'task' => 'eat',
'priority' => 3,
))
->values(array(
'pid' => $john,
'task' => 'sleep',
'priority' => 4,
))
->execute();
drupal_set_message($message);
?>
Other codebases
I like to look what what others are doing for consistency and to lower the barrier to entry for those coming from other places. So, here are some observations and standards:
jQuery
jQuery doesn't have a code standard that I could easily find. But, as Crell pointed out above there is something we can observe in style. Take this bit from the jQuery UI datepicker:
divSpan.addClass(this.markerClassName).append(inst.dpDiv).bind("setData.datepicker", function(event, key, value){
inst.settings[key] = value;
}).bind("getData.datepicker", function(event, key){
return this._get(inst, key);
});
This use of chaining and indenting is common in jQuery.
Zend Framework
This example from Zend_Form displays their coding style:
<?php$username->addValidator('alnum')
->addValidator('regex', false, array('/^[a-z]/'))
->setRequired(true)
->addFilter('StringToLower');
?>
You can read their coding standard if you want more detail. Though I could not find any guidelines on chain style it is consistently like this example.
Joomla!
Joomla uses the PEAR standard, like drupal. The PEAR standard doesn't talk about or give examples of long chains. I couldn't find an example in Joomla. Instead of chaining they seem to do things like:
<?php
// Push the model into the view (as default)
$view->setModel($model, true);
// Set the layout
$view->setLayout('form');
// Display the view
$view->display();
?>
If someone know a better example please share.
Doctrine
Looking at an example from the Doctrine project we can see they do chains like:
<?php$user = Doctrine_Query::create()
->from('User u')
->leftJoin('u.Email e')
->leftJoin('u.Phonenumber p')
->leftJoin('u.Group g')
->execute();
?>
Digging around I couldn't find any coding standard that touched on this use case.
Symfony
I poked around the symfony project as well. In their examples I found cases like:
<?php$c = new Criteria();
$c->add(CommentPeer::AUTHOR, 'Steve');
$c->addJoin(CommentPeer::ARTICLE_ID, ArticlePeer::ID);
$c->add(ArticlePeer::CONTENT, '%enjoy%', Criteria::LIKE);
$c->addAscendingOrderByColumn(CommentPeer::CREATED_AT);
$comments = CommentPeer::doSelect($c);
?>
This is more like Joomla!. But, digging through their source code I found examples of multi-line chains like:
<?php$query = Doctrine_Query::create()
->from('Author a')
->where('a.active = ?', true);
?>
If we are going for consistency and a low barrier to entry for outside developers an indentation style like that in webchicks original post looks to be the best option.
Matt Farina
www.innovatingtomorrow.net
www.geeksandgod.com
www.superaveragepodcast.com
www.mattfarina.com
more examples
Some more examples I've seen in the wild, also including new things, like http://drupal.org/node/425872.
<?php
// 1. Static query with one param.
$result = db_query("SELECT name FROM {test} WHERE age = :age", array(':age' => 25));
// 2. Static query with multiple parameters and fetchField()
$name = db_query("SELECT name FROM {test} WHERE age = :age AND color = :color", array(
':age' => 25,
':color' => 'blue',
))
->fetchField();
// 3. Long static query (note that there is only one entry per array, in contrast to above example.
$result = db_query("SELECT mlid FROM {menu_links} WHERE link_path = :path AND module = 'menu'", array(':path' => 'node/'. $node->nid), array('fetch' => PDO::FETCH_ASSOC));
// 4. db_select() with extenders and joins
$query = db_select('accesslog', 'a')->extend('PagerDefault')->extend('TableSort');
$query->join('users', 'u', 'a.uid = u.uid');
$result = $query
->fields('a', array('aid', 'timestamp', 'path', 'title', 'uid'))
->fields('u', array('name'))
->limit(30)
->setHeader($header)
->execute();
// 5. Single db_insert()
db_insert('test_task')
->fields(array(
'pid' => $john,
'task' => 'eat',
'priority' => 3,
))
->execute();
// 6. db_insert with multiple Values.
$query = db_insert('node_access')->fields(array('nid', 'realm', 'gid', 'grant_view', 'grant_update', 'grant_delete'));
foreach ($grants as $grant) {
$query->values($grant);
}
$query->execute();
// 7. db_insert() with a single value
db_insert('test_task')
->fields(array('pid' => $join))
->execute();
?>
( I hope formatting will be correct and I may have made mistakes..)
Based on that, I do have a few questions..
a) Placement of the fetchField() in example 2. should that be on a new line or not and how should it be indented (that word is killing me...)?
b) single or double quotes for queries? If single quotes, we need to convert at least all static strings to params, see also http://drupal.org/node/394524 where DamZ already did that for SimpleTest
c) example 3 is correctly formatted, but it's still a huge line, any idea if and how we could improve that?
example 2
Dries specifically asked me to change the position of the ->fetchField() to this in the path alias cache patch:
<?php$name = db_query("SELECT name FROM {test} WHERE age = :age AND color = :color", array(
':age' => 25,
':color' => 'blue',
))->fetchField();
?>
I said he'll have to face you :p
Re: example 2
The syntax looks alright for one attribute, it becomes ugly for two or more. What standard do we have for array() when only one item is given? We should follow the same rules.
That's fine with me..
As I said in IRC already, changing that is fine with me and makes sense as it would be on the same line if the array would only contain a single value.
Unfortunatly, I can't edit my own comment, but question a) is resolved then.
Newline Semicolon as a "closing bracket".
I know this is all too late now, but:
I like to use semicolon on a new line, to visually go back one indentation level.
<?phpif (TRUE) {
db_insert('test_task')
->fields(array('pid' => $join))
->execute()
;
}
?>
Same in js
$('.foo').thing2()
.thing3().thing4().end()
.thing5()
;
$('.bar')
.css(..)
.hide()
;
Advantages:
- Lines can be moved around, without having to consider that one of them has a semicolon (for the same reason we put a comma on all array values, including the last, in a multiple-line php array definition).
- Semicolon looks like a "closing bracket".
- We avoid indentation looking wrong.
- It's harder to forget the semicolon.
Whatever, too late now.