Make WordPress Core

Opened 19 months ago

Last modified 7 days ago

#61244 reviewing defect (bug)

The export_wp() function inserts empty <wp:comment> data when get_comment() returns a false or empty value

Reported by: wpexplorer's profile WPExplorer Owned by: westonruter's profile westonruter
Milestone: 7.0 Priority: normal
Severity: minor Version: 4.4
Component: Export Keywords: has-test-info has-patch needs-testing changes-requested
Focuses: administration Cc:

Description

If you wish to exclude comments from the WordPress export you can use the following code:

add_action( 'export_wp', function() {
    add_filter( 'get_comment', '__return_false' );
} );

However, the function will still loop through empty values for the $comments array inserting unnecessary <wp:comment> items.

Attachments (3)

Screen Shot 2024-09-17 at 3.00.51 PM.png (329.5 KB) - added by lbones 15 months ago.
This is the test that was done using the patch [from PR=6574] which was PHP 8.2 and WP 6.6.2
Screen Shot 2024-09-17 at 3.14.49 PM.png (262.2 KB) - added by lbones 15 months ago.
Another view of the issue in the patch [with PR=6574] using PHP 8.2 and WP 6.6.2
resultingxml.xml (58.6 KB) - added by SirLouen 8 months ago.

Download all attachments as: .zip

Change History (35)

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


19 months ago
#1

  • Keywords has-patch added

If you wish to exclude comments from the WordPress export you can use the following code:

add_action( 'export_wp', function() {
    add_filter( 'get_comment', '__return_false' );
} );

However, the function will still loop through empty values for the $comments array inserting unnecessary <wp:comment> items.

#2 @freewebmentor
19 months ago

  • Focuses administration added

Hi @wpexplorer! 👋

Thank you for your contribution to WordPress! 💖

#3 @hellofromTonya
15 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.7
  • Version changed from 6.6 to 4.4

While scrubbing for 6.6.x minor, dove into which WordPress Version introduced this bug and code. It was in WP 4.4 via [33891].

As it has a clear explanation and patch, moving it into 6.7 cycle for possible resolution.

#4 @lbones
15 months ago

I am working on testing this one! I am planning on recreating the issue in WP 4.4, and then running the patch to verify if the issue has been resolved. I am using the WordPress Playground, but can test through the CLI or whatever you may need.

In the PlayGround, I did notice that the user is not able to enter an empty comment. To recreate, I wrote a comment, and then went into the WordPress editor and removed the text in the comment.

Version 0, edited 15 months ago by lbones (next)

@lbones
15 months ago

This is the test that was done using the patch [from PR=6574] which was PHP 8.2 and WP 6.6.2

@lbones
15 months ago

Another view of the issue in the patch [with PR=6574] using PHP 8.2 and WP 6.6.2

#5 @lbones
15 months ago

Initially, I thought that the issue was that the comments were coming through whether or not they contained content. I realized upon further investigation that the issue, is if the person wants to run

add_action( 'export_wp', function() {
    add_filter( 'get_comment', '__return_false' );
} );

then the patch, which contains an array_filter does work. Recommendation would be to add an export option without comments instead of the current patch.

Reasoning: This very niche solution makes the entire get_comment call in the wp_export function slower. The array_filter in combination with array_map creates two maps (could probably have a map_reduce like function), and with the for loop, we are going through all of the comments thrice.

P.S. I plan on opening a new ticket with the commenting editing issue I found. I believe the checks on modify_item in the comment controller just needs to be a little more robust.

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


14 months ago

#7 @stoyangeorgiev
14 months ago

  • Milestone changed from 6.7 to 6.8

This one was discussed at a bug-scrub. By the looks of it this one needs a bit more time, and with RC1 right around the corner, moving this one to 6.8. Cheers!

#8 @mdibrahimk48
12 months ago

Test Report:

The applied patch [PR=6574] shows no changes in functionality when tested with PHP 8.3 and WordPress 6.7.1.

Observation:


Attempting to submit an empty column does not work. However, when using the WordPress editor, removing text in a comment is possible without issues.

Last edited 12 months ago by mdibrahimk48 (previous) (diff)

#9 @desrosj
10 months ago

  • Keywords needs-unit-tests added

This looks good, but I'd love to see some unit tests that demonstrate the failure and confirming that the change fixes the issue.

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


10 months ago
#10

  • Keywords has-unit-tests added; needs-unit-tests removed

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

This PR adds unit tests for the patch present to verify the behavior of comment filtering during exports.

The new test ensures that when comments are filtered out using get_comment filter, no empty <wp:comment> tags appear in the export XML.

Run the tests:

npm run test:php -- --filter=Tests_Admin_ExportWp

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


9 months ago

#12 @audrasjb
9 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

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


9 months ago

#14 @saurabh.dhariwal
9 months ago

  • Keywords has-testing-info added

WordPress Version 6.7
Issue Summary

Problem: Previously, the system allowed users to submit blank comments on posts and pages.
Expected Behavior: The system should prevent users from submitting empty comments.

Testing Steps & Results

Navigate to a post/page and open the comment section. ✅
Attempt to submit a blank comment. ✅
Observe the system’s response. ✅

Result: The issue could not be reproduced. The system correctly prevents blank comments from being submitted.

Test Conclusion

Status: ✅ Not Reproduced
Remarks: The issue appears to be resolved or was not present in the tested environment. Further verification may be needed if reported again.

#15 @audrasjb
9 months ago

  • Keywords has-testing-info removed
  • Milestone changed from 6.8 to 6.9

Moving to 6.9 as we're too close from beta 3 and this still needs testing.

