#36578 closed defect (bug) (fixed)
wp_ajax_send_attachment_to_editor() bug
Reported by: | otterstein | Owned by: | joemcgill |
---|---|---|---|
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)
Change History (22)
#1
@
9 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
@
9 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... :)
#4
in reply to:
↑ 2
@
9 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.
9 years ago
#6
@
9 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.
9 years ago
#8
@
9 years ago
Note: Some comments ajax tests are currently failing with 36578.2.diff applied, which #36616 fixes. Looking into it.
#9
@
9 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
@
9 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.
9 years ago
#13
@
9 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.
#16
@
9 years ago
- Keywords fixed-major added
- Resolution fixed deleted
- Status changed from closed to reopened
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.