Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#53091 closed defect (bug) (fixed)

[Twenty Twenty-One] Incorrect use of `the_password_form` filter argument

Reported by: burhandodhy's profile burhandodhy Owned by: davidbaumwald's profile davidbaumwald
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.6
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:

Description (last modified by davidbaumwald)

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.

3 years ago

  • Keywords has-patch added

#2 in reply to: ↑ description @burhandodhy
3 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

PR link:

#3 @davidbaumwald
3 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 @mukesh27
3 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.

#5 @burhandodhy
3 years ago

@mukesh27 feedback implemented.

#6 @SergeyBiryukov
3 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.

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

#7 @davidbaumwald
3 years ago

  • Owner set to davidbaumwald
  • Status changed from new to accepted

#8 @mukesh27
3 years ago

As per @SergeyBiryukov quick update and Changeset 50791 we have to change the patch.

#9 @burhandodhy
3 years ago

@mukesh27 changes are done.

#10 follow-up: @davidbaumwald
3 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 @SergeyBiryukov
3 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 @davidbaumwald
3 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?

#13 @burhandodhy
3 years ago

@davidbaumwald I have updated my PR.

#14 @mukesh27
3 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?

#15 @burhandodhy
3 years ago

@mukesh27 this patch depends on patch. So this will probably go with 5.8

Version 0, edited 3 years ago by burhandodhy (next)

#16 @SergeyBiryukov
3 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 :)

This ticket was mentioned in Slack in #core-themes by burhandodhy. View the logs.

3 years ago

#18 @burhandodhy
3 years ago

Any update on this ?

cc: @SergeyBiryukov @davidbaumwald @mukesh27

#19 @SergeyBiryukov
3 years ago

  • Keywords commit added

Marking for commit.

#20 @davidbaumwald
3 years ago

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

In 50841:

Bundled Theme: Update twenty_twenty_one_password_form function to actually use a $post parameter.

The twentytwentyone filtered the_password_form with a twenty_twenty_one_password_form callback that, by default, passed only one parameter that was assumed to be the post or post ID. However, the first parameter for the_password_form is the filtered output value. This fix updates both the filter reference and callback to use two parameters: $output and $post.

Props burhandodhy, mukesh27.
Fixes #53091.

Note: See TracTickets for help on using tickets.