Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#61681 closed defect (bug) (fixed)

PHP Fatal error: Uncaught Error: Object of class WP_Comment could not be converted to string

Reported by: ambrosiawt Owned by: hellofromTonya
Priority: high Milestone: 6.6.1
Component: Comments Version: 6.6
Severity: normal Keywords: has-patch has-unit-tests commit dev-reviewed fixed-major
Cc: Focuses:

Description

Error message is: PHP Fatal error: Uncaught Error: Object of class WP_Comment could not be converted to string in /var/web/site/public_html/wp-includes/comment-template.php:27

The problem appears to be that in some cases, code in comment-template.php is calling get_comment_author() in comment-template.php which expects an optional parameter that is used as the comment_ID but is being passed a WP_Comment object instead at times. Discovered the issue when trying to edit a Divi template and the builder hung, and found the errors in the log. I do have a proposed patch attached.

Attachments (1)

comment-template.patch​ (1.8 KB ) - added by ambrosiawt 2 years ago.

Download all attachments as: .zip

Change History (25)

#1 @hellofromTonya
2 years ago

  • Milestone Awaiting Review → 6.6.1

Hello @ambrosiawt,

Welcome to WordPress Core Trac. Thank you for reporting this issue.

[58335] introduced the (string) type casting change. It triggers when the WP_Comment instance's comment_ID is not empty (see the code here).

