WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 11 months ago

Last modified 10 months ago

#36576 closed enhancement (fixed)

Split do_all_pings

Reported by: dshanske Owned by: dshanske
Milestone: 5.6 Priority: normal
Severity: trivial Version: 2.1
Component: Pings/Trackbacks Keywords: has-patch needs-testing has-dev-note
Focuses: Cc:

Description

do_all_pings is a function that does pingbacks, trackbacks, enclosures, and updates. It is attached to a hook called do_pings.

Suggesting that the function be broken into individual pieces and those pieces attached in priority order to the same hook.

This would allow the specific removal/replacement of one of more services.

Attachments (3)

36576.diff (2.4 KB) - added by garrett-eclipse 12 months ago.
Updated patch to introduce a perform_pings action inside of the do_all_pings function.
36576.1.diff (2.4 KB) - added by garrett-eclipse 12 months ago.
Minor fix as I'd left a action param from where I'd copied it from. There should be no params for the action.
36576.2.diff (2.6 KB) - added by Mista-Flo 11 months ago.
Refreshed patch, fix typo

Download all attachments as: .zip

Change History (32)

#1 @dshanske
5 years ago

  • Keywords needs-patch added
  • Severity changed from normal to trivial

#2 @dshanske
5 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to dshanske
  • Status changed from new to assigned

This ticket was mentioned in PR #494 on WordPress/wordpress-develop by dshanske.


13 months ago

  • Keywords has-patch added; needs-patch removed

Splits do_all_pings into separate functions. Keeps the do_all_pings function for backcompat, but hooks individual functions on to the action.

Trac ticket: https://core.trac.wordpress.org/ticket/36576

#4 @dshanske
13 months ago

  • Milestone set to 5.6

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


12 months ago

#6 @Mista-Flo
12 months ago

  • Keywords needs-unit-tests needs-refresh added

#7 @hellofromTonya
12 months ago

  • Keywords needs-dev-note added

New public function added. Needs a mention in the Misc Dev Note.

Last edited 12 months ago by hellofromTonya (previous) (diff)

#8 @dshanske
12 months ago

@Mista-Flo I am a bit confused about what unit testing is required or what refresh. There is effectively no change. Three sets of code were in one function hooked into an action, they are now separate functions hooked into the same action, so they can be individually removed. The code in question was already being unit tested.

@hellofromTonya Where does one write a dev note? If I were to note it here, it would say, "Since WordPress 2.1, do_all_pings has, every time a post is published, queried for all pending pingbacks, trackbacks, and enclosures, regardless of whether you are actually using these features. It is hooked into the do_pings action. This change splits each query into its own function(do_pingbacks, do_enclosures, do_trackbacks), which are added to the do_pings action so you could remove one or more on systems not using them. The do_all_pings function is retained for backward compatibility and now calls the individual functions."

#9 @Mista-Flo
12 months ago

  • Keywords needs-unit-tests needs-refresh removed

Hi @dshanske Yes, after looking at the code, I think it's safe to assume it does not need unit test, sorry for the wrong alert.

Nice catch about the unused global wpdb variable btw.

The code looks good, great job

#10 @hellofromTonya
12 months ago

@dshanske Here's a link in the handbook for "Writing developer notes" https://make.wordpress.org/core/handbook/tutorials/writing-developer-notes/. This ticket may only need a mention in the Misc Dev Note.

#11 @dshanske
12 months ago

@hellofromTonya I think it would be in a Misc one.

#12 @hellofromTonya
12 months ago

Thanks @dshanske. I think you're right. I edited my comment above to avoid any confusion when we're doing the Misc dev note.

#13 @johnbillion
12 months ago

Consideration: If a site has removed the do_all_pings action from the do_pings hook in order to disable all pinging, this change will break that and cause all pinging to be reactivated.

#14 @dshanske
12 months ago

@johnbillion That is a good point...how do we address it while allowing the increased granularity?

#15 @johnbillion
12 months ago

  • Keywords dev-feedback added

Ideas anyone?

#16 @garrett-eclipse
12 months ago

