Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#48246 closed defect (bug) (wontfix)

Explicitly state priority in remove_filter on `remove_filter( 'upgrader_post_install', array( $this, 'check_parent_theme_filter' ) )`

Reported by: itowhid06's profile itowhid06 Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.3
Component: Themes Keywords: has-patch close 2nd-opinion
Focuses: coding-standards Cc:

Description

Just for consistency we can add the priority in the remove_filter.

Attachments (1)

48246.diff (747 bytes) - added by itowhid06 6 years ago.

Download all attachments as: .zip

Change History (7)

@itowhid06
6 years ago

#1 @itowhid06
6 years ago

  • Version set to trunk

#2 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.4

#3 @williampatton
5 years ago

  • Keywords commit added

Patch is good, logic for why it could be added here makes sense to me - consistency with other removes.

Nothing blocking this one unless other have alternative opinions. It's ready for commit.

#4 @garrett-eclipse
5 years ago

  • Keywords close 2nd-opinion added

Sorry, can't say I see the point of this?

A priority is only added in the remove_filter if the original add_filter had a priority other than 10. When it uses the default there's no benefit. Throughout core I've only seen non-default priorities with the exception of some unit tests.

Apologies if I'm completely overlooking something. If we do set this as a precedent would all remove_filter implementations in core then need to be updated to add the priority?

#5 @williampatton
5 years ago

  • Keywords commit removed

@garrett-eclipse you make a good point. The patch is simple enough but I did not check if others throughout core were removed with a priority as well. If your saying it's not done with default priorities - only with others that have non-default priorities then can close this one out.

I don't see making one adjustment though as setting a president for all actions in core to the point where they would all need updated. Just one here wouldn't affect anything else.

It's one and the same - makes no difference if this is added here or not. Let's leave this one open and see if others see something else that we don't but I'll remove the commit tag and keep the 'close' on your recommending :)

#6 @SergeyBiryukov
5 years ago

  • Milestone 5.4 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Thanks for the patch!

I agree with comment:4. Looking at Theme_Upgrader::install() the priority is only passed to the add_filter() call so that the number of arguments (3) could be passed next.

Since 10 is the default priority, adding it to the remove_filter() call doesn't seem to provide any benefit or affect consistency here.

Note: See TracTickets for help on using tickets.