Make WordPress Core

Opened 17 years ago

Closed 16 years ago

#3495 closed defect (bug) (fixed)

Pingback excerpt-fetching code looks for a link to the SOURCE site instead of the TARGET site.

Reported by: markjaquith's profile markjaquith Owned by: markjaquith's profile markjaquith
Milestone: 2.1 Priority: high
Severity: normal Version: 2.0.5
Component: XML-RPC Keywords: has-patch
Focuses: Cc:



$sem_regexp_pb = "/(\\/|\\\|\*|\?|\+|\.|\^|\\$|\(|\)|\[|\]|\||\{|\})/";
$sem_regexp_fix = "\\\\$1";
$link = preg_replace( $sem_regexp_pb, $sem_regexp_fix, $pagelinkedfrom );

So now $link is a regex-safe version of the SOURCE URL.


foreach ( $p as $para ) {
	if ( $finished )
	if ( strstr( $para, $pagelinkedto ) ) {
		$context = preg_replace( "/.*<a[^>]+".$link."[^>]*>([^>]+)<\/a>.*/", "$1", $para );
		$excerpt = strip_tags( $para );
		$excerpt = trim( $excerpt );
		$use     = preg_quote( $context );
		$excerpt = preg_replace("|.*?\s(.{0,100}$use.{0,100})\s|s", "$1", $excerpt);
		$finished = true;

The SOURCE URL's paragraphs are iterated. Once one is found that contains the TARGET URL, it (mistakenly) looks for a link to the SOURCE URL and uses that as context for the excerpt. It doesn't find it, of course. But it doesn't really matter, because even if the context regex used the TARGET URL as it should, the excerpt regex matches the whole paragraph.

I thought I was going crazy when I saw this code... was pretty sure that I was missing something.

This dates back all the way to [2619]

Patch coming.

Attachments (2)

pingback_excerpt.001.diff (2.4 KB) - added by markjaquith 17 years ago.
Patch for trunk
pingback.diff (2.4 KB) - added by ruckus 17 years ago.
Patch against 2.0.6-RC1

Download all attachments as: .zip

Change History (17)

17 years ago

Patch for trunk

#1 @markjaquith
17 years ago

  • Milestone changed from 2.0.6 to 2.1
  • Owner changed from anonymous to markjaquith
  • Status changed from new to assigned

Uploaded: pingback_excerpt.001.diff

  • use preg_quote() instead of wacky manual escaping
  • use the target URL for context
  • mark the link text to prevent more than one occurrence of that string from interfering
  • make the context-finding regex lazy
  • trim link text if it is more than 100 characters long

There. Now that should be functioning like it was intended to function. You get a nice excerpt with an absolute upper limit of 300 chars instead of the 1000+ char monstrosities that frequently showed up with the broken code.

I'll let this sit a day or two for auditing.

#2 @ryan
17 years ago

Haven't tested yet but code looks good.

#3 @tenpura
17 years ago

I have tested. It seems to work like it was originally intended.

#4 @markjaquith
17 years ago

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

(In [4667]) Pingback excerpt fetching improvements and fixes. fixes #3495

#5 @markjaquith
17 years ago

  • Milestone changed from 2.1 to 2.0.7

Too late for 2.0.6, but definitely a 2.0.7 candidate.

17 years ago

Patch against 2.0.6-RC1

#6 @ruckus
17 years ago

  • Keywords bg|has-patch added
  • Milestone changed from 2.0.7 to 2.0.8
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening so we could maybe get this into 2.0.8

#7 @markjaquith
17 years ago

  • Milestone changed from 2.0.8 to 2.0.9

Want to see what 2.1 users think of this before getting it into 2.0.x

#8 @Nazgul
17 years ago

  • Keywords has-patch added; bg|has-patch removed
  • Milestone changed from 2.0.9 to 2.0.10

2.0.9 is out, so bumping the milestone to 2.0.10.

#9 @foolswisdom
17 years ago

  • Milestone changed from 2.0.10 to 2.0.11

#10 follow-up: @Nazgul
17 years ago

Instead of bumping these tickets after each security release. Wouldn't it make more sense if we milestone them as "2.0.eventually" and set the final correct milestone when they're commited? It would save us some work. :)

#11 in reply to: ↑ 10 @foolswisdom
17 years ago

Replying to Nazgul:

Wouldn't it make more sense if we milestone them as "2.0.eventually"

True enough. Next time, or maybe Mark J will do that ;-)

#12 @ryan
17 years ago

Note that when a milestone is marked as completed, trac offers to move all open bugs to another milestone. It's not difficult to bulk move items from one milestone to another.

#13 @Nazgul
17 years ago

Any change of this going in 2.0.11 or are we going to bump again?

#14 @rob1n
17 years ago

  • Milestone changed from 2.0.11 to 2.0.eventually

I suppose we can just bump it -- if Mark wants to put it in the next 2.0.x release, then he can move the milestone and commit it.

#15 @ryan
16 years ago

  • Milestone changed from 2.0.eventually to 2.1
  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.