Could we leave a do_all_pings function and call action, and inside the do_all_pings function call the others seperately?
So if you remove do_all_pings all would be disabled. But you could also selectively disable from their own actions.

<?php
function do_all_pings() {
    do_action( 'doing_pings' );
}
add_action( 'do_pings', 'do_all_pings', 10, 0 );
add_action( 'doing_pings', 'do_pingbacks', 10, 0 );
add_action( 'doing_pings', 'do_enclosures', 10, 0 );
add_action( 'doing_pings', 'do_tracksbacks', 10, 0 );
add_action( 'doing_pings', 'generic_ping', 10, 0 );

This leaves the ability to remove all pings, but also introduces the ability to remove specific pings.

Thoughts?

#17 @azaozz
12 months ago

  • Keywords needs-patch added; has-patch removed

Yeah, seems this will work. Refactor do_all_pings() into a function that only triggers the action all "ping types" are attached to.

Of course all add_action() will go in /wp-includes/default-filters.php and the code from do_all_pings() will be separated similarly to
https://github.com/WordPress/wordpress-develop/pull/494/commits/6a527c50c47cb4d0f534a8cccc270a5410e8837b.

#18 follow-up: @dshanske
12 months ago

The interim measure could be just to split the do_all_pings functions into individual functions, have them still loaded onto the hook using do_all_pings, and allow for customization by removing it and adding the individual functions.

Does that make sense?

@garrett-eclipse
12 months ago

Updated patch to introduce a perform_pings action inside of the do_all_pings function.

#19 @garrett-eclipse
12 months ago

  • Keywords has-patch needs-testing added; dev-feedback needs-patch removed

I've put together an initial patch 36576.diff in this direction, please test and review.

In the patch I introduce a new action hook perform_pings used to hook in the other new functions. I did rename the new methods from do_ as there's already a do_trackbacks so wanted to avoid a duplicate use of the function name. And I removed the first comments inside the new functions as their function docblock already handles the description and they're mostly duplicated there.

@garrett-eclipse
12 months ago

Minor fix as I'd left a action param from where I'd copied it from. There should be no params for the action.

#20 in reply to: ↑ 18 @garrett-eclipse
12 months ago

Replying to dshanske:

The interim measure could be just to split the do_all_pings functions into individual functions, have them still loaded onto the hook using do_all_pings, and allow for customization by removing it and adding the individual functions.

Does that make sense?

Sorry @dshanske didn't see your comment while I uploaded. Take a look at the patch and let me know if that's the direction that would make sense. Thanks

#21 @dshanske
12 months ago

@garrett-eclipse It looks good and I'm fine with doing it this way. The reason I asked the other question was because we'd be adding another hook to accomplish it this way.

But if everyone is fine with that, it solves the problem neatly.

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


11 months ago

#23 @hellofromTonya
11 months ago

  • Keywords needs-refresh added

Notes from today's scrub warrant changes:

  • Prefix function with wp_. Helen suggests

I think it might be best to go with wp_do_trackbacks() etc. but I am not SUPER strongly opinionated on it

  • Sergey noted a typo

There's also a typo in the second file, tracksbacks.

Beta 1 is tomorrow. But if we can get the above changes and then testing, there's a chance for it to land in 5.6.

@Mista-Flo
11 months ago

Refreshed patch, fix typo

#24 @Mista-Flo
11 months ago

  • Keywords needs-refresh removed

Hi there, I have uploaded a new patch, just a quick update to fix the typo and add a wp prefix to the functions. I am not confident to rename generic_ping for backward compatible reasons.

#25 @SergeyBiryukov
11 months ago

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

In 49211:

Pings/Trackbacks: Split do_all_pings() into several functions:

  • do_all_pingbacks()
  • do_all_enclosures()
  • do_all_trackbacks()

This allows for the specific removal/replacement of one of more services.

Props dshanske, garrett-eclipse, Mista-Flo, azaozz, hellofromTonya.
Fixes #36576.

#26 @SergeyBiryukov
11 months ago

Went back and forth on function naming for a bit, decided to use the existing pattern for consistency :)

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


11 months ago

Note: See TracTickets for help on using tickets.