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: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (35)
This ticket was mentioned in PR #6574 on WordPress/wordpress-develop by @WPExplorer.
19 months ago
#1
- Keywords has-patch added
#2
@
19 months ago
- Focuses administration added
Hi @wpexplorer! 👋
Thank you for your contribution to WordPress! 💖
#3
@
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
@
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.
@
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
#5
@
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
@
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
@
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.
#9
@
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
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
9 months ago
#14
@
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
@
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
@
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
- Added the provided code:
<?php add_action( 'export_wp', function() { add_filter( 'get_comment', '__return_false' ); } );
- Go to Tools > Export > Download Export File (for Posts, Categories: All, Authors: All, Status: All, No dates selected)
- ❌ I attach the resulting XML, full of Warnings.
Actual Results
- ❌ Can't reproduce this problem
Additional Notes
- Note that I'm using Xdebug for debugging (this is why warnings in the resulting file, show xdebug errors).
- This XML file can be opened as an HTML to better see all the errors
- @WPExplorer are you confident that this provided snippet works?
- @abcd95 have you been able to reproduce it?
Supplemental Artifacts
#18
@
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
#19
follow-up:
↓ 20
@
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
@
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:
↓ 22
@
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.
#22
in reply to:
↑ 21
@
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".
Oh, you meant changes-requested for the tests 👍. I'm going to fix this.
cc @abcd95
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
@
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
@
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
@
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.
#31
@
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 ) { 240 240 * 241 241 * @since 2.3.0 242 242 * 243 * @param WP_Comment $_comment Comment data.243 * @param WP_Comment|null $_comment Comment data. 244 244 */ 245 245 $_comment = apply_filters( 'get_comment', $_comment ); 246 if ( ! ( $_comment instanceof WP_Comment ) ) { 247 return null; 248 } 246 249 247 250 if ( OBJECT === $output ) { 248 251 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.
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.