has_filter/has_action return true instead of the hook priority

[coreyworrell]

Normally has_filter returns the priority of the filter if it finds one (https://developer.wordpress.org/reference/functions/has_filter/). But this library only returns bool. Perhaps it is not super important, but here's an example of where testing the correct priority would be good.

class Example
{
  protected int $priority;

  public function __construct(int $priority = 10)
  {
    $this->priority = $priority;
  }

  public function register(): void
  {
    add_filter('something', [$this, 'something'], $this->priority);
  }

  public function something(): string
  {
    return 'whatever';
  }
}

class ExampleTest extends TestCase
{
  public function testPriority(): void
  {
    $priority = 50;

    $instance = (new Example($priority))->register();
    
    $this->assertSame(
      $priority,
      has_filter('something', [$instance, 'something']),
    );
  }
}

[gmazzap]

Thanks @coreyworrell this is indeed a bug. I'll try to find the time to fix as soon as I can.

[gmazzap]

Thanks to @widoz this is fixed in 2.5.0.

Thanks for reporting @coreyworrell

[coreyworrell]

Thanks for the quick fix @widoz @gmazzap!

It should be noted that this is a breaking change as well. As existing tests (and the docs) that use $this->assertTrue(has_filter('...', $callback)) will now fail.

I think converting to $this->assertNotFalse(has_filter('...', $callback)) will work if you don't care about the priority and just want to check that the callback is added.

Also, the docs (https://brain-wp.github.io/BrainMonkey/docs/wordpress-hooks-added.html) have a line in the 4th code sample on that page that uses

self::assertTrue( has_action('init', 'Some\Name\Space\MyClass->init()', 20) );

which appears to just be an oversight where the priority was passed to the has_action function, which does not do anything.

[coreyworrell]

Actually having an optional third parameter for has_action/filter() for $priority could be a better API, that way those functions can always return a bool instead of bool|int. But then it wouldn't match with WordPress` API (which is poor design to begin with). Not sure which is more important for a testing library though.

[widoz]

I'm sorry to not have updated the documentation, I really missed that. I'll do asap, maybe including both example for asserting with assertEquals and assertTrue if for @gmazzap is ok to stick with the proposed solution.

I think converting to $this->assertNotFalse(has_filter('...', $callback)) will work if you don't care about the priority and just want to check that the callback is added.
Actually having an optional third parameter for has_action/filter() for $priority could be a better API, that way those functions can always return a bool instead of bool|int.

Dunno honestly which whould be the better approach in this case, I think consistency with WordPress should be important but I understand the fact that do something like self::assertTrue( is_numeric(has_action('init', 'Some\Name\Space\MyClass->init()'))); or the other form you suggested $this->assertNotFalse(has_filter('...', $callback)) isn't immediated as one would expect.

[gmazzap]

It should be noted that this is a breaking change as well.

Yes, I considered this. But being a test tool I'm not really concerned about this. Yes, some tests which previously passed now could fail, but that's a good thing because it means that some bug that was unnoticed now could be found.

I think converting to $this->assertNotFalse(has_filter('...', $callback)) will work if you don't care about the priority and just want to check that the callback is added.

Sure that's an option, and that still works. And I will probably use it in the documentation, thanks!

Also, the docs have a line in the 4th code sample on that page that uses...

Yes, that was an oversight, thanks for letting me know. Will fix.

Actually having an optional third parameter for has_action/filter() for $priority could be a better API...

Well, yes, maybe. But that's not the point. The declared and main goal of Brain Monkey is to make WordPress hooks API functions to work as they do in WordPress. This is what was reported in this issue was definitively a bug, and what documentation says is wrong. The code was fixed, I'll fix the documentation.

[gmazzap]

And done :)