Make WordPress Core

Opened 10 months ago

Last modified 8 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: audrasjb's profile audrasjb
Milestone: 6.9 Priority: normal
Severity: minor Version: 4.4
Component: Export Keywords: has-patch needs-testing has-unit-tests
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 (2)

Screen Shot 2024-09-17 at 3.00.51 PM.png (329.5 KB) - added by lbones 6 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 6 months ago.
Another view of the issue in the patch [with PR=6574] using PHP 8.2 and WP 6.6.2

Download all attachments as: .zip

Change History (17)

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


10 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
10 months ago

  • Focuses administration added

Hi @wpexplorer! 👋

Thank you for your contribution to WordPress! 💖

#3 @hellofromTonya
7 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
6 months ago

I am working on testing this one! I am planning on recreating the issue in WP 6.6., 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.

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

@lbones
6 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
6 months ago

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

#5 @lbones
6 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.


5 months ago

#7 @stoyangeorgiev
5 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
3 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.

Version 0, edited 3 months ago by mdibrahimk48 (next)

#9 @desrosj
5 weeks 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.


4 weeks 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.


4 weeks ago

#12 @audrasjb
2 weeks 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 days ago

#14 @saurabh.dhariwal
8 days 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
8 days 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.

Note: See TracTickets for help on using tickets.