WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#14789 reviewing defect (bug)

Inconsistency in 'all' hook invocation

Reported by: hakre Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0.1
Component: Plugins Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

The 'all' hook (catchall hook for any hook name) is getting inconsistently called depending on how the concrete hook is being invoked: by apply_filters, apply_filters_ref_array, do_action or do_action_ref_array.

In the simple cases (apply_filters, do_action), the hook will get all parameters passed, in the ..._ref_array cases, the hook will get only two parameters passed in which the first parameter is the name of the hook (a.k.a. tag) and the second parameter is an array of all other parameters.

Every 'all' hook-function-callbacks should be called in the same way regardless whether which of the four invocation functions have been used on that pointcut.

Additionally all four routines share a lot of the same code which could benefit of a refactoring.

Attachments (7)

14789.patch (701 bytes) - added by hakre 4 years ago.
14789.2.patch (6.4 KB) - added by hakre 4 years ago.
Basically filters and actions are hooks and the same.
14789.3.patch (6.7 KB) - added by hakre 4 years ago.
Remove of some redundant code.
14789.4.patch (15.4 KB) - added by hakre 4 years ago.
Touched other functions as well.
14789.5.patch (15.4 KB) - added by hakre 4 years ago.
Corrected some whitespace issues on empty lines.
14789.6.patch (705 bytes) - added by hakre 3 years ago.
Vertical rythm fixed.
14789.7.patch (1.4 KB) - added by hakre 3 years ago.
no need to shift plus type hints

Download all attachments as: .zip

Change History (24)

hakre4 years ago

hakre4 years ago

Basically filters and actions are hooks and the same.

hakre4 years ago

Remove of some redundant code.

comment:2 hakre4 years ago

  • Keywords dev-feedback added

comment:3 hakre4 years ago

  • Summary changed from Inconsitency in 'all' hook invocation to Inconsistency in 'all' hook invocation

hakre4 years ago

Touched other functions as well.

hakre4 years ago

Corrected some whitespace issues on empty lines.

comment:4 hakre4 years ago

Related: #10535

comment:5 hakre4 years ago

Second Parameter Definition in Function Header of do_action() might be bogus: #14881

comment:6 hakre3 years ago

First patch is the important one: http://core.trac.wordpress.org/attachment/ticket/14789/14789.patch

The other patches aren't the actual bugfix, they should go into a ticket on it's own.

comment:7 westi3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

First patch might be ok.

Needs testing and validating with the unit test suite.

comment:8 hakre3 years ago

  • Keywords dev-feedback removed
  • Resolution maybelater deleted
  • Status changed from closed to reopened

14789.patch had been extensively tested, unit test suite run same with and w/o patch. If you have some specific test in mind, let me know.

Reference: #16661

comment:9 follow-up: scribu3 years ago

  • Owner set to scribu
  • Status changed from reopened to reviewing

The inconsistent spacing in 14789.patch makes it harder to read. No, really.

comment:10 scribu3 years ago

  • Owner scribu deleted

comment:11 in reply to: ↑ 9 hakre3 years ago

Replying to scribu:

The inconsistent spacing in 14789.patch makes it harder to read. No, really.

That's right, the vertical rythm is broken. Will update it.

hakre3 years ago

Vertical rythm fixed.

hakre3 years ago

no need to shift plus type hints

comment:12 ocean903 years ago

  • Milestone set to Awaiting Review

comment:13 follow-up: c3mdigital8 months ago

  • Keywords close 2nd-opinion added

comment:14 in reply to: ↑ 13 ; follow-up: hakre7 months ago

Replying to c3mdigital:

Can you please justify your close suggestion?

comment:15 in reply to: ↑ 14 ; follow-up: c3mdigital7 months ago

Replying to hakre:

Replying to c3mdigital:

Can you please justify your close suggestion?

To me this seems more like refactoring than a bug and hasn't had any traction in 3 years. I added 2nd opinion so someone else could give a suggestion on whether or not this ticket should stay open. I really don't see a bug here.

comment:16 in reply to: ↑ 15 hakre7 months ago

  • Keywords close removed

Replying to c3mdigital:

Replying to hakre:

Replying to c3mdigital:

Can you please justify your close suggestion?

To me this seems more like refactoring

No, this is not refactoring. Refactoring would mean that behavior is not changed even the code changed. Here the behavior is changed with this patch. Which by definition is not refactoring.

than a bug

The term bug is not really precise, as it's not refactoring can you please explain what you mean by that?

Last edited 7 months ago by hakre (previous) (diff)

comment:17 nacin3 months ago

  • Component changed from General to Plugins
Note: See TracTickets for help on using tickets.