WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 22 hours ago

Last modified 22 hours 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: needs-dev-note has-patch needs-testing
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 8 days 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 8 days 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 22 hours ago.
Refreshed patch, fix typo

Download all attachments as: .zip

Change History (29)

#1 @dshanske
5 years ago

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

#2 @dshanske
4 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.


2 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
2 months ago

  • Milestone set to 5.6

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


13 days ago

#6 @Mista-Flo
13 days ago

  • Keywords needs-unit-tests needs-refresh added

#7 @hellofromTonya
13 days ago

  • Keywords needs-dev-note added

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

Last edited 13 days ago by hellofromTonya (previous) (diff)

#8 @dshanske
13 days 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
13 days 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
13 days 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
13 days ago

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

#12 @hellofromTonya
13 days 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
9 days 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
9 days ago

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

#15 @johnbillion
9 days ago

  • Keywords dev-feedback added

Ideas anyone?

#16 @garrett-eclipse
9 days 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
9 days 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
8 days 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
8 days ago

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

#19 @garrett-eclipse
8 days 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
8 days 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
8 days 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
8 days 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.


25 hours ago

#23 @hellofromTonya
23 hours 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
22 hours ago

Refreshed patch, fix typo

#24 @Mista-Flo
22 hours 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
22 hours 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
22 hours ago

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

Note: See TracTickets for help on using tickets.