Clarify scope of PHP coding standards for core (major version) and contrib code

First of all, thanks to @sun for posting this issue!

I would like to see the following clarification:

Every part of Drupal core, regardless of version, is required to comply with the
coding standard in force when that part was written. Failure to do so is a bug.

To avoid scope creep, context lines not modified by a patch should not be
required to meet the current standard, as long as they were in compliance when
originally written.

When a patch is written to bring old code into compliance with a particular
standards change, it should limit changes to that purpose for easier
reviewing.  Unrelated changes should be committed separately.

I have had patches marked "needs work" by one reviewer because lines included in context were non-compliant. Then when I expanded the patch scope to modify the additional lines, another reviewer marked it "needs work" because the patch scope had expanded beyond the original problem statement. Having a clear standard would eliminate such frustration.

NOTE: Crossposted the issue summary to Coding Standards and Documentation groups.

What if I replaced "part" with "line" to indicate that it's a line-by-line distinction, rather than a word-by-word distinction?

My personal feeling is that coding standards for patches should be based on whatever the code base they're patching currently is at. Even if that code is using tabs instead of 2 spaces, camelCasedFunctionNames(), no(spaces between control structures){, and whatever other nonsense that people shouldn't be doing (according to the recommended coding standards), but still do.

Why? Because the entire point of coding standards is consistency in the code. You're doing people absolutely no favours if the entirety of a given file is indented with tabs and your little four-line patch indents with spaces. While your patch is technically correct, it introduces inconsistency with the rest of the code base, and should be rejected on those grounds. If however, in a separate patch, the entire code base you're working on is brought into line with the community's coding standards that says spaces instead of tabs, then your patch that indents with spaces instead of tabs could get accepted. Otherwise, you need to suck it up and use tabs, regardless of how "wrong" it is.

Developers, by and large, don't read documentation. By and large, they look for and then copy/paste examples that are pretty close to what they need, then modify as required. We have a motto that goes "When in doubt, do as core does." Developers look to Drupal core when there's ambiguity around how to do something and copy/paste whatever examples from that code base. This is why it's critical that within a stable release of Drupal core, we do not make radical changes to the coding standards, such as concatenation operators or requiring data types before @param/@return elements. Otherwise, the answer "What does core do?" changes on a day-to-day basis and can no longer be a trusted source of information.

So, for wide-scale coding standards changes, such as the concatenation spacing in D6, these would only apply to new code (i.e. D7), and the existing, stable code would be left untouched. Contrib has more options for what "new" code means, however, and so tools like Coder should be adjusted to accept either the old default or the new recommendation in D6, and only the new recommendation in D7.

At least that's how we've always done it, and unless there's a hugely compelling reason not to do that, I don't know why we'd want to switch it up. This gives the advantages of:

- Each stable version of core is a predictable source of what the correct standards are to use for that version.
- Contrib is able to use whichever standard they prefer: the one in place when the module was coded, or the latest recommended standard if they want to get a jump on Drupal N+1.
- The coding standard used in a given patch always conforms to the surrounding code, and doesn't introduce any inconsistencies which, even if they may be "correct" for that particular time and place, make the code harder to read and understand.

@#4 -- What about this issue where I'm clearly being asked by the Documentation co-maintainer to patch one function up so that it is inconsistent with the rest of the file?

That's a D8-only patch that's updating a single function to the proper coding standards, as part of a larger movement to update all of the code to coding standards. That's different than one-off patches fixing actual bugs including mis-matching coding standards which sun demonstrates in #2.

The coding standard used in a given patch always conforms to the surrounding code, and doesn't introduce any inconsistencies which, even if they may be "correct" for that particular time and place, make the code harder to read and understand.

So I take it that by "surrounding code" you mean "in the same line or the same function" but not "in the same file". Am I correct, or have I still misunderstood you?

Isn't that just semantics...?

If 99% of the code in D7 (the stable release of core) looks like this:

/**
 * Flargen hargen slurgen glurg.
 *
 * @param $foo
 *   Foos a foo.
 * @return
 *   Blah.
 */

...then a patch comes along that adds one new function formatted like the following:

