Opened 4 years ago
Closed 4 years ago
#53091 closed defect (bug) (fixed)
[Twenty Twenty-One] Incorrect use of `the_password_form` filter argument
Reported by: | burhandodhy | Owned by: | davidbaumwald |
---|---|---|---|
Milestone: | 5.8 | Priority: | normal |
Severity: | normal | Version: | 5.6 |
Component: | Bundled Theme | Keywords: | has-patch commit |
Focuses: | Cc: |
Description (last modified by )
the_password_form
filter function has only one parameter $output
that is html output of from. Where Twenty Twenty-One theme tries to use it as $post
object.
Change History (20)
This ticket was mentioned in PR #1208 on WordPress/wordpress-develop by burhandodhy.
4 years ago
#1
- Keywords has-patch added
#2
in reply to:
↑ description
@
4 years ago
Replying to burhandodhy:
the_password_form
filter function has only one parameter$output
that is hthml output of from. Where Twenty Twenty-One theme tries to use it as$post
object.
Filter definition https://core.trac.wordpress.org/browser/tags/5.7.1/src/wp-includes/post-template.php#L1740
PR link: https://github.com/WordPress/wordpress-develop/pull/1208
#3
@
4 years ago
- Component changed from General to Bundled Theme
- Description modified (diff)
- Milestone changed from Awaiting Review to 5.7.2
- Version set to 5.6
#4
@
4 years ago
- Keywords needs-refresh added
Hi there!
Thank you for the ticket and patch.
The PR #1208 looks fine to me.
Can you please add doc for global $post;
like below?
* @global WP_Post $post Global post object.
#6
@
4 years ago
Just noting that there should be an empty line between @global
and @return
and ideally after global $post
too, looks good to me otherwise.
On a related note, it might be a good idea to pass the $post
argument to the_password_form
filter. Looks like it was previously suggested in #29008, but never implemented. Reopening that ticket.
#8
@
4 years ago
As per @SergeyBiryukov quick update and Changeset 50791 we have to change the patch.
#10
follow-up:
↓ 11
@
4 years ago
- Keywords dev-feedback added; needs-refresh removed
@SergeyBiryukov With [50791] in place, what would you like to see done on this ticket?
Option 1: Add the global for $post
and adding to the docblock, with twenty_twenty_one_password_form
accepting no params.
Option 2: Update the filter to pass the second arg for the $post
and update twenty_twenty_one_password_form
to use both params.
#11
in reply to:
↑ 10
@
4 years ago
Replying to davidbaumwald:
With [50791] in place, what would you like to see done on this ticket?
Good question :) My initial thought was option 1, since we want the theme to remain compatible with WP 5.6 and 5.7.
But it looks like if we set a default value for $post
, that would also be compatible with older WP versions:
/** * Retrieve protected post password form content. * * @since Twenty Twenty-One 1.0 * @since Twenty Twenty-One 1.4 Corrected parameter name for `$output`, * added the `$post` parameter. * * @param string $output The password form HTML output. * @param int|WP_Post $post Optional. Post ID or WP_Post object. Default is global $post. * @return string HTML content for password form for password protected post. */ function twenty_twenty_one_password_form( $output, $post = 0 ) { $post = get_post( $post ); ... return $output; } add_filter( 'the_password_form', 'twenty_twenty_one_password_form', 10, 2 );
So both options are technically interchangeable. I guess option 2 might be a bit cleaner, as it keeps the current behavior on WP 5.6 and 5.7, and offers better compatibility with WP 5.8.
#12
@
4 years ago
- Keywords needs-refresh added; dev-feedback removed
@SergeyBiryukov Thanks! I like option #2 better as well.
@burhandodhy Do you can to re-work your PR to include Sergey's suggestions?
#14
@
4 years ago
Updated PR looks good to me.
@davidbaumwald Do we need to update Milestone to 5.8 because we do not update the bundled theme in minor versions or @SergeyBiryukov backport those [50791] these changes to the 5.7 branches?
#16
@
4 years ago
- Keywords needs-refresh removed
- Milestone changed from 5.7.2 to 5.8
Yes, a new theme version can technically be released earlier, but we might want to include some other fixes in the update as well, so I think this can be moved to 5.8. Thanks for the PR, it looks good to me too :)
Trac ticket:
https://core.trac.wordpress.org/ticket/53091#ticket