Coding standards: PHP line length / wrapping

Zend standard seems to have the most reasonable and thought out description. I like that it has a suggestion and a max but that they're somewhat far apart. I think that I can keep everything within 120, but 80 is sometimes a stretch for me when working on form code for instance.

Also, this would be easy to implement in Coder so I think that should get some bonus points.

We normally discuss coding standards in the Drupal Core issue queue

Some core developers and also some comments on http://drupal.org/coding-standards#comment-3793566 keep on asking for clarifications on the line length. Especially with regard to control structures (long, compound if conditions).

I'm personally not sure why, as it's mainly a matter of readability, sanity, and human sense attached to a common rule of wrapping at 80 chars, but it looks like we have to setup hard rules here, as I'm sick of repeating the same comments all over again.

So here's a concrete proposal:

1. In general, all lines of code should not be longer than 80 chars.
2. Lines containing longer function names, variable declarations, aso. are allowed to exceed 80 chars.
3. Control structure conditions may exceed 80 chars, if they are simple to read and understand:
   

     if ($something['with']['something']['else']['in']['here'] == mymodule_check_something($whatever['else'])) {
       ...
     }
     if (isset($something['what']['ever']) && $something['what']['ever'] > $infinite && user_access('galaxy')) {
       ...
     }
     // Non-obvious conditions of low complexity are also acceptable, but should
     // always be documented and explain WHY a particular check is performed.
     if (preg_match('@(/|\\)(\.\.|~)@', $target) && strpos($target_dir, $repository) !== 0) {
       return FALSE;
     }
   

4. Wrapping control structure conditions into multiple lines is discouraged. Furthermore, control structure conditions should also NOT attempt to win the Most Compact Condition In Least Lines Of Code Award™:
   

     if ((isset($key) && !empty($user->uid) && $key == $user->uid) || (isset($user->cache) ? $user->cache : '') == ip_address() || isset($value) && $value >= time())) {
       ...
     }
   

   It is recommended practice to split out and prepare the conditions separately instead, which additionally allows to document underlying reasons for the conditions performed:

     // Key is only valid if it matches the current user's ID, as otherwise other
     // users could access any user's things.
     $is_valid_user = (isset($key) && !empty($user->uid) && $key == $user->uid);
   
     // Note: The following is still a bit dense. Always consider and decide on
     // your own whether people unfamiliar with your code will be able to make
     // sense of the logic.
     // IP must match the cache to prevent session spoofing.
     $is_valid_cache = (isset($user->cache) ? $user->cache == ip_address() : FALSE);
     // Alternatively, if time is [...]
     $is_valid_cache = $is_valid_cache || (isset($value) && $value >= time());
   
     if ($is_valid_user || $is_valid_cache) {
       ...
     }
   

5. For precise rules on documentation and comments, see Doxygen and comment formatting conventions.

Subscribe. As the author of some of the non-obvious and syntactically incorrect code in #3, I need all the help I can get.

I do find it confusing that we absolutely forbid the use of is_null() because isset() is 5% faster, yet we also absolutely forbid multi-line conditionals in favor of (somewhat slower) multiply-nested conditionals.

I'm looking forward to the rules being written down where I can see them instead of being carried around in the heads of senior developers where I can't.

subscribing

I'm happy with the coding standards proposal in #3. Regarding #5 - please file a separate issue. This issue is only about line length.

Style/proofread of #3:
- Please use etc. not aso (which I think means and so on?). That's not a common abbreviation.
- Generally I was not clear at first glance, especially in item 4, which were supposed to be good examples and which were supposed to be bad examples. It would be good to label them as such, probably with a code comment like this in the code block:
// DO NOT DO THIS!
- "which additionally allows to document underlying reasons " ==> "which additionally permits documenting the underlying reasons" ... and I think I would leave out the word "performed" at the end of that phrase.
- This comment in your code sample violates our commenting standards (complete sentence, ends with period):
// Alternatively, if time is [...]
- Maybe at the very start, say that these rules apply to code and not comments, and put the link to the doxygen/comments page there?

Thanks for reviewing, @jhodgdon! So how about this?

Line length and wrapping

The following rules apply to code. See Doxygen and comment formatting conventions for rules pertaining to comments.

• In general, all lines of code should not be longer than 80 chars.
• Lines containing longer function names, function/class definitions, variable declarations, etc are allowed to exceed 80 chars.
• Control structure conditions may exceed 80 chars, if they are simple to read and understand:
  

    if ($something['with']['something']['else']['in']['here'] == mymodule_check_something($whatever['else'])) {
      ...
    }
    if (isset($something['what']['ever']) && $something['what']['ever'] > $infinite && user_access('galaxy')) {
      ...
    }
    // Non-obvious conditions of low complexity are also acceptable, but should
    // always be documented, explaining WHY a particular check is done.
    if (preg_match('@(/|\\)(\.\.|~)@', $target) && strpos($target_dir, $repository) !== 0) {
      return FALSE;
    }
  

• Wrapping control structure conditions into multiple lines is discouraged. Control structure conditions should also NOT attempt to win the Most Compact Condition In Least Lines Of Code Award™:
  

    // DON'T DO THIS!
    if ((isset($key) && !empty($user->uid) && $key == $user->uid) || (isset($user->cache) ? $user->cache : '') == ip_address() || isset($value) && $value >= time())) {
      ...
    }
  

  Instead, it is recommended practice to split out and prepare the conditions separately, which also permits documenting the underlying reasons for the conditions:

    // Key is only valid if it matches the current user's ID, as otherwise other
    // users could access any user's things.
    $is_valid_user = (isset($key) && !empty($user->uid) && $key == $user->uid);
  
    // NOTE: The following is still a bit dense. Always consider and decide on
    // your own whether people unfamiliar with your code will be able to make
    // sense of the logic.
    // IP must match the cache to prevent session spoofing.
    $is_valid_query = (isset($user->cache) ? $user->cache == ip_address() : FALSE);
    // Alternatively, if the request query parameter is in the future, then it
    // is always valid, because the galaxy will implode and collapse anyway.
    $is_valid_query = $is_valid_cache || (isset($value) && $value >= time());
  
    if ($is_valid_user || $is_valid_query) {
      ...
    }
  

1 vote for #8.

Moving to the docs queue. There's nothing for me to commit here. :D

We normally discuss coding standards in the drupal core issue queue, actually, since the key developers do not read the doc issue queue. Changing status so committers don't think there's something to commit...

I alrdy do this so I agree with everything in 8.

I agree with @jhodgdon. In fact, updates to coding standards rules need to be known by core maintainers. Otherwise, there's no way to enforce them throughout Drupal, since not even sloppy core may not follow them.

So ideally, I'd also like a confirmation from Dries that he has read this issue. Pinged him.

In this case, I can only hope that 99.9% of Drupal core code is following the rules already. After updating the coding standards page, the next step would be to check whether Coder is able to identify violations of these rules (fun! - probably not...)

Meanwhile, I'll update the coding standards page.

http://drupal.org/coding-standards#linelength

Sure, but if it's RTBC, no more need to discuss here, right?

Oops sorry. I just re-read #14. Does make sense for core maintainers to know coding standards changes, that's true.