WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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)

36578.diff (583 bytes) - added by joemcgill 4 years ago.
36578.2.diff (4.6 KB) - added by swissspidy 4 years ago.
Patch including ajax tests to ensure the bug is fixed
36578.3.diff (5.1 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (22)

@joemcgill
4 years ago

#1 @joemcgill
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.6
  • Owner set to joemcgill
  • Status changed from new to assigned

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.

#2 follow-up: @azaozz
4 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.

Version 0, edited 4 years ago by azaozz (next)

#3 @swissspidy
4 years ago

  • Keywords has-unit-tests added

#4 in reply to: ↑ 2 @swissspidy
4 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 :)

Last edited 4 years ago by swissspidy (previous) (diff)

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


4 years ago

@swissspidy
4 years ago

Patch including ajax tests to ensure the bug is fixed

#6 @swissspidy
4 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.


4 years ago

#8 @swissspidy
4 years ago

Note: Some comments ajax tests are currently failing with 36578.2.diff applied, which #36616 fixes. Looking into it.

#9 @boonebgorges
4 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
4 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?

Edit: Of course I meant #35199 and not #36616.

Last edited 4 years ago by swissspidy (previous) (diff)

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


4 years ago

#12 @ocean90
4 years ago

#36626 was marked as a duplicate.

#13 @boonebgorges
4 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
4 years ago

#14 @swissspidy
4 years ago

@boonebgorges Thanks for sharing. That actually makes the most sense!

#15 @ocean90
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 37288:

Media: Remove an extra quote when sending a link of a media file to the editor.

Introduced in [37035].

Props joemcgill, swissspidy, boonebgorges.
Fixes #36578.

#16 @ocean90
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#17 @ocean90
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 37289:

Media: Remove an extra quote when sending a link of a media file to the editor.

Introduced in [37035].

Merge of [37288] to the 4.5 branch.

Props joemcgill, swissspidy, boonebgorges.
Fixes #36578.

#18 @swissspidy
4 years ago

#36632 was marked as a duplicate.

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


4 years ago

Note: See TracTickets for help on using tickets.