#16 @SirLouen
8 months ago

  • Keywords reporter-feedback needs-testing-info added; has-patch needs-testing has-unit-tests removed

Bug Reproduction Report

Description

❌ This report can't validate that issue can be reproduced.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/8383.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 135.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.2
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Bug Reprodution Steps

  1. Added the provided code:
<?php
add_action( 'export_wp', function() {
    add_filter( 'get_comment', '__return_false' );
} );
  1. Go to Tools > Export > Download Export File (for Posts, Categories: All, Authors: All, Status: All, No dates selected)
  1. ❌ I attach the resulting XML, full of Warnings.

Actual Results

  1. ❌ Can't reproduce this problem

Additional Notes

  1. Note that I'm using Xdebug for debugging (this is why warnings in the resulting file, show xdebug errors).
  2. This XML file can be opened as an HTML to better see all the errors
  3. @WPExplorer are you confident that this provided snippet works?
  4. @abcd95 have you been able to reproduce it?

Supplemental Artifacts

Last edited 8 months ago by SirLouen (previous) (diff)

#17 @wordpressdotorg
7 months ago

  • Keywords needs-test-info added; needs-testing-info removed

#18 @SirLouen
6 months ago

  • Keywords close added

Also noting as commented, that apart that I'm completely unable to reproduce this "filter" as is, this will bring some additional performance issues.

Personally I would consider here a close as wontfix

Last edited 6 months ago by SirLouen (previous) (diff)

#19 follow-up: @jorbin
5 months ago

  • Keywords needs-refresh added; close removed

While the patch might not be exactly what it is needed, this is a problem that should be solved.

#20 in reply to: ↑ 19 @SirLouen
5 months ago

Replying to jorbin:

While the patch might not be exactly what it is needed, this is a problem that should be solved.

Have you been able to reproduce it? Can you provide the steps on how you did?

What needs refresh?

#21 follow-up: @jorbin
5 months ago

To reproduce I made an mu-plugin with the code from the ticket description

<?php
add_action( 'export_wp', function() {
    add_filter( 'get_comment', '__return_false' );
} );

I then used wp-cli to run export_wp() and observed that it contains an empty comment.

The refresh is described on the PR.

#22 in reply to: ↑ 21 @SirLouen
5 months ago

  • Keywords changes-requested has-test-info has-patch added; reporter-feedback needs-test-info needs-refresh removed

Replying to jorbin:

To reproduce I made an mu-plugin with the code from the ticket description

Ok, I can remember now what I was thinking back in the day:

All the warnings are being caused by an unexpected value (the false)

When I first reviewed get_comment I could not understand why the person was returning a false because get_comment is not expecting to return a bool value:

@return WP_Comment|array|null Depends on $output value.

But a null at best.

There is no sanitization of what the user can return after the filter and what gets to the next conditional block (which can end returning whatever value has been passed through the filter).

So here I was thinking: "If false was being used is normal that everything completely fails".

But after your comment, I'm rethinking this: if he had used __return_null instead, he could also expect to wipe all comments from export. And since null and false might behave similarly with array_filter then we could expect the same result whatever the formula we use (despite it's a little out of the boundaries)

So I wonder if:
A) get_comment should also be reviewed to accept a bool return, or
B) better sanitize the "filtered comment".

The refresh is described on the PR.

Oh, you meant changes-requested for the tests 👍. I'm going to fix this.
cc @abcd95

@abcd95 commented on PR #8383:


5 months ago
#23

can we utilize the get_the_export() method

Yes, I completely missed that. Thanks for pointing out.

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


5 weeks ago

#25 @wildworks
5 weeks ago

  • Keywords needs-testing added; changes-requested removed

This ticket was brought up in today's bug scrub. I think the PR is in good shape, and I would like to suggest at least removing changes-requested and adding needs-testing.

If there is no Activity, I will punt it to 7.0 milestone.

This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.


4 weeks ago

#27 @mabfahad
4 weeks ago

I tested this with the latest WordPress version (6.8.3) on a fresh installation.
I created several posts with comments and also deleted some comments directly from the database to simulate missing data.

In all my tests, get_comment() did not return false, and no empty <wp:comment> elements were found in the exported WXR file.

It appears that this issue no longer occurs in the current version of WordPress.

#28 @wildworks
4 weeks ago

  • Milestone changed from 6.9 to 7.0

Since the RC1 release is approaching, I'd like to punt this ticket to 7.0.

It appears that this issue no longer occurs in the current version of WordPress.

@mabfahad, I think this is the correct procedure to reproduce the problem.

https://core.trac.wordpress.org/ticket/61244#comment:22

@abcd95 commented on PR #8383:


7 days ago
#29

I think the failing test is unrelated to the changes in the PR.

#30 @westonruter
7 days ago

  • Owner changed from audrasjb to westonruter

#31 @westonruter
7 days ago

  • Keywords changes-requested added

As part of this, the get_comment filter should also be updated to indicate that it can return null as well:

  • src/wp-includes/comment.php

    a b function get_comment( $comment = null, $output = OBJECT ) { 
    240240         *
    241241         * @since 2.3.0
    242242         *
    243          * @param WP_Comment $_comment Comment data.
     243         * @param WP_Comment|null $_comment Comment data.
    244244         */
    245245        $_comment = apply_filters( 'get_comment', $_comment );
     246        if ( ! ( $_comment instanceof WP_Comment ) ) {
     247                return null;
     248        }
    246249
    247250        if ( OBJECT === $output ) {
    248251                return $_comment;

Otherwise, if someone tries to return null (or false) in a callback to the get_comment filter, then technically they are violating the filter's allowed types.

Note: See TracTickets for help on using tickets.