/**
 * Flargen hargen slurgen glurg.
 *
 * @param string $foo
 *   Foos a foo.
 *
 * @return array
 *   Blah.
 */

...then it is introducing inconsistency. Any new code in the stable release should match the existing coding standards.

Your linked example is for D8. D8 is not a stable release, so is free to update the coding standards and format code to conform to these standards, in whatever way is most convenient.

(I would argue that function-by-function coding standards conversion is not convenient, and it should be done module-by-module (or file-by-file in case of includes/) instead, but that's not really within the scope of this discussion.)

Anyhow, it's hard to enforce the rule that if patching d6 I should be following the coding standards as they were written for d6, if I have no coding standards to look at. I really don't want to grep through the entire d6 codebase every time there's a patch to backport, just to try and deduce a rule from what is, honestly, an inconsistent standard.

Regarding consistency, I'll say this. Every single time I've submitted a patch that copies non-standards-compliant examples from other parts of the same file, it's been slapped back to "needs work". If you want to enforce this consistency rule, you really should document it somewhere so I've got a leg to stand on when debating that "needs work" status.

@webchick in #8

If 99% of the code in D7 (the stable release of core) looks like this:
(examples with and without a blank-line added before the @return)

I challenge you to quote any standard which was followed by 99% of any Drupal core version and which has subsequently been changed. The example you mention is split 69/31 in favor of the blank line:

Blank line before @return:

bobvin@bowie:~/www/7.x$ egrep -rB1 '@return' includes modules themes profiles | grep -v '@return' | egrep -v '^--$' | cut -f 2 -d '-' | egrep '^ *\*$' | wc -l
986

No blank line before @return:

bobvin@bowie:~/www/7.x$ egrep -rB1 '@return' includes modules themes profiles | grep -v '@return' | egrep -v '^--$' | cut -f 2 -d '-' | egrep -v '^ *\*$' | wc -l
442

In D6, the percentages were very nearly reversed, but no less underwhelming:

bobvin@bowie:~/www/6.x$ egrep -rB1 '@return' includes modules themes profiles | grep -v '@return' | egrep -v '^--$' | cut -f 2 -d '-' | egrep -v '^ *\*$' | wc -l
312
bobvin@bowie:~/www/6.x$ egrep -rB1 '@return' includes modules themes profiles | grep -v '@return' | egrep -v '^--$' | cut -f 2 -d '-' | egrep '^ *\*$' | wc -l
150

If your example was intended to highlight type-hinting rather than the blank line, the ratios are D7/D8: 14/86, D6: 8/91 -- so I would probably conclude based on the ratios that type-hinted @return lines were forbidden in both versions, or slightly less discouraged in D7/D8. Neither conclusion would be correct.

So how am I supposed to deduce an unwritten rule? Perform gyrations like the above and presume that the 69% statistic is the preferred one? Or am I supposed to have been sufficiently aware of the outdated standards that I can simply refer to memory? Either choice sets what IMHO is an unrealistically high bar for contribution to core.

I submit that such inconsistencies are the main reason that standards are changed. People start doing things a different way, despite the standard, and eventually the standard changes to reflect (current) best-practice. People like you who personally inspect nearly every new line of code that goes into core obviously have a better handle on what current best-practice entails. Therefore, it is up to you to codify such standards so the rest of us don't have to clone your brain (or duplicate your research) to achieve the same level of understanding. If you want to enforce different coding standards on different core versions, then by all means please document the differences rather than assuming they are common knowledge. We're not all as smart as you are, and even equally smart people can disagree sometimes. :-)

Any new code in the stable release should match the existing coding standards.

At last we agree, except that:

By existing coding standard, I mean:

currently existing and documented by the readily available coding standards page.

whereas it appears that:

By existing coding standard, you mean:

existing at the time the code was written, either as a written (but no longer readily available) standard or an unwritten (but usually followed) one.

If I am still misunderstanding you, please correct me again. I promise that I'll eventually get it right.

New proposal: coding standards should be defined by the Coder review module.

Since coder review already exists in separate branches targeting the separate core versions, we already have separately-published, de-facto standards.

Here is my suggested workflow for accomplishing this change:

