Make WordPress Core

Opened 11 years ago

Closed 9 years ago

#23776 closed defect (bug) (fixed)

A string of text starting with "http://" turned into a paragraph

Reported by: winterstreet's profile winterstreet Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

In either editor, if there's a line of text that starts with http:// wordpress will create a new paragraph.

This description box isn't working to show what I mean see this support post.

http://wordpress.org/support/topic/a-string-of-text-starting-with-http-turned-into-a-paragraph

Attachments (3)

23776.patch (555 bytes) - added by UmeshSingla 10 years ago.
Fixes new line being inserted for a url, URL being not converted to Hyperlink
autoembed-preserve-whitespace.diff (951 bytes) - added by chipx86 9 years ago.
Preserves whitespace around URLs in the auto-embed code.
patch.diff (1.0 KB) - added by sgrant 9 years ago.

Download all attachments as: .zip

Change History (26)

#1 @winterstreet
11 years ago

  • Cc dave@… added

#2 @esmi
11 years ago

Confirmed using Twenty Twelve & no active plugins in both Text & Visual editors. The bug appears to be triggered by any text that immediately follows http:// where http:// is at the start of a new line.

#3 @SergeyBiryukov
11 years ago

  • Component changed from General to Embeds
  • Version changed from 3.5 to 2.9

Introduced in [12023].

WP_Embed::autoembed() has a regex to catch any unlinked URLs that are on their own line:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/class-wp-embed.php#L237

WP_Embed::autoembed_callback() then adds an empty line after the URL.

#4 @SergeyBiryukov
11 years ago

#24615 was marked as a duplicate.

#5 @Viper007Bond
11 years ago

  • Cc Viper007Bond added

#6 @garhdez
11 years ago

I think that this is not a bug.

Any string (containing allowed chars) can be a correct URL. For example, "http://a" could be the URL of real server in an intranet, so I think this regex is well written, because matches any string before http:// or https:// that is not a whitespace.

#7 @Viper007Bond
11 years ago

It is a bug. :)

The intention of the embed code is to find a URL on its own line. Once found, it tries to embed it and if it is unable to, then it hyperlinks the URL. However the number of line breaks during this whole process should remain the same.

WP_Embed::autoembed_callback() returns new lines before and after the string it returns because those new lines are matched as a part of the search process.

So really the more proper solution is likely to fix the regex to find URLs on their own line but not include the line breaks in the match so that the function that returns the result won't need to include them, potentially including extra line breaks in the process.

#8 @DrewAPicture
11 years ago

  • Keywords needs-patch dev-feedback added

#9 follow-up: @wonderboymusic
10 years ago

  • Milestone changed from Awaiting Review to 4.0

Let's take a look at this while we are in fellowship around oEmbed

#10 in reply to: ↑ 9 ; follow-up: @UmeshSingla
10 years ago

In order to fix this I've added a strpos check for <iframe on returned URL, So if a URL contains an embed iframe, new lines will be added to it, otherwise a normal hyperlink is returned.

It also fixes the url not being converted to a hyperlink.

This part of code seems to be creating the issue, I've removed it as I didn't found any use for it.

$oldval = $this->linkifunknown;
$this->linkifunknown = false;

@UmeshSingla
10 years ago

Fixes new line being inserted for a url, URL being not converted to Hyperlink

#11 in reply to: ↑ 10 ; follow-up: @SergeyBiryukov
10 years ago

I think $this->linkifunknown should be left as is, it's used in WP_Embed::maybe_make_link() (as noted in comment:1:ticket:20315).

#12 in reply to: ↑ 11 @UmeshSingla
10 years ago

Replying to SergeyBiryukov:

I think $this->linkifunknown should be left as is, it's used in WP_Embed::maybe_make_link() (as noted in comment:1:ticket:20315).

For autoembed_callback it is dead code, $this->linkifunknown is set to false before calling shortocode function. As a result if there is a link in content it is never being converted to a link, it is left as it is, which is wrong, as per Once found, it tries to embed it and if it is unable to, then it hyperlinks the URL

Version 0, edited 10 years ago by UmeshSingla (next)

#13 @nofearinc
10 years ago

  • Keywords has-patch added; needs-patch removed

#14 @UmeshSingla
10 years ago

@SergeyBiryukov, your feedback please.

#15 follow-up: @DrewAPicture
10 years ago

23776.patch still applies. What's left here?

#16 in reply to: ↑ 15 @UmeshSingla
10 years ago

Replying to DrewAPicture:

23776.patch still applies. What's left here?

I find the patch absolutely right, but as per Sergey, "I think $this->linkifunknown should be left as is, it's used in WP_Embed::maybe_make_link()​ (as noted in comment:1:ticket:20315)."

This ticket was mentioned in IRC in #wordpress-dev by DrewAPicture. View the logs.


10 years ago

#18 @DrewAPicture
10 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@SergeyBiryukov: Can you please review what's left here?

#19 @markjaquith
10 years ago

  • Milestone changed from 4.0 to Future Release

What is the reasoning behind the iframe check? Couldn't other things be returned than iframes? Not really confident in this, and it's an old issue, so punting for now.

#20 @chipx86
9 years ago

This is an old issue, but one that I spent a good chunk of the day trying to deal with.

I first noticed this issue when trying to display sample output from a command in a little tutorial I was writing. In one section of the output, there would be a URL displayed on each line. Wordpress was rendering this with a blank line in-between. Inside the <pre> block, this just manifested as a blank line in-between. Outside a <pre>, this manifested as a <p> for each line.

Expected output was:

$ my command
First line.

http://example.com/1/
http://example.com/2/
Last line.

Resulting output with 4.0.1:

$ my command
First line.

http://example.com/1/

http://example.com/2/

Last line.

The current attached patch converts the URLs to links and removes a leading blank line that should have remained. It results in the following (pretend that the brackets are links):

$ my command
First line.
[http://example.com/1/]
[http://example.com/2/]
Last line.

My new patch (attaching in the next update) doesn't add any heuristics like checking for iframes, or removal of linkifunknown, but rather simply captures the whitespace matched in the regex and preserves it in the output. The resulting output matches my expected output in the first example above, and was tested both with and without pre-formatted text.

@chipx86
9 years ago

Preserves whitespace around URLs in the auto-embed code.

#21 @SergeyBiryukov
9 years ago

  • Keywords needs-unit-tests added; dev-feedback removed
  • Milestone changed from Future Release to 4.2

#22 @sgrant
9 years ago

Here's two tests, one simple with the empty string, and another using the example from @chipx86 . (Thanks!) The second test fails before autoembed-preserve-whitespace.diff is applied, and passes afterwards.

@sgrant
9 years ago

#23 @SergeyBiryukov
9 years ago

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

In 31066:

Don't force newlines around URLs in WP_Embed::autoembed().

props chipx86, sgrant.
fixes #23776.

Note: See TracTickets for help on using tickets.