Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#38197 assigned enhancement

Update Pingback function To Add Return

Reported by: dshanske Owned by: nathanatmoz
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Pings/Trackbacks Keywords: good-first-bug has-patch needs-refresh
Focuses: performance Cc:

Description (last modified by dshanske)

Related to #36824.

Updated to reflect focus on the return issue.

In #36824, the proposal was to improve performance of the do_all_pings function. Part of this involves the fact that if a pending trackback URL is sent a pingback successfully, it should not also send a trackback.

The function should also optionally return which URLs were successfully pinged or a WP_Error object in the event the sending fails, etc.

This would change the signature of the function but would allow for debugging, response to errors, as well as assist in optimizing the do_all_pings function.

The original proposal was to deprecate pingback because it includes $content, a parameter better retrieved from the post itself for purposes of integrity. However, that can be addressed in other ways.

Attachments (2)

38197.diff (1.4 KB) - added by NathanAtmoz 4 years ago.
38197.1.diff (1.6 KB) - added by soulseekah 4 years ago.
refresh + docblock

Download all attachments as: .zip

Change History (29)

#1 @dshanske
5 years ago

  • Description modified (diff)
  • Summary changed from Deprecate and Replace Pingback() Function to Update Pingback function To Add Return

#2 @welcher
5 years ago

  • Keywords needs-patch added

#3 @shulard
5 years ago

Hello !

I'm actually trying to make a patch for that ticket, but can't find a clean way to handle this. The pingback function can ping multiple URLs in one call, how the return value can be consistent with that :

  • Always return an array of found URL as key and true / WP_Error if URL was pinged successfully or not ?
  • Return an array of pinged URL or a WP_Error if at leat one failed ?
Last edited 5 years ago by shulard (previous) (diff)

#4 @dshanske
5 years ago

You are right, handling this is difficult.

My suggestion is to start by splitting the code that sends a single ping based on a URL into a new function with a return. This will get a single true/false to build an array. We can also add split the function to find the content in a content block.

That would allow the existing function to call them but future changes in do_all_pings to call each piece independently.

In the meantime, pingback can return true if all successful, which should shutdown Trackbacks for the post_id.

4 years ago

#5 @NathanAtmoz
4 years ago

The patch attached returns an array with the $pagelinkedto as the key and the result of WP_HTTP_IXR_Client::query() as the key. Not sure if this is a sufficient solution for your needs.

#6 @dshanske
4 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.9

Needs a second opinion, but I think this is acceptable.

#7 @jbpaul17
4 years ago

  • Keywords 2nd-opinion added

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

4 years ago

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

4 years ago

#10 @jbpaul17
4 years ago

  • Milestone changed from 4.9 to Future Release

Punting this to Future Release per the 4.9 bug scrub earlier today.

#11 @NathanAtmoz
4 years ago

  • Keywords dev-feedback added

#12 @DrewAPicture
4 years ago

  • Owner set to nathanatmoz
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

@dshanske Can you please have a look a the patch and provide any feedback @NathanAtmoz might need to keep this ticket moving?

#13 @dshanske
4 years ago

@DrewAPicture I do not see anything wrong with the patch. I asked for a second opinion at the time. It returns an array of statuses, so someone can evaluate whether there was an issue with any of them.

#14 @dshanske
4 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Future Release to 4.9.5

#15 @dshanske
4 years ago

#41842 was marked as a duplicate.

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

4 years ago

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

4 years ago

#18 @ocean90
4 years ago

  • Keywords needs-refresh added

The change needs to be documented in the function's DocBlock.

#19 @audrasjb
4 years ago

  • Milestone changed from 4.9.5 to 4.9.6

Bumping to 4.9.6 due to 4.9.5 beta release.

4 years ago

refresh + docblock

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

4 years ago

#21 @soulseekah
4 years ago

  • Keywords needs-refresh removed

#22 @desrosj
4 years ago

I am inclined to punt this to 5.0 because the function signature is changing.

#23 @dshanske
4 years ago

@desrosj I don't think the signature has changed. The arguments passed are the same. There was no return, now there is.

I think if functionality changes in a non-backward compatible way, that is a signature change. If you add a parameter or return one, the original signature is still maintained.

#24 @soulseekah
4 years ago

It used to return null which is falsey, now it returns an array which truthy. I'd err on the safe side as well.

#25 @desrosj
4 years ago

  • Milestone changed from 4.9.6 to 5.0

I am going to move this to 5.0 because of the potential for issues with the change in returned values. If a committer is confident in this, it can be moved back in.

#26 @pento
3 years ago

  • Milestone changed from 5.0 to 5.1

#27 @pento
3 years ago

  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from 5.1 to Future Release

It seems like it would be more useful for the array elements to be true or a WP_Error, with the error extracted from $client->error.

Also, $ping_status doesn't contain an error if discover_pingback_server_uri() is unable to discover the URI.

Note: See TracTickets for help on using tickets.