Make WordPress Core

Opened 2 years ago

Closed 2 years ago

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

Failed wp_insert_category() when IMPORT_DEBUG is set to true produces a fatal error.

Reported by: enshrined's profile enshrined Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Import Keywords: has-patch
Focuses: Cc:

Description

When calling wp_insert_category(), setting the second parameter to true lets us return a WP_Error. In this case, we don't, so 0 is returned in the case of a failure.

If a WP_Error or 0 value is returned, it will echo that it failed to import the category.
If we also have IMPORT_DEBUG set to true, it will try and fetch the error message from the returned value. This is fine, if we're returning a WP_Error, but in the case that it returns 0, it causes a fatal error.

See: https://github.com/WordPress/wordpress-importer/blob/99462ec2d3390bc0de3bc5ffbe817f2160e5ef54/src/class-wp-import.php#L426-L438

Fatal error: Uncaught Error: Call to a member function get_error_message() on int

I suggest that we add an additional check, to see if we have a WP_Error, before trying to render the error message:

$id = wp_insert_category( $data );
if ( ! is_wp_error( $id ) && $id > 0 ) {
        if ( isset( $cat['term_id'] ) ) {
                $this->processed_terms[ intval( $cat['term_id'] ) ] = $id;
        }
} else {
        printf( __( 'Failed to import category %s', 'wordpress-importer' ), esc_html( $cat['category_nicename'] ) );
        if ( defined( 'IMPORT_DEBUG' ) && IMPORT_DEBUG && is_wp_error( $id ) ) {
                echo ': ' . $id->get_error_message();
        }
        echo '<br />';
        continue;
}

Attachments (2)

54973.1.patch (550 bytes) - added by enshrined 2 years ago.
Adding patch
54973.2.patch (510 bytes) - added by enshrined 2 years ago.

Download all attachments as: .zip

Change History (11)

@enshrined
2 years ago

Adding patch

#1 @enshrined
2 years ago

  • Keywords has-patch added; needs-patch removed

Added a patch that does a is_wp_error() check before trying to access the get_error_message() method on it.

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

#4 in reply to: ↑ 3 @SergeyBiryukov
2 years ago

  • Milestone changed from 6.0 to WordPress.org

Replying to mukesh27:

Hi there! thanks for the ticket and patch.

Can you please add is_wp_error for below all occurrences so we don't get a similar error in any other code base?

Thanks for the review! Unless I'm missing something, all of those instances already have an is_wp_error() check just a few lines above, so adding another check would be redundant.

The instance in the patch is different because it gets executed not only if $id is a WP_Error object, but also if it's 0:

$id = wp_insert_category( $data );
if ( ! is_wp_error( $id ) && $id > 0 ) {
	...
} else {
	printf( __( 'Failed to import category %s', 'wordpress-importer' ), esc_html( $cat['category_nicename'] ) );
	if ( defined( 'IMPORT_DEBUG' ) && IMPORT_DEBUG ) {
		echo ': ' . $id->get_error_message(); // The instance being patched here.
	}
	echo '<br />';
}

Instead of adding another is_wp_error() check, I think the correct fix here would be to pass true as the second parameter to wp_insert_category(), otherwise it can never return a WP_Error in the first place.

That would also be consistent with wp_insert_post( $postdata, true ) in line 746.

Moving the ticket to the WordPress.org milestone instead of 6.0, as this code is in the WordPress importer plugin and not in WordPress core.

#5 @azouamauriac
2 years ago

I question myself about the use of '0' returned by wp_insert_category, someone can help me?

Also I think it would be better to add 'true' as second arg to wp_insert_category( $data ) that will become wp_insert_category( $data, true );

@enshrined
2 years ago

#6 @enshrined
2 years ago

  • Keywords needs-refresh removed

@SergeyBiryukov is correct; as far as I can see, this error won't occur with any of the other instances as they return WP_Error objects.

I've attached a new patch that means the wp_insert_category() call returns a WP_Error object. I agree; this approach makes more sense as it's more consistent with the rest of the script.

#7 @ocean90
2 years ago

@enshrined Could you please open an issue and pull request on https://github.com/WordPress/wordpress-importer for this change? This Trac isn’t really the best place for the WordPress Importer anymore. Thank you!

#9 @ocean90
2 years ago

  • Milestone WordPress.org deleted
  • Resolution set to reported-upstream
  • Status changed from new to closed

Thank you! Closing as reported-upstream.

Note: See TracTickets for help on using tickets.