Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify blank line after <?php #984

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Conversation

PowerKiKi
Copy link
Contributor

Blank line after <?php opening should be standardized clearly and be code samples should abide to the rules.

Now all header blocks are treated equally with a unique and simple rule for all of them.

See discussion: https://groups.google.com/forum/#!topic/php-fig/gKb63vV7k_4

Blank line after `<?php` opening should be standardized clearly and be code samples should abide to the rules.

Now all header blocks are treated equally with a unique and simple rule for all of them.

See discussion: https://groups.google.com/forum/#!topic/php-fig/gKb63vV7k_4
@@ -38,6 +38,7 @@ This example encompasses some of the rules below as a quick overview:

~~~php
<?php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare usually goes w/o blank line as was noted in discussion at mailing list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the discussion Woody mentions what is commonly found in existing code, not what we should aim for.

Why should we make an exceptional case for declare ? How is it simpler to say, sometimes we must write:

<?php
declare(strict_types=1);

use namespace Foo;

$a = 123;

But then sometime we must write:

<?php

/**
 * Some comment
 */

declare(strict_types=1);

use namespace Foo;

$a = 123;

Allowing no blank line before declare in some cases, make the rules harder to remember, harder to use, and look inconsistent across files of the same project.

Copy link
Contributor

@Crell Crell Dec 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making declare special makes little to no sense to me.

I MIGHT be convinced that "the first thing after the php tag could omit the newline" is a worthwhile exception, but only if that's the case regardless of what that thing is. Making an exception only sometimes just makes it harder for the human parser and machine parser.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree with @Crell here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe people used to regard this line break as a break before the namespace (or any other code), not after the <?php. Or as a separation between regular PHP code and "redundant header stuff".

Since <?php and declare(strict_types=1) will usually be the same in all files across a package, people have seen it as part of the "opening shebang", which needs to be separated from the rest.

Not having a line break before declare() makes it easier to visually skip this part and focus on the things that are specific to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donquixote, the "opening shebang", as you call it, would most likely be the PHP opening tag immediately followed by a file-level docblock in many open source projects, because it is common practice to include some boilerplate about license. A declare statement would only come after those.

If you are willing to add boilerplate headers of several lines (which again seems to be a common practice and brings absolutely no value in terms of pure coding), and somehow be able mentally "skip" those, you might as well tolerate a single more blank line.

@KorvinSzanto KorvinSzanto merged commit 95a37b5 into php-fig:master Dec 20, 2017
@gmponos
Copy link
Contributor

gmponos commented Dec 20, 2017

I am really confused.

Quoted from the mailing list:

I just found that whether there should be one blank line after opening <?php is not clarified now

I agree that it has to be standardized and not left open to interpretation.

Quoted from the comments here: #861 (comment)

In my opinion I believe that this new PSR should stricly declare if the <?php tag MUST or MUST NOT be followed by a blank line

It does.

What does?

[PSR-12] should strictly declare if the <?php tag MUST or MUST NOT be followed by a blank line.

@PowerKiKi
Copy link
Contributor Author

@gmponos what is your question ?

@gmponos
Copy link
Contributor

gmponos commented Dec 21, 2017

Aren't those two statements above contradicting and also which one of these two statements is true at the end? I think both of them were stated from members of this repo, aren't they?

Look, I am not a native English speaker neither I use English all the time. So when I read something and a member comes and says "It does" and closes the issue I just rub my eyes and try to see where.

Another member could have said something like "Look you are wrong, it does not but never the less we want to leave it like this" and since no-one else said anything about it I just thought that probably I am lost in translation and let's back off a little.

Now someone comes and says "it does not". It's really discouraging contributions this way.

Just stating my mind here.

@PowerKiKi
Copy link
Contributor Author

PowerKiKi commented Dec 21, 2017

I don't know about the discussion you had on that other thread, maybe best to ask over there for clarification.

But one thing is now sure. Because this PR was merged, a blank line MUST follow the <?php opening tag.

@donquixote
Copy link

donquixote commented Dec 26, 2017 via email

@PowerKiKi
Copy link
Contributor Author

The order has indeed been defined, and is not what you seem to expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants