WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 5 weeks ago

#48246 new defect (bug)

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

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

Description

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

Attachments (1)

48246.diff (747 bytes) - added by itowhid06 3 months ago.

Download all attachments as: .zip

Change History (6)

@itowhid06
3 months ago

#1 @itowhid06
3 months ago

  • Version set to trunk

#2 @SergeyBiryukov
2 months ago

  • Milestone changed from Awaiting Review to 5.4

#3 @williampatton
5 weeks 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 weeks 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 weeks 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 :)

Note: See TracTickets for help on using tickets.