Make WordPress Core

#57883 closed defect (bug) (reported-upstream)

block_core_comment_template_render_comments() fatal error

Reported by: gqevu6bsiz's profile gqevu6bsiz Owned by:
Milestone: Priority: normal
Severity: normal Version: 6.1
Component: Comments Keywords: has-patch gutenberg-merge has-screenshots
Focuses: Cc:

Description (last modified by hellofromTonya)

I got this error.

PHP Fatal error:  Uncaught ArgumentCountError: xxx arguments are required, 1 given in /wp-includes/blocks/comment-template.php:65

This error is when the following conditions are met.

  • WP: 6.1.1
  • Theme: Twenty Twenty Three
  • Permalink setting: Post name
  • And, WP blog have comments depth more than option the "Enable threaded (nested) comments xx levels deep"

(In other words, when "Enable threaded (nested) comments 2 levels deep" and WP blog have comments 3 depth)

This error code below

wp-includes/blocks/comment-template.php Line-65

$block_content .= sprintf( $inner_content );

Details of the cause

For example, using Japanese as the permalink, the permalink would look like this:

https://example.com/example-%e6%8a%95%e7%a8%bf

And $inner_content contains html with percent characters(post permalink).

sprintf does the formatting but gives an error because there are no arguments.

Reference:

Attachments (2)

no-problem-comment-depth.png (113.7 KB) - added by ocean90 14 months ago.
problem-comment-depth.png (151.8 KB) - added by ocean90 14 months ago.

Download all attachments as: .zip

Change History (20)

#1 @antonvlasenko
15 months ago

  • Keywords needs-patch added

Great catch!
Core should be scanned for similar cases as there are several other places where sprintf() and (similar functions, e.g. printf()) is called with only one argument (this can be done in the scope of this ticket).
Regarding the comment-template.php file, I think the fix should be submitted to the Gutenberg repo instead and then backported to Core.

Last edited 15 months ago by antonvlasenko (previous) (diff)

#2 @ocean90
14 months ago

#57919 was marked as a duplicate.

This ticket was mentioned in PR #4232 on WordPress/wordpress-develop by @aristath.


14 months ago
#3

  • Keywords has-patch added; needs-patch removed

#4 @ocean90
14 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 6.2

@audrasjb commented on PR #4232:


14 months ago
#5

Hey @aristath thanks for the PR but AFAIK, this file shouldn't be edited directly in Core. This probably has to be fixed on Gutenberg's side only and merged into Core via a package update.

@aristath commented on PR #4232:


14 months ago
#6

I wasn't clear on the process now that we're on RC, that's why I created the separate PR for Core... Just in case it's too late for automated package updates (there's a lot of confusion around what goes where, until when etc)

@Mamaduka commented on PR #4232:


14 months ago
#7

The changes will be part of the package update. We can close this PR and related Trac ticket.

#8 @hellofromTonya
14 months ago

  • Description modified (diff)
  • Keywords gutenberg-merge added

Bug introduced in #56467 / [54335] in a @worpdress package update during 6.1.0 Beta 2 cycle (back in September 2022).

With 6.2 in the RC phase, RC guidelines limit commits to only bugs and regressions introduced in 6.2 https://make.wordpress.org/core/2023/03/09/wordpress-6-2-release-candidate-phase/.

For the remainder of the cycle, only two types of tickets may be placed on/remain on the 6.2 milestone:

  • Regressions: bugs that have been introduced during the WordPress 6.2 development cycle, either to existing or new features.
  • Test suite expansion: tests can be committed at any time without regard to code or string freezes. This can cover either new or existing features.

That said, this is a fatal error. Since it's a fatal error, let's discuss if it should be fixed during RC or punted to 6.2.1.

Some thoughts:

  • The code change is minor with little to no risk.
  • But why hasn't been reported sooner? This makes me think it's a highly specific use case, meaning it could be okay to punt it to 6.2.1.

#9 @hellofromTonya
14 months ago

Strictly speaking per the RC guidelines, this should be punted to 6.2.1. But as I noted in comment 8, this is a fatal, though likely a specific use case.

@audrasjb what do you think?

Last edited 14 months ago by hellofromTonya (previous) (diff)

#10 @audrasjb
14 months ago

Given it's a known fatal error and as we have a patch for this I'd try to include it in RC2.
We can still revert in RC3 if needed, and punt to 6.2.1 but in my opinion, it's worth trying to remove the error now.

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


14 months ago

#12 @desrosj
14 months ago

  • Version changed from 6.1.1 to 6.1

#13 @desrosj
14 months ago

  • Keywords has-screenshots added
  • Version changed from 6.1 to 6.1.1

#14 @desrosj
14 months ago

  • Version changed from 6.1.1 to 6.1

#15 @hellofromTonya
14 months ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from 6.2 to 6.2.1

After discussing with Core Committers in Make/Core slack, there's consensus for the following:

  • Punt this fix to 6.2.1
  • Add tests in Gutenberg and Core for this fix

And for the question of if a fatal error from a previous release can be considered during a current RC cycle:

When in a RC cycle, fatal errors from a previous release can be considered on a case-by-case basis with open committer discussions (like the one started here today). BUT serious consideration should be given to whether that fatal error fix should go into the RC including minimum criteria of must-have automated tests and the risk assessments of the code changes.

Punting this ticket to 6.2.1.

Its fix will be part of a @wordpress packages update commit as the file in question is maintained in Gutenberg and not in Core.

Though reported upstream, keeping this ticket open due to the fatal error for tracking and awareness.

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


13 months ago

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


13 months ago

#18 @audrasjb
13 months ago

  • Keywords needs-testing needs-unit-tests removed
  • Milestone 6.2.1 deleted
  • Resolution set to reported-upstream
  • Status changed from new to closed

This should be fixed by [55737]. Closing as reported-upstream.

Note: See TracTickets for help on using tickets.