Make WordPress Core

Opened 7 years ago

Closed 21 months ago

#40039 closed defect (bug) (fixed)

Remove closing PHP tags from bundled themes

Reported by: netweb's profile netweb Owned by: desrosj's profile desrosj
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch
Focuses: coding-standards Cc:

Description (last modified by netweb)

Previously in #12307 / r19712, #23505 / r28678, and #30534 / r34887 closing PHP tags have been removed from PHP files and is now a documented coding standard:

""Omitting the closing PHP tag at the end of a file is preferred. If you use the tag, make sure you remove trailing whitespace."

The PHP manual states:

"If a file is pure PHP code, it is preferable to omit the PHP closing tag at the end of the file. This prevents accidental whitespace or new lines being added after the PHP closing tag, which may cause unwanted effects because PHP will start output buffering when there is no intention from the programmer to send any output at that point in the script."

  • Themes that exclude closing PHP tags: Twenty Fourteen and Twenty Seventeen
  • Themes that include closing PHP tags: Twenty Ten, Eleven, Twelve, Thirteen, Fifteen, and Sixteen

Should the closing PHP tags be removed from the above themes?

Related: #bb3078

Attachments (9)

40039.patch (269 bytes) - added by yahil 7 years ago.
Remove closing PHP tags
40039-twentyten.diff (6.0 KB) - added by milindmore22 7 years ago.
removed closing php tag from Tewnty ten theme
40039-twentyeleven.patch (3.4 KB) - added by yahil 7 years ago.
Remove closing tag for twentyeleven theme.
40039-twentysixteen.patch (1.9 KB) - added by vishalkakadiya 7 years ago.
I added patch for TwentySixteen theme.
40039-twentythirteen.patch (5.0 KB) - added by NomNom99 7 years ago.
Removed closing php tags for twentythirteen theme
40039-twentyfifteen.patch (2.9 KB) - added by manishsongirkar36 7 years ago.
Added patch for twentyfifteen theme.
40039-twentyseventeen-1.diff (495 bytes) - added by yahil 7 years ago.
Removed closing php tag from twentyseventeen theme.
40039-twentyeleven-1.diff (5.4 KB) - added by yahil 7 years ago.
Removed closing php tag from Twentyeleven theme.
40039.2.patch (32.0 KB) - added by sabernhardt 3 years ago.

Download all attachments as: .zip

Change History (36)

#1 @netweb
7 years ago

  • Description modified (diff)
Last edited 7 years ago by netweb (previous) (diff)

#2 @dd32
7 years ago

IMHO changing older bundled themes doesn't gain much benefit, but we should ensure that Twenty Seventeen is consistently sticking to the guideline.

Last edited 7 years ago by dd32 (previous) (diff)

#3 @yahil
7 years ago

Remove closing tag for Seventeen theme.

Last edited 7 years ago by yahil (previous) (diff)

@yahil
7 years ago

Remove closing PHP tags

@milindmore22
7 years ago

removed closing php tag from Tewnty ten theme

@yahil
7 years ago

Remove closing tag for twentyeleven theme.

@vishalkakadiya
7 years ago

I added patch for TwentySixteen theme.

@NomNom99
7 years ago

Removed closing php tags for twentythirteen theme

#4 follow-up: @netweb
7 years ago

Thanks for all the patches :)

@NomNom99, @vishalkakadiya, and @yahil could you all please refresh your patches from the "root" of the repo please.

The 40039-twentyeleven.patch shows the full path in the diff to the file location:

src/wp-content/themes/twentyten/

Including the full path is easier for everyone to checkout the patch to both test and when and if needed to commit :)

There's some docs on working with patches in our handbook:

https://make.wordpress.org/core/handbook/tutorials/working-with-patches/

@manishsongirkar36
7 years ago

Added patch for twentyfifteen theme.

#5 follow-up: @vishalkakadiya
7 years ago

@netweb

I am making patch from github repo, where I didn't found twentysixteen theme check this https://github.com/WordPress/WordPress/tree/master/wp-content/themes, Can you advise on this ?

Thanks!

#6 in reply to: ↑ 5 ; follow-up: @netweb
7 years ago

Replying to vishalkakadiya:

@netweb

I am making patch from github repo, where I didn't found twentysixteen theme check this https://github.com/WordPress/WordPress/tree/master/wp-content/themes, Can you advise on this ?

Thanks!

@vishalkakadiya Right you are, can you instead then create a pull request to the Twenty Sixteen theme and add a link here to this ticket to the pull request please? https://github.com/WordPress/twentysixteen

@yahil
7 years ago

Removed closing php tag from twentyseventeen theme.

@yahil
7 years ago

Removed closing php tag from Twentyeleven theme.

#7 in reply to: ↑ 4 @yahil
7 years ago

@netweb Done :)

Replying to netweb:

Thanks for all the patches :)

@NomNom99, @vishalkakadiya, and @yahil could you all please refresh your patches from the "root" of the repo please.

The 40039-twentyeleven.patch shows the full path in the diff to the file location:

src/wp-content/themes/twentyten/

Including the full path is easier for everyone to checkout the patch to both test and when and if needed to commit :)

There's some docs on working with patches in our handbook:

