Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#46679 closed defect (bug) (fixed)

Themes: Add a shim for wp_body_open()

Reported by: pento's profile pento Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:

Description

Split off from #12563.

WordPress 5.2 adds the wp_body_open() function, and the default themes make use of it.

They don't have a shim for this function, however, so will trigger fatal errors if they're run on an older version of WordPress.

Attachments (3)

46679.patch (4.2 KB) - added by ramiy 6 years ago.
46679-back-comp.patch (6.8 KB) - added by ramiy 6 years ago.
46679-back-comp.2.patch (6.8 KB) - added by ramiy 6 years ago.

Download all attachments as: .zip

Change History (24)

#1 @adamsilverstein
6 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#2 @lgedeon
6 years ago

Proposed solution to add this to each of the default themes.

<?php
if ( ! function_exists( 'wp_body_open' ) ) {
        function wp_body_open() {
                do_action( 'wp_body_open' );
        }
}

We might also add the doc block. Not sure on expectations there.

#3 follow-up: @johnbillion
6 years ago

  • Version set to trunk

What's the real purpose of the wp_body_open() wrapper function? It presents consistency with other functions such as wp_head() and wp_footer(), but would placing a direct call to do_action( 'wp_body_open' ) be so bad?

Last edited 6 years ago by johnbillion (previous) (diff)

#4 in reply to: ↑ 3 @timph
6 years ago

Replying to johnbillion:

What's the real purpose of the wp_body_open() wrapper function? It presents consistency with other functions such as wp_head() and wp_footer(), but would placing a direct call to do_action( 'wp_body_open' ) be so bad?

I understand the consistency part, and agree with this. Just adding that do_action( 'wp_body_open' ) is 100% more clear about what it actually does as well. It's frustrating when you have to search through code just to figure out something is only a wrapper around an action.

#5 @klihelp
6 years ago

Core functions looks consistent in the theme, and without any parameter. Needed action:)

wp_head();

wp_body_open();

wp_footer();

#6 @adamsilverstein
6 years ago

What's the real purpose of the wp_body_open() wrapper function?

Mainly this was added this way for consistency with other function calls/existing pattern for header/footer calls. It was discussed a bit in #12563. I do see how switching to triggering an action directly would remove the need for a shim though, which we didn;t consider previously.

This ticket was mentioned in Slack in #themereview by tim. View the logs.


6 years ago

#8 @ramiy
6 years ago

The default themes, and any other theme should use:

<?php if ( function_exists( 'wp_body_open' ) ) wp_body_open(); ?>

instead of just:

<?php wp_body_open(); ?>

Unless the theme "Minimum Required WordPress Version" is 5.2+.

@ramiy
6 years ago

#9 @ramiy
6 years ago

  • Keywords has-patch added; needs-patch removed

#10 @ramiy
6 years ago

The second patch implements the solution suggested by @lgedeon.

Note that some themes has a separate file with backward compatibility functions (inc/back-compat.php). In other themes the code implemented in functions.php.

#11 @adamsilverstein
6 years ago

  • Keywords 2nd-opinion added

Thanks for the patches @ramiy! I like 46679-back-comp.patch - it brings the functionality wherever these themes are used, benefiting users.

Does this look good to you @pento?

#12 @pento
6 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

Yah, I like the direction of 46679-back-comp.patch. The function shouldn't go in inc/back-compat.php, though, as that file is only loaded on versions of WordPress where the theme won't work, and it'll prevent the theme from being activated. Where the file exists, lets put it in inc/template-tags.php.

To address the other options:

  • I think the wp_body_open() function call fits better with the wp_head() / wp_footer() pattern. Particularly as this is the actual function call that works in WP 5.2+, using a do_action() call solely for back compat is presenting a sub-optimal example for folks to base their themes off.
  • Adding a function_exists( 'wp_body_open' ) check is an unnecessary logic check in a template file, which should be avoided where possible.

#13 @ramiy
6 years ago

@pento Thanks for the feedback. I've updated the patch.

In 2010-2014 I used the functions.php file, In 2015-2019 I used the inc/template-tags.php file.

Also, I replaced if () { ... } with if () : ... endif;, to match the current code convention in theme files.

#14 @ramiy
6 years ago

  • Keywords needs-refresh removed

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


6 years ago

#16 @ramiy
6 years ago

@adamsilverstein @pento please review the new patch.

#17 @adamsilverstein
6 years ago

Thanks @ramiy - patch looks good to me.

Version 0, edited 6 years ago by adamsilverstein (next)

#18 @adamsilverstein
6 years ago

  • Keywords 2nd-opinion commit added

#19 @ramiy
6 years ago

@pento ?

#20 @pento
6 years ago

  • Keywords 2nd-opinion removed

Thanks, @ramiy! Patch looks good.

#21 @adamsilverstein
6 years ago

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

In 45256:

Bundled Theme: add a wp_body_open shim for older WordPress versions.

WordPress 5.2 adds the wp_body_open() function, and the default themes make use of it. This patch adds a shim for wp_body_open to bundled themes so this function will also work in older versions of WordPress.

Props lgedeon, johnbillion, timph, ramiy, pento.
Fixes #46679.

Note: See TracTickets for help on using tickets.