1. Migrate Coder review into core.
   EDIT: retracted as per webchick's response in the next comment.

1. Integrate coder review into PIFR/PIFT so that patches which pass Simpletest are also subject to a coder review test.
   EDIT: Partially being worked on as part of project application revamp.
2. At least initially, the coder review should be included in test results but should not result in a needs work status.
3. Recruit or inspire more active maintainers for Coder.
4. When the coder review reports violations in code that has been RTBC'd, this should be filed as a bug, either:
   • Against coder review for falsely identifying standards-compliant code as being in violation.

   or:

   • Against the RTBC'd code, for being in violation of the standards.

   and possibly, in rare circumstances:

   • Against the coding standards documentation, for being in disagreement with the coder review.
5. When the coder review finds no problems with code that is subsequently marked CNW for being in violation of coding standards, this should also be filed as a bug, either:
   • Against coder review, for failing to detect the violation.

   or possibly, in very rare circumstances:

   • Against the coding standards documentation, for being in disagreement with the coder review.
6. In this way, we will gradually find and eliminate differences between what Coder reports and what the current standards recommend or require.
7. When the entire body of Drupal core can pass coder review with no errors, (or when that goal is within easy reach), the testbots should be re-programmed so that coder review affects the actual test result. Violations should result in a needs work status.

Updated issue summary.

jthorson is currently doing a lot of this work on PIFT/PIFR. See the tags under here http://drupal.org/project/issues/search/?issue_tags=project%20applicatio...

I don't think Coder Review needs to be moved into core for this to happen (in fact, that would be bad since we couldn't then use Coder Review stats on contributed modules as well). It does, however, need some more active maintainers.

Updated the issue summary, listing proposed resolutions and their drawbacks.

+1 for Require all code to meet currently-published standards, regardless of surrounding code.

EDIT: Basically, what the coding standard says right now (as listed in the issue summary).

IMHO, defining separate standards for the different versions of core would add to the complexity/learning curve we want to alleviate.

subscribing, will read this later... but after reading just the summary, I am not understanding why this is an issue? We've always required new but not old versions of core to comply with new coding standards (although enforcement is not always uniform), and contrib has always been of varying quality in many ways (including coding standards). What do you want to change?

For the convenience of keeping development up to date, any bugs in the current version must be fixed in development first. Hence, any new comer trying to help fill D7 bugs must learn how D8 core has changed on a logical basis as well as from a coding perspective. Then, once completing the rather daunting task of getting a D8 patch committed, the patch then has to be re-rolled for both D7 logic changes and documentation regressions to get the fix into the D7 production code. Does this really make sense??

I thought as a community we wanted more people to be involved with patches and eventually become core contributors. Since all code enhancements have to be done in D8 first, what is the harm in introducing docblock best practices back into D7 bit by bit with logical code fixes?

Attempt to summarize the solutions proposed thus far.

Make the "Proposed resolutions" an ordered list for easy reference, and include "Pro" and "Con" sections for each.

Added id to proposed resolutions to make it linkable.

Better description of why we make standards in Resolution #5.

Better wording.

More wordsmithing. (Why does that term carry a negative connotation?)

@jhodgdon -- Sorry you didn't find the issue summary helpful. I have updated the list of Proposed resolutions with the main "Pro" and "Con" points raised in individual comments. Should I gather a straw poll of users in favor and opposed to each of the five possible resolutions?

My feeling is that your favorite is #5; webchick prefers #1 or #2; I initially preferred #3 but am now leaning toward #4. (Not that my opinion counts for anything.)

RE #17 - yes, that is correct. Anyone wanting to do core development seriously pretty much has to get into the in-development core version. It's always been that way, and it's always been a daunting burden to newcomers. We can't really change that, because we need to make sure that we don't have bugs that are fixed in, say, Drupal 6 but become broken again in Drupal 7/8.

A newcomer can start by porting patches from the dev version back to previous versions though, if they want to work on the version they're actually using.

