WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#12307 closed defect (bug) (fixed)

files missing closing php tags

Reported by: washer Owned by: ryan
Milestone: 3.4 Priority: lowest
Severity: trivial Version: 2.9.2
Component: General Keywords: has-patch early
Focuses: Cc:

Description

Numerous files in the 2.9.2 'wp-includes' folder are missing closing php tags (cron.php, wp-cron.php, version.php, etc).

Attachments (3)

closing_tags_patch.diff (18.9 KB) - added by sorich87 4 years ago.
fix closing tags in all the wordpress core files
12307.diff (25.0 KB) - added by RyanMurphy 3 years ago.
diff as of r17488
12307.2.diff (49.1 KB) - added by ryan 2 years ago.
Remove ?> from EOF. Trim newlines.

Download all attachments as: .zip

Change History (29)

comment:1 nacin4 years ago

  • Milestone changed from Unassigned to 3.0

Closing PHP tags aren't required, though the general consensus among the core developers are to include them. We leave them off in files that are edited by users, which means wp-config.php and any default theme functions.php file. (The other theme files, of course, are expected to generate output.) For consistency we should review all files and add the ones that are missing.

comment:2 sorich874 years ago

  • Owner set to sorich87
  • Status changed from new to accepted
  • Summary changed from files in wp-includes missing closing php tags to files missing closing php tags

This applies to files to other wordpress files as well, not to wp-includes files only.

sorich874 years ago

fix closing tags in all the wordpress core files

comment:3 sorich874 years ago

  • Cc sorich87 added
  • Keywords has-patch added; cron missing php tags removed

comment:4 markmcwilliams4 years ago

  • Keywords reporter-feedback added

Simple patch, nothing wrong I can see, another ticket we can tick off the list? Or is there a plan to go through all the files before release of 3.0 and catch them?

comment:5 scribu4 years ago

  • Priority changed from normal to lowest

comment:6 kkarpieszuk4 years ago

  • Cc kkarpieszuk added
  • Resolution set to invalid
  • Severity changed from normal to trivial
  • Status changed from accepted to closed
  • Type changed from defect (bug) to feature request

missing closing php tag is not a bug, but coding style. IMO it's good coding style because this eliminates problem with white spaces after closing tag and common problem with error 'headers already send'

i suggest to close this issue

comment:7 scribu4 years ago

  • Milestone 3.0 deleted

comment:8 nacin4 years ago

  • Keywords reporter-feedback removed
  • Milestone set to 3.0
  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Type changed from feature request to defect (bug)

Re-opening.

It is the WordPress coding standard to include closing PHP tags on files that are not expected to be edited.

That excludes a theme's functions.php and wp-config.php.

The 'headers already sent' argument only works for those files. All core files should not be edited, and should have closing tags.

comment:9 nacin4 years ago

  • Owner changed from sorich87 to nacin
  • Status changed from reopened to accepted

comment:10 scribu4 years ago

I vote we change the WP coding standard so that closing tags are not included in any file.

comment:11 scribu4 years ago

Also, I think there are more important issues to fix atm. Maybe this could be moved to 3.1 early?

comment:12 nacin4 years ago

  • Keywords needs-refresh early added
  • Milestone changed from 3.0 to 3.1

Yeah, patch is stale anyway. I didn't bother applying, but since we changed include paths, it'll bork.

I vote we change the WP coding standard so that closing tags are not included in any file.

I would tend to disagree with that. It identifies the end of the file so you know the file is complete. The alternative would be to add // end. It's a fine sanity check. We're normally pretty good about them, so I'm surprised there's so many files missing them.

comment:13 dd324 years ago

I vote ....

I'd generally agree with that :)

We're normally pretty good about them, so I'm surprised there's so many files missing them.

I'm not. If you look through the list of files, a fair chunk of them will be ones which have been written(or majorly rewritten) in the not-too-distant past. To me, to not include them has been an attraction to most developers for awhile, even if against the coding standards, There are a few things in core which go against the standards which are generally seen as ok to some.

comment:14 jacobsantos4 years ago

Well, someone had to commit patches or take them out. I know in the past that patches would not be committed that took them out and there was a huge battle over taking it out of the wp-config.php. I believe eventually reason prevailed but only after the 20th ticket[1] was created that dealt with the "headers already sent" issue.

I think it was more or less the least favorite part of the standard. The arguments on why it should be included seem a little lacking, but a standard is a standard and what can one do? Regardless, a standard should be followed or it becomes more of a guideline and then we'll have spaces instead of tabs.

The standard should at least be updated to when it is acceptable to remove the closing tag or when it is required to have it.

[1] It was more like the 6th or 7th ticket, but okay.

comment:15 hakre4 years ago

Related: #10633

comment:16 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release

RyanMurphy3 years ago

diff as of r17488

comment:17 RyanMurphy3 years ago

  • Cc ryan@… added
  • Keywords needs-refresh removed

comment:18 follow-up: toscho3 years ago

  • Cc info@… added

wp-includes/default-filters.php has a blank line after the closing tag. Is that intended?

I think, we should drop the closing tags. They do not add any value.

comment:19 in reply to: ↑ 18 ; follow-up: RyanMurphy3 years ago

Replying to toscho:

wp-includes/default-filters.php has a blank line after the closing tag. Is that intended?

I don't see a blank line after the closing tag - I do see one between the last command and the tag though.

I think, we should drop the closing tags. They do not add any value.

IMO, they do add value, in that you know you have the full file. When I was making the patch, I had to look at some of the files in the trac browser to make sure I had the full file, because where it ended didn't look right.

If we don't want to use the PHP closing tag, we should at least add a comment, like I did with wp-config-sample.php. But right now, the standard is to include them.

comment:20 in reply to: ↑ 19 toscho3 years ago

Replying to RyanMurphy:

Replying to toscho:

wp-includes/default-filters.php has a blank line after the closing tag. Is that intended?

I don't see a blank line after the closing tag - I do see one between the last command and the tag though.

Oh, sorry, I shouldn’t rely on SVN. The Subclipse plugin is adding blank lines. Dangerous.

IMO, they do add value, in that you know you have the full file. When I was making the patch, I had to look at some of the files in the trac browser to make sure I had the full file, because where it ended didn't look right.

That’s probably a question of style. When I see a closing tag, I expect some output afterwards.
Shouldn’t diff show you if the patch is complete?

If we don't want to use the PHP closing tag, we should at least add a comment, like I did with wp-config-sample.php.

I don’t see such a comment in wp-config-sample.php?

#!end :)

ryan2 years ago

Remove ?> from EOF. Trim newlines.

comment:21 markjaquith2 years ago

Fine by me. Trailing whitespace issues are nasty, and hard to debug. I can see an argument for giving them a // EOF comment so people don't think the file terminated prematurely, but that's probably an edge case (incomplete file uploads, that is).

comment:22 westi2 years ago

I much prefer to keep them and would prefer that over a comment at the end of the file.

I also think there are much more important things to concern ourselves over that whether or not a file ends with ?>

comment:23 nacin2 years ago

  • Owner nacin deleted
  • Status changed from accepted to assigned

comment:24 ryan2 years ago

  • Milestone changed from Future Release to 3.4

comment:25 ryan2 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from assigned to closed

In [19712]:

Lose EOF ?>. Clean up EOF newlines. fixes #12307

comment:26 hakre2 years ago

Thanks for fixing this, please change close reason in the #10633 duplicate ticket.

Note: See TracTickets for help on using tickets.