Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Currently, the standards don't define whether there should be a blank line after a file's initial <?php
line. To remedy this, we propose appending the following to the coding standards' "PHP Code Tags" section:
All blocks at the beginning of a PHP file should be separated by a blank line. This includes the
/** @file */
block, the namespace declaration and theuse
statements as well as the following code in the file. So, for example, a file header might look as follows:<?php namespace This\Is\The\Namespace; use Drupal\foo\Bar; /** * Provides examples. */ class ExampleClassName {
Or, for a non-class file (e.g.,
.module
):<?php /** * @file * Provides example functionality. */ use Drupal\foo\Bar; /** * Implements hook_help(). */ function example_help($route_name) {
Comments
Comment #1
silverwing CreditAttribution: silverwing commentedComment #2
jhodgdonI am not aware that we have a standard for that. We can discuss it. Changing title and tags to reflect such a discussion
By the way, please do not add random tags to issues. Read the issue tag guidelines (just below the tags field).
Comment #3
Michael Hodge Jr CreditAttribution: Michael Hodge Jr commentedI went through a project that we recently did and looked at the 50+ contributed modules that were installed trying to see if i could ascertain if there was a sort of de facto standard (even if the sample size is really small).
I found more modules tended to use the space, rather than ones that didn't, but then again there were a handful of modules that didn't even have the @file declaration at all.
Personally, I find the space to be a little easier on the eyes, but that's just me. Having not been a part of a coding standards discussion before, I'm curious as to what arguments are made for and against the space or even if it should be included in the coding standards at all.
Comment #4
jhodgdonCoding standards changes are now part of the TWG
Comment #5
jhodgdonThere are several duplicates of this issue that I'm in the process of closing; will make sure they are listed as parent/related issues so they will appear on the sidebar here.
Comment #6
tizzo CreditAttribution: tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #7
pfrenssenI have a slight preference for having a space in between the opening PHP tag and the file docblock, but I personally think this is not of such importance that it should be a part of the standard. The code is perfectly readable either way.
Comment #8
tim.plunkettYes.
Comment #9
drunken monkeyI also prefer it with the blank line, it just looks a bit nicer. But maybe we can tone it down to a recommendation instead of a fixed rule. Holding up, e.g., the project application process for that wouldn't be worth it.
Comment #10
Liam MorlandMy preference is to keep the files shorter by leaving the space out, but it's not a big deal. The most important thing for me is that there is a recommendation one way or the other.
Comment #11
pfrenssenLet's do a tally for this issue as well as the duplicates.
From #1339022: Standardize yes/no on empty line before the @file docblock:
From #1852152: [policy, no patch] Blank line before @file docblock in PHP file types?:
This issue:
Total: 6 in favor, 2 against, 2 meh.
Comment #12
alex.skrypnykComment #13
alex.skrypnykCan we please make a decision on this after 2 years?
Comment #14
pfrenssenIt looks like it's pretty much decided. We just need to write the statement to include in the coding standards and approve it.
Comment #15
dawehnerIMHO its kinda pointless to discuss this issue as you can get rid of #2304909: Relax requirement for @file when using OO Class or Interface per file
Comment #16
pfrenssenYeah but then the question will be between the <?php and the namespace declaration, which is in essence the same discussion.
Maybe it's better to rename this to "require a blank line after <?php".
Comment #17
drunken monkey@ pfrenssen: Thanks for tallying, seems this is really decided now.
Tagging for final discussion. (I hope that's the way to go, sorry otherwise.)
Comment #18
khusingh CreditAttribution: khusingh commentedComment #19
khusingh CreditAttribution: khusingh commentedComment #20
DamienMcKennaI'd vote to "no", but I don't think there was a poll set up anywhere ;) I find it to be needless - there are already six lines (the :lt;?php line, a four-line docblock, plus one blank line) before it gets to the docblock for the first actual bit of code, plenty of breathing space, so why do we need another line?
Comment #21
andypost+1 cos readabilty, just require empty line after
<?php
Comment #22
Crell CreditAttribution: Crell as a volunteer commentedIt's not approved yet, but it appears likely that PSR-12, the new FIG coding standards that extend PSR-2, will require the header section of a file to be newline separated between each block. Vis, opening PHP tag (blank line) docblock (blank line) namespace declaration (blank line) use statements (blank line), etc.
That is, this change is a good anticipation of PSR-12. I am therefore +1.
[Edit: I'm a dope and referenced the wrong PSR number.]
Comment #23
dawehnerHere is a suggested addition to the coding standard page:
Comment #24
Perignon CreditAttribution: Perignon as a volunteer and commented@dawehner That example doesn't include a blank line after the php declaration. That is what is primarily being discussed here.
I am on the fence about this one. I could honestly go either way. If you ask me what I do in practice, I include the blank like after <?php
Comment #25
dawehner@Perignon
Well, this are some rendering issue of the syntax highlighting on
drupal.org
Comment #26
DamienMcKenna@dawehner: I almost LOL'd at how the email notification of your message removed all of the blank lines your comment tried to demonstrate. Technology, eh! ;-)
Comment #27
Crell CreditAttribution: Crell as a volunteer commentedFor reference, here's the PR where FIG is discussing this question for PSR-12: https://github.com/php-fig/fig-standards/pull/730
While I cannot of course guarantee that it won't change before PSR-12 is approved, I would be supportive of us following FIG's current trajectory on this one and adopting that header style (which includes a blank line after the opening php tag, as discussed here).
[Edit: I'm a dope and referenced the wrong PSR number.]
Comment #28
xjmI think the standard as stated in the current title (blank line after
<?php
) improves readability and is more consistent with our other whitespace guidelines about blank linkes between members, at the beginning and end of classes, etc., so +1.Comment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commented+1 to announcing this for final discussion. I'll bring it up on our next call to see if the rest of the committee agrees.
Meanwhile, retitling with the proposal that has the most support so far.
Also tagging for an issue summary update. Would be good to copy the specific suggestion from #23, mention who's against it so far and why, and include the reference to the PSR-12 draft. Also, any insight on how close PSR-12 is to being adopted? If it's close, would it make sense to postpone this issue on it?
Comment #30
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAhem, this was actually already done: https://groups.drupal.org/node/509885. Fixing the tag. Issue summary update would still be good though.
Comment #31
alex.skrypnykComment #32
alex.skrypnykSorry, did not realise this was already changed on purpose.
Comment #33
NerOcrO CreditAttribution: NerOcrO at Axess Open Web Services commented+1 for a new line like @xjm :)
Comment #34
drunken monkeyUpdated the issue summary, also correcting the example to follow #2304909: Relax requirement for @file when using OO Class or Interface per file (and actually mentioning where this should go). Please correct me if I got anything wrong there.
Comment #35
alexpottSo looking at the examples on PSR-12... https://github.com/php-fig/fig-standards/blob/master/proposed/extended-c... actually having a new line after the opening PHP tag is appears against that standard from the first few examples. Looking at vendor though Symfony always has a new line here - but they always have a comment about copyright. Guzzle doesn't have a new line between php and namespace. Actually reading the proposed standard - it actually does not make any statement about what immediately follows a php open tag apart from
And in fact the proposal has examples of both with and without.
The only time it mandates anything is this...
But given we're not using this PHP feature atm that doesn't feel too relevant.
So, tldr, PSR-12 doesn't guide us here at all (yet - of course that could change).
Comment #36
alexpottIf we do mandate a blank line then we are going to need to think about how this contradicts the one thing PSR-12 does say on the subject of blank lines ie.. the bit about
declare
statements. And given that I actually think we should consider having no blank line because it will be easier to implement when we do have the need to declare strict types and given the @file docblocks have been removed or a collapsed by default in editors I don't really think readability suffers anyway.Comment #37
Liam MorlandI suggest making a pull request against PSR-12 either adding or removing the blank lines so that all examples are consistent. Then discussion about the pull request could iron out this issue there. I think Drupal should follow whatever PSR is doing.
Comment #38
DamienMcKenna+1 for adopting an existing standard, and if one doesn't exist then we push the closest one to covering this too (i.e. PSR-12).
Comment #39
Crell CreditAttribution: Crell as a volunteer commentedThe mandatory blank line for PSR-12 is still in a pull request; I'll have to harass the editor to see why it's not been merged yet.
At this point PSR-12 is at best advisory, as it is subject to change, as noted. However, it's also true that we can influence it. Although Drupal doesn't use PSR-2, "what are people actually doing" is a significant input to PSR-12. If Drupal approves a mandatory blank line, PSR-12 is (somewhat) more likely to do so eventually as well. And vice versa.
The alternative would be to clearly tell FIG that we want to go blank-line, but are waiting on them to figure this stuff out so we can follow their lead, at least for the "header formatting". I'm fine with that approach as well.
Disclaimer: I'm the Drupal FIG rep so telling FIG what our preference is would explicitly be part of my job.
Comment #40
dawehnerWell, we use the newline pretty much everywhere, so documenting that as part of the CS seems more like a technical detail at that point.
Comment #41
effulgentsia CreditAttribution: effulgentsia at Acquia commented@jthorson, @tizzo, and I discussed this on our last coding standards call, and were wondering why PSR-12 is currently proposing to require no blank line before
declare
statements? If they had a good reason for this, and if it's likely to remain this way when the standard is adopted, then perhaps we should try to keep Drupal compliant with that, and modify this issue's proposal to specifically call out an exception for when a file has adeclare
statement?On the other hand, we generally prefer simpler coding standards to more complex ones (i.e., where possible, we prefer do this over do this except when that), so we're open to reading arguments about why Drupal should violate the current PSR-12 proposal with respect to a blank line before
declare
. Such an argument could be along the lines of:If there's a link with a discussion about that.
Comment #42
Crell CreditAttribution: Crell at Platform.sh for Acquia commentedThe PR discussion is here:
https://github.com/php-fig/fig-standards/pull/730
Disclaimer: Yes, I wrote it. :-) The main argument in favor is the same "simplicity, fewer exceptions" one as made here. The main argument against is some existing doc parsers will apparently complain because they mandate the opposite. The response to that seems to be "that's nice, they'll need to be updated anyway for PSR-12, they can update that too." The PR is basically waiting on "people doing other things and got busy".
There's an amusing chicken-and-egg here, as PSR-12 is, in theory, supposed to be based in part on what people are doing in practice, whereas we're here waiting to see what PSR-12 does before deciding what to do in practice.
My personal stance is that Drupal adopting the simpler/better "new line between each section, consistently" format now will help nudge PSR-12 to adopting that same policy, so we should do so. It's not a guarantee, but it improves the odds. And if in the end PSR-12 goes the other way and we decide to adapt to match PSR-12 in this regard, it's a fairly non-invasive change and easily scriptable across the repo.
Comment #43
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThanks for #42. Given the discussion on that PR, @jthorson, @tizzo, and I agreed it makes sense to move forward with this as described and not make an exception for
declare
.If PSR-12 is adopted with the requirement of no blank line before
declare
, then we can open a new issue at that time to discuss bringing Drupal's coding standards into alignment with that.Moving to the core queue per step 7 of https://www.drupal.org/project/coding_standards, although I did not do a grep to see if any core code or core patches are affected.
Comment #44
xjmThanks @effulgentsia.
We'll adopt and apply this for core once #2754187: Add rule for a blank line following a PHP tag is available. Postponing on that.
Comment #45
klausiAll required sniffs for this are now fully implemented in Coder (dev version, not released yet). Please test!
Comment #46
xjmYay, thanks @klausi! I tested the rule locally and it seems to work correctly. There are a handful of files in core that are not compliant.
Filed #2755677: Fix 'Drupal.WhiteSpace.OpenTagNewline' coding standard.
This rule is committer-approved pending making core compliant, having the rule in a tagged release of coder, and enabling the rule in
phpcs.xml.dist
. We will do these things during the RC phase of 8.2.x which begins September 9, 2016.Comment #47
alexpottI agree with @xjm about waiting till the rc phase for 8.2 for introducing a new rule. Also doing #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core will allow us to manage coding standards change better.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedApproved (belatedly) for Drupal 7 core also. (I guess if these issues aren't going into the Drupal 7 RTBC queue ever, Drupal 7 maintainers will have to start keeping an eye for them over here too...)
One minor suggestion, when moving the final documentation into the coding standards, instead of this:
I would suggest this:
(emphasis added)
Since I don't think any of the previous-mentioned items are always required to be there in the first place.
Comment #49
Crell CreditAttribution: Crell at Platform.sh for Acquia commentedFeel free to steal the exact language from the PSR-12 draft, or suggest improvements to it over in that PR.
Comment #50
xjmComment #51
tizzo CreditAttribution: tizzo commented