| CARVIEW |
#36578 closed defect (bug) (fixed)
wp_ajax_send_attachment_to_editor() bug
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 4.5.1 | Priority: | normal |
| Severity: | normal | Version: | 4.5 |
| Component: | Media | Keywords: | has-patch commit fixed-major |
| Focuses: | Cc: |
Description
there is a problem at wp_ajax_send_attachment_to_editor()
in the file ajax-actions.php in wp-admin/includes/
on line 2605
the link to editor comes with <a href="mediafile"">mediafile</a>
i fixed the file with:
Old one
$html = isset( $attachment['post_title'] ) ? $attachment['post_title'] : '';
$rel = $rel ? ' rel="attachment wp-att-' . $id . '"' : ''; // Hard-coded string, $id is already sanitized
if ( ! empty( $url ) ) {
$html = '<a href="' . esc_url( $url ) . '"' . $rel . '">' . $html . '</a>';
}
into new one
$rel = $rel ? ' rel="attachment wp-att-' . $id . '"' : ''; // Hard-coded string, $id is already sanitized
if ( ! empty( $url ) ) {
$html = '<a href="' . esc_url( $url ) . '"' . $rel . '>' . $html . '</a>';
}
the " before the '>' . $html was a doublicate
Attachments (3)
- 36578.diff (583 bytes) - added by joemcgill 10 years ago.
- 36578.2.diff (4.6 KB) - added by swissspidy 10 years ago.
- Patch including ajax tests to ensure the bug is fixed
- 36578.3.diff (5.1 KB) - added by boonebgorges 10 years ago.
Download all attachments as: .zip
Change History (22)
@joemcgill
10 years ago
- Attachment 36578.diff added
#1
@joemcgill
10 years ago
- Keywords has-patch added
- Milestone changed from Awaiting Review to 4.6
- Owner set to joemcgill
- Status changed from new to assigned
#2
follow-up:
↓ 4
@azaozz
10 years ago
- Milestone changed from 4.6 to 4.5.1
Ah I see. There is a typo in the tag. Seems TinyMCE is correcting it so nobody noticed until now?
Thinking this is minor but should be considered for 4.5.1 as well.
Wondering why the tests didn't catch that... :)
#3
@swissspidy
10 years ago
- Keywords has-unit-tests added
#4
in reply to:
↑ 2
@swissspidy
10 years ago
- Keywords commit added; has-unit-tests removed
Replying to azaozz:
Ah I see. There is a typo in the tag. Seems TinyMCE is correcting it so nobody noticed until now?
Thinking this is minor but should be considered for 4.5.1 as well.
Wondering why the tests didn't catch that... :)
That's because the tests are not for the ajax handler, where the bug resides :)
This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.
10 years ago
@swissspidy
10 years ago
- Attachment 36578.2.diff added
Patch including ajax tests to ensure the bug is fixed
#6
@swissspidy
10 years ago
As asked by @ocean90, I wrote some ajax tests in 36578.2.diff. Not sure if we'd backport these to 4.5 as well, but we can certainly use them for trunk.
This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.
10 years ago
#8
@swissspidy
10 years ago
Note: Some comments ajax tests are currently failing with 36578.2.diff applied, which #36616 fixes. Looking into it.
#9
@boonebgorges
10 years ago
The failures referenced by @swissspidy are pretty baffling to sort out. Briefly, the generation of user fixtures during ajax tests is happening in such a way that the WP_UnitTest_Generator_Sequence incrementor - which is intended to ensure uniqueness for user_login etc - is being incremented inconsistently or in an inconsistent order. I think that this has something to do with the fact that some AJAX tests are run in separate processes, along with some weirdness having to do with the way that PHPUnit loads setUpBeforeClass() early in the process of running tests.
In any case, a tweak to the way that the fixture incrementor works appears to resolve the problem. It also solves a developer-facing annoyance, discussed in #35199, which is that a generated user might have user_login "user_5" but user_email "user_6@…". https://core.trac.wordpress.org/attachment/ticket/35199/35199.diff should fix all of this.
#10
@swissspidy
10 years ago
Thanks a bunch for investigating, @boonebgorges! An improved incrementor in trunk will make the tests more robust.
Since the tests do not pass with 36578.2.diff applied, should we
a) only commit 36578.diff (without the tests) for 4.5.1 and 36578.2.diff for trunk, or
b) move #36616 to the 4.5.1 milestone and implement these together?
This ticket was mentioned in Slack in #core by ocean90. View the logs.
10 years ago
#13
@boonebgorges
10 years ago
Thinking this over, I think we should be conservative and not rush #35519 for 4.5.1. We can work around the current problem by explicitly providing unique user_login and user_email for the admin user required in these new tests. See 36578.3.diff.
@boonebgorges
10 years ago
- Attachment 36578.3.diff added
#14
@swissspidy
10 years ago
@boonebgorges Thanks for sharing. That actually makes the most sense!
#16
@ocean90
10 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
#18
@swissspidy
10 years ago
#36632 was marked as a duplicate.
Hi @otterstein,
Great catch. Thanks for taking the time to track down the solution as well.
This was introduced in [37035] and is fixed by 36578.diff.