function get_comment_author( $comment_id = 0 ) {
	        $comment = get_comment( $comment_id );
	
	        $comment_id = ! empty( $comment->comment_ID ) ? $comment->comment_ID : (string) $comment_id;

At first glance:

  • I'm not seeing how the comment object's comment_ID property isn't initialized (not empty) at this point.
  • If the incoming $comment_id is an instance of WP_Comment, passing it to get_comment() should also return the object.
  • Thus, if it's an object and it has an ID, then it should fall into the ? branch.

So how and why is it falling into the else branch (i.e. running this (string) $comment_id)?

Hmm.

Thinking the place to start is in understanding how, when, and why an object passed into get_comment() may not have its comment_ID assigned / initialized.

Once that's understood, then the root cause may be elsewhere.

I'm pulling this into 6.6.1 milestone for awareness and investigation.

#2 @hellofromTonya
2 years ago

Oh wait, what if an instance of WP_Comment is instantiated before it has an assigned ID?

The ID is auto-assigned in the database. So if a new comment is instantiated but not yet saved in the database, the comment_ID property should be empty.

Testing this theory and writing tests to show it.

Last edited 2 years ago by hellofromTonya (previous) (diff)

This ticket was mentioned in ​PR #7053 on ​WordPress/wordpress-develop by ​@hellofromTonya.


2 years ago
#3

  • Keywords has-patch has-unit-tests added

If an instance of WP_Comment is passed to get_comment_author() without an ID (either before the comment is saved in the database or somehow the ID is reset), a fatal error can happen.

Trac ticket: https://core.trac.wordpress.org/ticket/61681

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


2 years ago

#5 follow-up: @hellofromTonya
2 years ago

Patch: ​https://github.com/WordPress/wordpress-develop/pull/7053

This patch:

  1. First adds tests to show the fatal error happening in get_comment_author() when the incoming WP_Comment object does not have a comment ID (for whatever reason) or that property is empty.
  2. Second adds the fix, which (a) defensively guards the string type casting (string) $comment_id for when $comment_id is scalar and (b) if neither path is true, sets it an empty string as a default.

The $comment_id is not used for getting the comment author; rather it was added to pass to the filter.

This patch fixes the fatal error. But what is passed to the filter would no longer be an object. I don't think it's a BC break in that the $comment_id passing in the filter is documented to a string data type, not an instance of WP_Comment.

@ambrosiawt are you able to test the patch to see if resolves the issue?

#6 in reply to: ↑ 5 @ambrosiawt
2 years ago

Replying to hellofromTonya:

Patch: ​https://github.com/WordPress/wordpress-develop/pull/7053

...

@ambrosiawt are you able to test the patch to see if resolves the issue?

Yes, I will test it shortly. Thanks!

#7 @ambrosiawt
2 years ago

For what it's worth, I did add some logging before submitting the ticket to validate that sometimes there is an object present, and in those situations you are correct, it did have an empty comment_ID in the object.

#8 @ambrosiawt
2 years ago

I tested this in my failure situation and the problem is not occurring now.

#9 @hellofromTonya
2 years ago

  • Priority normal → high

That's awesome @ambrosiawt. Thank you for confirming.

I'm elevating the priority on this ticket to high for better tracking as it's one of the drivers for 6.6.1.

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


2 years ago

​@jorbin commented on ​PR #7053:


2 years ago
#11

Since I needed to search for them, noting that the test failures from before the second commit are in: ​https://github.com/WordPress/wordpress-develop/actions/runs/9980970593

#12 @hellofromTonya
2 years ago

  • Keywords commit added
  • Owner set to hellofromTonya
  • Status new → reviewing

Patch: ​https://github.com/WordPress/wordpress-develop/pull/7053

@jorbin reviewed and approved.

Marking for commit.

#13 @hellofromTonya
2 years ago

  • Resolution → fixed
  • Status reviewing → closed

In 58755:

Comments: Fix fatal error when get_comment_author() receives an object with no comment_id.

[58335] introduced (string) type casting of the passed in $comment_id value. If $comment_id is a scalar, it works as expected. But if it's an object, the following fatal error is thrown:

Object of class WP_Comment could not be converted to string

This fatal error happens when the incoming $comment_id is an instance of WP_Comment (or any object) without a comment_ID (empty).

This changeset adds tests to demonstrate the fatal error and validate the fix.

It fixes the fatal error by restructuring the ternary checks into an if/elseif/else structure for the 3 paths:

  • When $comment->comment_ID is not empty, then it uses the property.
  • When $comment_id is scalar, then it type casts it to a string.
  • Else, the default is an empty string.

Follow-up to [58335], [41127], [52818].

Props ambrosiawt, hellofromTonya, jorbin, mukesh27, SergeyBiryukov.
Fixes #61681.

#14 @hellofromTonya
2 years ago

In 58756:

Coding Standards: Capitalize inline comment in get_comment_author() test dataset.

Per coding standards, capitalizes the first character of the inline comment in the test dataset.

Follow-up to [58755].

Props SergeyBiryukov.
See #61681.

​@hellofromTonya commented on ​PR #7053:


2 years ago
#15

Committed via https://core.trac.wordpress.org/changeset/58755. Thank you everyone 🎉

#16 @hellofromTonya
2 years ago

  • Keywords dev-feedback added
  • Resolution fixed
  • Status closed → reopened

Reopening for 2nd committer sign-off to backport [58755] and [58756] to the 6.6 branch for 6.6.1.

#17 @SergeyBiryukov
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good to backport 👍

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


2 years ago

#19 @jorbin
2 years ago

[58755] and [58756] both look good to backport

#20 @hellofromTonya
2 years ago

Awesome. Thank you both. Backporting them now.

#21 @hellofromTonya
2 years ago

  • Resolution → fixed
  • Status reopened → closed

In 58762:

Comments: Fix fatal error when get_comment_author() receives an object with no comment_id.

[58335] introduced (string) type casting of the passed in $comment_id value. If $comment_id is a scalar, it works as expected. But if it's an object, the following fatal error is thrown:

Object of class WP_Comment could not be converted to string

This fatal error happens when the incoming $comment_id is an instance of WP_Comment (or any object) without a comment_ID (empty).

This changeset adds tests to demonstrate the fatal error and validate the fix.

It fixes the fatal error by restructuring the ternary checks into an if/elseif/else structure for the 3 paths:

  • When $comment->comment_ID is not empty, then it uses the property.
  • When $comment_id is scalar, then it type casts it to a string.
  • Else, the default is an empty string.

Follow-up to [58335], [41127], [52818].

Reviewed by SergeyBiryukov, jorbin.
Merges [58755,58756] to the 6.6 branch.

Props ambrosiawt, hellofromTonya, jorbin, mukesh27, SergeyBiryukov.
Fixes #61681.

#22 @jorbin
2 years ago

In 58763:

General: Provide _is_utf8_charset() in compat.php for early use

#61182 introduced is_utf8_charset() as a way of standardizing checks for charset slugs referring to UTF-8. This is called by _mb_strlen() inside of compat.php, but is_utf8_charset() is defined in functions.php, which isn't loaded early on. Code calling mb_strlen() early on before functions.php loads in hosts without the multibyte extension therefore may crash.

Props dmsnell, jonsurrell, joemcgill, jorbin.
Fixes #61681.

#23 @jorbin
2 years ago

The above commit should have been to #61680. Sorry future code archeologists.

#24 @hellofromTonya
2 years ago

  • Keywords fixed-major added
Note: See TracTickets for help on using tickets.