Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Comments: Fix 'sprintf requires more than 1 params' error #49054

Merged
merged 1 commit into from
Mar 14, 2023
Merged

Comments: Fix 'sprintf requires more than 1 params' error #49054

merged 1 commit into from
Mar 14, 2023

Conversation

aristath
Copy link
Member

What?

Fixes #49000

Why?

To avoid PHP errors. Please take a look at the report on #49000 for more details.

NOTE: The same error happens in WP-Core, so this will need to be backported to Core as well. cc @Mamaduka

@aristath aristath added [Type] Bug An existing feature does not function as intended Needs PHP backport Needs PHP backport to Core labels Mar 14, 2023
@aristath aristath requested a review from Mamaduka March 14, 2023 09:33
@Mamaduka Mamaduka added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop and removed Needs PHP backport Needs PHP backport to Core labels Mar 14, 2023
@aristath
Copy link
Member Author

Created an issue on WordPress's Core trac and added a patch: https://core.trac.wordpress.org/ticket/57919

@Mamaduka
Copy link
Member

@aristath, there's no need for a separate core ticket and PR. The PHP changes from the block-library package are synced via package updates.

@aristath aristath mentioned this pull request Mar 14, 2023
Closed
Mamaduka
Mamaduka approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the issue for me.

@Mamaduka Mamaduka changed the title sprintf requires more than 1 params Comments: Fix sprintf requires more than 1 params error Mar 14, 2023
@Mamaduka Mamaduka changed the title Comments: Fix sprintf requires more than 1 params error Comments: Fix 'sprintf requires more than 1 params' error Mar 14, 2023
@aristath
Copy link
Member Author

@aristath, there's no need for a separate core ticket and PR. The PHP changes from the block-library package are synced via package updates.

Ah OK, I didn't know if we need to backport this one manually since we're now at RC in Core. Good to know, thanks!

@aristath aristath enabled auto-merge (squash) March 14, 2023 09:49
costdev
costdev approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@costdev costdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aristath! Since the unnecessary sprintf() was removed, and there were no changes to the string, this LGTM! 👍

@aristath aristath merged commit 93a6791 into trunk Mar 14, 2023
@aristath aristath deleted the fix/49000 branch March 14, 2023 10:06
@github-actions github-actions bot added this to the Gutenberg 15.4 milestone Mar 14, 2023
@Mamaduka
Copy link
Member

I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 784081e

Mamaduka pushed a commit that referenced this pull request Mar 14, 2023
@aristath @Mamaduka
sprintf requires more than 1 params (#49054)
784081e
@Mamaduka Mamaduka removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Mar 14, 2023
@Mamaduka Mamaduka mentioned this pull request Mar 14, 2023
Closed
@hellofromtonya
Copy link
Contributor

This bug was introduced in 6.1, not in 6.2. Technically, this should not be backported during 6.2's RC cycle. Why? Only bugs and regressions introduced in the milestone and found during RC can be backported.

cc @Mamaduka

Mamaduka added a commit that referenced this pull request Mar 14, 2023
@Mamaduka
Revert "sprintf requires more than 1 params (#49054)"
4486a6b 
This reverts commit 784081e.
@Mamaduka
Copy link
Member

I just reverted PR from the wp/6.2 branch.

@hellofromtonya
Copy link
Contributor

Thank you @Mamaduka! This fix is slated for 6.2.1. It also needs automated tests added in Gutenberg and Core. See the details of the committer discussion here https://core.trac.wordpress.org/ticket/57883#comment:15.

@Mamaduka Mamaduka added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label May 5, 2023
Mamaduka pushed a commit that referenced this pull request May 8, 2023
@aristath @Mamaduka
sprintf requires more than 1 params (#49054)
5384ee8
@Mamaduka
Copy link
Member

Mamaduka commented May 8, 2023

I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 5384ee8

@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label May 8, 2023
@Mamaduka Mamaduka mentioned this pull request May 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costdev costdev costdev approved these changes

@Mamaduka Mamaduka Mamaduka approved these changes

@michalczaplinski michalczaplinski Awaiting requested review from michalczaplinski michalczaplinski is a code owner

Assignees
No one assigned
Labels
[Block] Comments Affects the Comments Block - formerly known as Comments Query Loop [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 15.4
Development

Successfully merging this pull request may close these issues.

Sprintf requires 2 arguments in gutenberg_block_core_comment_template_render_comments
4 participants
@aristath @Mamaduka @hellofromtonya @costdev