https://make.wordpress.org/core/handbook/tutorials/working-with-patches/

#8 in reply to: ↑ 6 @vishalkakadiya
7 years ago

@netweb

Send pull request for twentysixteen, you can check on https://github.com/WordPress/twentysixteen/pull/497

Replying to netweb:

Replying to vishalkakadiya:

@netweb

I am making patch from github repo, where I didn't found twentysixteen theme check this https://github.com/WordPress/WordPress/tree/master/wp-content/themes, Can you advise on this ?

Thanks!

@vishalkakadiya Right you are, can you instead then create a pull request to the Twenty Sixteen theme and add a link here to this ticket to the pull request please? https://github.com/WordPress/twentysixteen

#9 follow-up: @vishalkakadiya
7 years ago

@netweb
Here is the refreshed pull request on https://github.com/WordPress/twentysixteen/pull/498.

#10 in reply to: ↑ 9 @netweb
7 years ago

Replying to vishalkakadiya:

@netweb
Here is the refreshed pull request on https://github.com/WordPress/twentysixteen/pull/498.

Thanks, I've replied on the PRs

This ticket was mentioned in Slack in #core by hareesh-pillai. View the logs.


4 years ago

@sabernhardt
3 years ago

#12 @sabernhardt
3 years ago

  • Focuses coding-standards added
  • Keywords has-patch added

40039.2.patch combines previous patches and includes more recent themes.

#13 @desrosj
23 months ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to desrosj
  • Status changed from new to reviewing

This ticket was mentioned in PR #2992 on WordPress/wordpress-develop by audrasjb.


22 months ago
#14

Trac ticket: https://core.trac.wordpress.org/ticket/40039

Putting together the previous patch in a Github PR so it's easier to review and to refresh if needed.

#15 @audrasjb
22 months ago

  • Keywords 2nd-opinion removed

☝️ I put together a Github PR so it's easier to review and to refresh if needed.

sabernhardt commented on PR #2992:


22 months ago
#16

Thanks! This needed to be in a PR, and it will require quite a few newlines following the opening PHP tags.

This ticket was mentioned in PR #3018 on WordPress/wordpress-develop by sabernhardt.


22 months ago
#17

(Revisions to PR 2992)

Putting get_footer(); on its own line at the end of 63 files seems simple enough.

The value of modifying the other 20 files is a bit more questionable, particularly these 8 that have endif; at the bottom:

  • twentyten\loop.php
  • twentytwelve\sidebar.php
  • twentythirteen\sidebar.php
  • twentysixteen\sidebar.php
  • twentynineteen\template-parts\footer\footer-widgets.php
  • twentynineteen\template-parts\header\entry-header.php
  • twentynineteen\template-parts\post\author-bio.php
  • twentytwenty\template-parts\entry-author-bio.php

Additionally, this PR has a few extra changes:

  • Fixes a typo in a translator comment for Twenty Eleven's category.php.
  • Edits unrelated indentation earlier in the file for Twenty Ten's loop-attachment.php and loop-single.php.

Trac ticket: https://core.trac.wordpress.org/ticket/40039

#18 @desrosj
22 months ago

In 53875:

Twenty Eleven: Correct inline translator comment.

Props sabernhardt.
See #40039.

#19 @desrosj
22 months ago

  • Keywords commit added

I'm a bit torn here.

On one hand, I do agree with @dd32 in that I don't see much benefit to changing this in older themes.

On the other, it is important that the default themes follow all of the recommended coding standards and best practices as much as possible as they serve as examples to the greater community. People new to WordPress and theming often use these as examples and learn from them.

However, using the active install numbers to gauge overall usage, it's unlikely that a significant number of developers are using the older themes as a starting point for a new child theme. In my eyes, this decreases the amount of weight to give this argument default themes serving as examples to the greater community.

My current stance is that I think it's reasonable to make this change for more recent themes, and I'll make that commit shortly.

In my opinion, there are three groups I would place the default themes into. Block-based themes (Twenty Twenty-Two), modern themes (Twenty Seventeen through Twenty Twenty-One), older themes (Twenty Sixteen and earlier). My personal line for "more recent themes" is Twenty Seventeen and newer, so that's where I'll focus.

I'll leave this open for a bit longer to allow for some counter points arguments to fix this in Twenty Sixteen and earlier.

#20 @desrosj
22 months ago

In 53880:

Bundled Themes: Remove closing PHP tag at the end of files.

To help avoid issues with trailing whitespace, omitting the closing PHP tag at the end of a file is preferred.

Props netweb, dd32, yahil, milindmore22, vishalkakadiya, NomNom99, manishsongirkar36, sabernhardt, audrasjb, desrosj.
See #40039.

desrosj commented on PR #2992:


22 months ago
#22

Superseded by #3018.

#23 @desrosj
22 months ago

  • Keywords close added; commit removed

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


21 months ago

#27 @audrasjb
21 months ago

  • Keywords close removed
  • Resolution set to fixed
  • Status changed from reviewing to closed

As per today's bug scrub, the contributors agreed that fixing this for older themes doesn't provide much value. Let's close this ticket as fixed.

Thanks everyone!

Note: See TracTickets for help on using tickets.