Make WordPress Core

Opened 16 years ago

Closed 13 years ago

#10633 closed enhancement (duplicate)

Omit closing PHP tag at the end of a file when using include() or require()

Reported by: hakre's profile hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.8.4
Component: General Keywords:
Focuses: Cc:

Description

The closing tag of a PHP block at the end of a file is optional, and in some cases omitting it is helpful when using include() or require(), so unwanted whitespace will not occur at the end of files, and you will still be able to add headers to the response later. It is also handy if you use output buffering, and would not like to see added unwanted whitespace at the end of the parts generated by the included files. (Source: http://www.php.net/manual/en/language.basic-syntax.instruction-separation.php)

Because this is proposed by PHP.net and a well known good practice, this closing tag should be removed from files that are included/required only.

Additionally I suggest to add this to the Coding Standards: Omit closing php tag. This is a usefull hint for any PHP developer.

References: #10577, #10106, #8621, #7685, #7524, #7149, #6791, #4901, [856]

Attachments (1)

10633.patch (59.0 KB) - added by hakre 15 years ago.

Download all attachments as: .zip

Change History (16)

#1 @hakre
15 years ago

  • Keywords has-patch added

just for having it I did a patch that removes all ending ?>-squences in php-files. you find the patch attached, but if it makes problems applying it, here is a regex that did the job for me in eclipse file search / replace:

pattern: \?>(\n*)\s*\Z
replace: $1

@hakre
15 years ago

#2 @hakre
15 years ago

While reviewing the patch I've seen that

wp-includes/js/tinymce/plugins/spellchecker/*.php

look non-utf8 encoded. wp-mce-help.php infact is utf-8 encoded on that sole char. i created a new ticket for that: #10731.

#3 @dd32
15 years ago

  • Type changed from defect (bug) to enhancement
  • Version set to 2.9

There was some talk not too long ago about only having user-editable files without the closing tag, as it was useful for people to know if the file was fully uploaded.

That being said, Some files (that are non-user-editable) are shipping without it i think.

#4 follow-up: @westi
15 years ago

I am -1 to this.

I don't see the need for this except possible on user-editable files.

Personally, I find this a sign of lazy programming rather than good programming practise - code should always be explicit about its intentions not implicit.

#5 in reply to: ↑ 4 ; follow-up: @hakre
15 years ago

Replying to dd32:

There was some talk not too long ago about only having user-editable files without the closing tag, as it was useful for people to know if the file was fully uploaded.

You're kidding, right? Having "?>" is no proof of a fully uploaded file, that argument sounds not very serious to me.

When I see spaces and linebreaks after the ?> near the end of a file I always asked myself, what went wrong here... . But I do not need to make a big discussion out of this, I already suggested something to deal with this as well.

Replying to westi:

I don't see the need for this except possible on user-editable files.

Do you want to say that good coding practices are actually not counting?

#6 follow-up: @dd32
15 years ago

  • Keywords dev-feedback added

You're kidding, right? Having "?>" is no proof of a fully uploaded file, that argument sounds not very serious to me.

To a technical user yes. But to a non-PHPer, No. Quite often people will ask why wp-config.php doesnt have it, as they think it means its not fully there.

Semantics are not always for the programming language.

Whitespace immediately following a ?> is ignored by PHP and not rendered, However, Multiple new lines are not ignored (Single is fine, multiple is not).

Wontfix pending dev-feedback

#7 in reply to: ↑ 6 @hakre
15 years ago

  • Milestone changed from Future Release to 2.9
  • Version changed from 2.9 to 2.8.4

Replying to dd32:

You're kidding, right? Having "?>" is no proof of a fully uploaded file, that argument sounds not very serious to me.

To a technical user yes. But to a non-PHPer, No. Quite often people will ask why wp-config.php doesnt have it, as they think it means its not fully there.

Then put a comment in there. This would also help in terms to signal a propper end. And having this documented in the coding styles can help as well.

Whitespace immediately following a ?> is ignored by PHP and not rendered, However, Multiple new lines are not ignored (Single is fine, multiple is not).

From php.net, the official documentation: "so unwanted whitespace will not occur at the end of files, and you will still be able to add headers to the response later." Its written as an argument to not use ?> at the end, so I think it will be much safter to do according the official documentation then to isolated tests. So I suggest to stick with the official recommendation to remove it for includes / requires.

#8 in reply to: ↑ 5 ; follow-up: @westi
15 years ago

  • Milestone 2.9 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Replying to hakre:

Replying to westi:

I don't see the need for this except possible on user-editable files.

Do you want to say that good coding practices are actually not counting?

This is not good coding practice IMHO.

#9 follow-up: @johnbillion
15 years ago

"so unwanted whitespace will not occur at the end of files"

Forgive me for asking, but how can whitespace appear at the end of a file after a closing PHP tag? Surely if it's not there now, it's not going to just appear?

#10 in reply to: ↑ 9 @hakre
15 years ago

Replying to johnbillion:

"so unwanted whitespace will not occur at the end of files"

Forgive me for asking, but how can whitespace appear at the end of a file after a closing PHP tag? Surely if it's not there now, it's not going to just appear?

if there are whitespace chars like spaces, tabs, newlines etc (everything that counts as a whitespace in HTML mainly) after the closing "?>" (often added by editors, like a newline) according to the php documentation (and against what was said here) is infact output by the php interpreter to the client.

as you can see (and as it answers your questions): it's just output because it is in the file.

#11 in reply to: ↑ 8 @hakre
15 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Replying to westi:

Replying to hakre:

Replying to westi:

I don't see the need for this except possible on user-editable files.

Do you want to say that good coding practices are actually not counting?

This is not good coding practice IMHO.

IMHO this is good coding practice but I won't let my personal opinion count that much. Therfore I suggested to go with the suggestion of many others, undergne scrunity and publicly documented on php.net. Let's put this on a more professional level then personal opinions.

From all the reasons argued against the suggestion to remove "?>" at the end, not much is left. For that argument that it marks the EOF, I strongly suggest for the case to have "?>" at the end to excplicitly add an additional comment to every file that this is the end of the file, because even partially transfered files might end with an "?>" and they are acutally partially transfered. If this is really the problem that users are asking questions if everything is right.

Anyway, having this documented properly in the coding guidelines (which could get a better controlled application to the source anyway) would help us all with little effort. Even users who do not fully understand how PHP is working can be directed there to give them a read.

I reopen this because this still can get enhancement in any way.

#12 @Denis-de-Bernardy
15 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed

@hakre: suggesting we remain pragmatic here. westi clearly doesn't want this in...

#13 @hakre
14 years ago

Related: #12307

#14 @hakre
13 years ago

  • Keywords has-patch dev-feedback removed
  • Resolution wontfix deleted
  • Status changed from closed to reopened

This has been fixed in [19712] and got traction in the #12307 duplicate ticket.

Reopening so the ticket can be closed as fixed (not as wontfix).

#15 @dd32
13 years ago

  • Resolution set to duplicate
  • Status changed from reopened to closed

duplicate of #12307

Note: See TracTickets for help on using tickets.