WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#40039 new defect (bug)

Remove closing PHP tags from bundled themes

Reported by: netweb Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: 2nd-opinion
Focuses: Cc:
PR Number:

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 (8)

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

Download all attachments as: .zip

Change History (18)

#1 @netweb
3 years ago

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

#2 @dd32
3 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 3 years ago by dd32 (previous) (diff)

#3 @yahil
3 years ago

Remove closing tag for Seventeen theme.

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

@yahil
3 years ago

Remove closing PHP tags

@milindmore22
3 years ago

removed closing php tag from Tewnty ten theme

@yahil
3 years ago

Remove closing tag for twentyeleven theme.

@vishalkakadiya
3 years ago

I added patch for TwentySixteen theme.

@NomNom99
3 years ago

Removed closing php tags for twentythirteen theme

#4 follow-up: @netweb
3 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
3 years ago

Added patch for twentyfifteen theme.

#5 follow-up: @vishalkakadiya
3 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
3 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
3 years ago

Removed closing php tag from twentyseventeen theme.

@yahil
3 years ago

Removed closing php tag from Twentyeleven theme.

#7 in reply to: ↑ 4 @yahil
3 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
3 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
3 years ago

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

#10 in reply to: ↑ 9 @netweb
3 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

Note: See TracTickets for help on using tickets.