Regarding docs standards, if someone is doing a D6 or D7 docs patch, I would always expect them to bring the docs up to the *current* docs standards (with the one exception of the Implementation of vs. Implements for hook implementations, which was not adopted until d7, and we made the decision at the time not to port to d6, although it could have largely been done with a script). So for docs at least, I'm in favor of #3 in the above (with that one exception) -- we don't have versions of the docs standards, just a "current" docs standard.

And that is pretty much true of the non-doc coding standards too, I think? The only major changes I'm aware of in coding standards between versions have been:
- D6 to D7 - changed "implementation of" to "implements" for doc headers for hooks.
- D6 to D7 - added whitespace around "." for concatenation
Are there more standards changes that are driving this issue? I just don't see it as a major problem. Am I missing something?

Make each proposed resolution linkable.

Added sub-option to resolution #3.1 as per jhodgdon's latest comment.

Changed "drawbacks" to "pros and cons" to better reflect current status.

@#19 by jhodgdon:

Are there more standards changes that are driving this issue?

Nobody knows but (possibly) people like Dries, webchick, Gábor Hojtsy, and catch. The rest of the us great unwashed masses are ignorant and uneducated, and likely to stay that way until somebody documents the standards differences in an easily-referenced spot.

I just don't see it as a major problem.

No, it's a normal problem, according to the issue priority. Feel free to downgrade it to minor if you must, but it's quite definitely a problem, as evidenced by the comments referenced in the summary. To be even more pedantic, it's a task, which implies that while nothing is really broken, the situation could and should be improved.

Am I missing something?

Yes! A documentation page listing standards differences that must be considered when backporting a patch from D8 to D7 to D6, both in core and contributed modules.

EDIT: Added resolution option 3.1 to the summary.

So, to flesh out option 3.1 and provoke further discussion, I propose the following documentation stub:

Changes to Drupal core must be applied the most recent development version first, then (if possible) backported to previous versions. When backporting a patch, the modified code should conform to currently-published Drupal coding standards, with the following exceptions:

When backporting from Drupal 7 (or later) to Drupal 6 (or earlier):

1. At the beginning of the summary documentation for hook implementations:

   Change

   Implements

   To

   Implementation of

2. When performing string concatenation with a string constant, remove the space between the string delimiter character and the dot. For example:

   Change

   $foo = $bar . 'some additional text';

   To

   $foo = $bar .'some additional text';

3. (Anything else?)

Project maintainers are encouraged to follow this same policy in their contributed module code.

Gracious, I would never make change number 2 when backporting to D6! And I would even question the wisdom of change #1. Really, the standards are the standards. They're not versioned. It's just that we're a bit more tolerant of things not being up to standards in past versions than in current versions.

I become more and more convinced that discussions like this are a complete mis-use of time, energy, and focus, and we would all be far better off spending all of that effort on automating Coder module reviews for uploaded patches. Then we can put whatever behaviour we want *in code* and make sure it's consistently enforced across all projects.

Marking postponed. See http://drupal.org/project/issues/search/?issue_tags=project%20applicatio... for a list of blocking issues to rolling out that functionality. jthorson and rfay are both familiar with the underlying testbot architecture and could likely help anyone who wants to help with this get pointed in the right direction.

Added '@todo" to the bit in 3.1

@#23 by webchick:

... Then we can put whatever behaviour we want *in code* and make sure it's consistently enforced across all projects.

Yay! That sounds like a decision!

Updated issue summary accordingly.

It appears that we have reached a decision.

Opened #1299710: [meta] Automate the coding-standards part of patch review

And updated Issue summary accordingly.

@sun - I very much agree with your sentiment in #26.

Having two different coding standards and always applying patches to D8 first means that every single patch for D7 at minimum will have to be changed to make the documentation more sloppy? That as an American general once said is "Nuts!"

I'd be okay with it as long as we actually publish two different coding standards. I am not willing to write code that adheres to an unwritten standard.

Unfortunately, this whole discussion probably makes #711918: Documentation standard for @param and @return data types a won't fix.

I do not want the RTBC queue of D7, which is primarily focused on bug fixes, to be cluttered and clogged with "make this more nicer" style patches. We had a "polish" phase for those types of patches in D7, and that phase is long done. D7's strictly in maintenance mode now.

