Make WordPress Core

Opened 11 years ago

Closed 6 months ago

Last modified 2 months ago

#33388 closed defect (bug) (invalid)

WP_Theme should use get_file_data() for retrieving page templates

Reported by: dd32's profile dd32 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Priority: normal
Severity: normal Version:
Component: Themes Keywords: needs-testing has-patch needs-refresh needs-unit-tests
Focuses: Cc:

Description

Currently WP_Theme::get_page_templates() uses a direct regular expression on the result of get_file_contents(), it should instead use the get_file_data() helper method we have to interact with file header data.

Currently switching will result in a back-compat break, as some themes have been using headers such as <?php // Template Name: Something ?> as the header, which get_file_data() doesn't like - See #33387

See r21117 for where we previously did use it temporarily during the 3.4 cycle, but broke due to #33387

Attachments (4)

33388.diff (790 bytes) - added by m_uysl 10 years ago.
depends on #33387
33388.2.diff (1.3 KB) - added by Thomas Vitale 8 years ago.
Patch refreshed
33888.3.diff (1.3 KB) - added by costdev 4 years ago.
Patch refreshed against trunk. Added missing {} in if statements, removed extraneous semi-colon + PHPCS fixes.
33888.4.diff (1.3 KB) - added by mosescursor 6 months ago.
Changed from $header to $type so declared post types are correctly applied.

Download all attachments as: .zip

Change History (20)

@m_uysl
10 years ago

depends on #33387

#1 @Thomas Vitale
8 years ago

  • Keywords needs-refresh added

@Thomas Vitale
8 years ago

Patch refreshed

#2 @Thomas Vitale
8 years ago

  • Keywords needs-testing added; needs-refresh removed

#3 follow-up: @joyously
6 years ago

  • Keywords close added

This ticket proposes a "should", but says it is not backward compatible. So why "should" it be changed?
It's not broken, so why change it?

#4 in reply to: ↑ 3 @SergeyBiryukov
5 years ago

  • Keywords has-patch added; close removed
  • Milestone changed from Awaiting Review to 5.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Replying to joyously:

This ticket proposes a "should", but says it is not backward compatible. So why "should" it be changed?

The backward compatibility concerns are addressed in [51182] / #33387, so this can now be addressed as well.

Why? Using an existing function instead of partially duplicating its functionality should make future maintenance easier.

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


4 years ago

#6 @audrasjb
4 years ago

  • Milestone changed from 5.9 to 6.0

As per today's bug scrub, let's move this ticket to the next milestone for proper review/refresh.

@costdev
4 years ago

Patch refreshed against trunk. Added missing {} in if statements, removed extraneous semi-colon + PHPCS fixes.

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


4 years ago

#9 follow-up: @peterwilsoncc
4 years ago

  • Keywords needs-refresh added

I created a pull request based in 33888.3.diff to get the tests running but there are few errors and failures that will need a little investigation.

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


4 years ago

peterwilsoncc commented on PR #2538:


4 years ago
#11

Closing this as it was just to get tests running.

#12 @peterwilsoncc
4 years ago

  • Milestone changed from 6.0 to Future Release

Per discussion in recent bug scrub, I'm moving this off 6.0.

Props @costdev, @chaion07

This ticket was mentioned in Slack in #core-test by sirlouen. View the logs.


6 months ago

@mosescursor
6 months ago

Changed from $header to $type so declared post types are correctly applied.

#14 in reply to: ↑ 9 @mosescursor
6 months ago

Replying to peterwilsoncc:

I created a pull request based in 33888.3.diff to get the tests running but there are few errors and failures that will need a little investigation.

Investigated and found the errors and fixed them with this patch https://core.trac.wordpress.org/attachment/ticket/33388/33888.4.diff

Test and see

#15 @mosescursor
6 months ago

  • Keywords needs-unit-tests added
  • Resolution set to invalid
  • Status changed from reviewing to closed

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested:
33888.4.diff​ (1.3 KB) - added by mosescursor 8 minutes ago.
Patch fixes the incorrect variable check when reading the "Template Post Type" header in theme files. Changed from $header to $type so declared post types are correctly applied.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.29
  • Server: nginx/1.29.0
  • Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 138.0.0.0
  • OS: macOS
  • Theme: Standard Template Theme 1.0, Legacy Template Theme 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

Additional Notes

After applying the patch, the template appears for the specified post types as expected.

Supplemental Artifacts

#16 @swissspidy
2 months ago

  • Milestone Future Release deleted

Removing milestone from closed ticket.

Note: See TracTickets for help on using tickets.