I also do not want to introduce massive amounts of inconsistency into D7's PHPDoc, where sometimes things have data types, sometimes they don't, sometimes they use new D8-style @param/@return spacing, sometimes they don't. It makes D7 harder to grok for the 99.999% of users who are not core developers and do not know or care what's happening in D8. They simply need to read through a given core file/function and understand what's going on, which is dramatically harder when their IDE shows completely inconsistent things depending on what particular part they're looking at.

And finally, D7 is a stable release. That means that the rules we set forth in terms of what's expected for coding standards compliance for contributed modules was established back in December 2009 or whenever the polish phase ended. We can't up and change this on contributed module developers during some arbitrary point release N months after 7.0, because they need a non-moving target to code against. See comments like http://drupal.org/node/711918#comment-2589916.

This is not some new, crazy position. We did the same thing with D6's concatenation operators when we changed those in D7. When we make a massive, developer-facing change to the coding standards those are enforced per-major version, though contributed module authors are encouraged to get a jump on the DN+1 version if they want.

So, yes, there's going to be a small inconvenience for re-rolling certain docs patches for D7, same as there's a small inconvenience for re-rolling patches that make string fixes in D8 that we don't deem important enough to break in D7. Tag 'em as Novice if you don't feel particularly inclined to do so yourself.

closed (won't fix) #711918: Documentation standard for @param and @return data types

Thanks for the clarification.

And no, this doesn't make #711918: Documentation standard for @param and @return data types "won't fix". D8 is in code thaw, and is perfectly free to make whatever improvements to the coding standards it would like.

However, I would say that I personally vastly prefer to "postpone" any large-scale docs clean-ups on getting Coder module integrated into the testbot (which is very close) and writing rules for Coder module so it enforces D7 vs. D8 coding style automagically, since even if this was documented somewhere in the coding standards (the onus of documenting those differences is on the shoulders of whoever changes the coding standards, btw), no one will actually read it. :P~ Every minute spent arguing about things like this vs. simply building automated tools for enforcing the rules that we need is a minute wasted, IMO.

writing rules for Coder module so it enforces D7 vs. D8 coding style automagically

As a practical matter, this means installing three different versions of coder review on qa.drupal.org.

See related: #1161796: Initial d8 port of Coder and Coder Review modules.

For the record, I also oppose the "intentionally make D7 more sloppy" approach. D7 is already inconsistent. From the root of a fresh 7.x git clone:

% egrep '@param [^\$]' */* */*/* */*/*/* | wc  
     364    2023   20792
% egrep -l '@param [^\$]' */* */*/* */*/*/* | wc
      64      64    1710

To translate, there are 64 files in core that put something between @param and $ for a total of 364 occurrences. Some of those are .js files where you see stuff like @param context. However, even with a sloppy way to avoid those in the counts, we still get a ton of hits for this in PHP code:

% egrep '@param [^\$]' */* */*/* */*/*/* | grep -v js | wc
     269    1643   16916
% egrep -l '@param [^\$]' */* */*/* */*/*/* | grep -v js | wc
      53      53    1472

I share the sentiment about not wanting an avalanche of doc-only patches flooding the D7 RTBC queue. But, for example, it seems pointless to strip this useful data out of patches that are already being back-ported. For example, I'm coming here from my experience at #1393234: API docs for field_access() are confusing.

I definitely don't have the energy to lead a big fight around this, but I just wanted to share my concerns that this policy seems misguided, and we should be more flexible (*gasp*) about incremental improvements like this, even in a stable release. In some ways, it's great how much our community and project considers the docs really part of the code and treats it all the same. But, IMHO, there's still a difference, and we should have different criteria for documentation fixes (and backports) than we do for the code itself.

My $0.02...

Thanks,
-Derek

Linked to newly-opened issue.

Coding standards changes are now part of the TWG

Moving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.

I have read the IS and most of the comments. I admit that after #23, which I agree with, I started to skim. It was interesting to read about the challenges faces 12 years ago. Fortunately, and as far as I can tell these problems are in the past. Complying with coding standards is still a challenge but much is automated now.

Based on #23 and the lack of discussion for 12 years I am closing this issue. I think outdated is the best status.