has_filter doen't work with type hints with namespaces
SergeyBeloglazov opened this issue · 10 comments
Good Day!
Thank you for this module!
I found, that has_filter() doesn't work with full qualified type hints, as it described here:
https://giuseppe-mazzapica.gitbook.io/brain-monkey/wordpress-specific-tools/wordpress-hooks-added
This test
self::assertNotFalse( has_filter('the_title', 'function ( array $foo, Foo\Bar\Baz $baz, Foo\Bar\Bar ...$bar )' ) );
leads to an error
Brain\Monkey\Name\Exception\InvalidCallable: Given string "function ( array $foo, Foo\Bar\Baz $baz, Foo\Bar\Bar ...$bar )" is not a valid PHP callable.
As I inspected the BrainMonkey code, it fails on preg_match() in
brain/monkey/src/Name/ClosureParamStringForm.php on line 62
if ($type && ! preg_match(self::VALID_PARAM_PATTERN, $type)) {
where VALID_PARAM_PATTERN = '/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/'.
A type with full qualified type name with namespaces will not satisfy it because it contains slashes .
See also https://stackoverflow.com/questions/41027816/what-are-the-valid-characters-in-php-namespace-names
My composer.lock:
"name": "brain/monkey",
"version": "2.6.0",
Hi @SergeyBeloglazov - good catch, and nice analysis! 🙂
I created a PR, #95, which is pending review.
Once merged, you should be able to use closures with fully-qualified type declarations, both in string form and as actual closure expressions. (See the new functional test.)
Thank You!
Thanks both of you @tfrommen @SergeyBeloglazov
The PR is merged and now.
@tfrommen One of the tests you added is failing on PHP 5.6 (https://github.com/Brain-WP/BrainMonkey/runs/2123646454?check_suite_focus=true), could you please investigate?
At some point I'll deprecate 5.6 but for current major it will stay supported.
@gmazzap hm, so, it seems the issue is that ReflectionParamater::hasType() does not exist in PHP 5.6, which is of course the reason why there is a >= 7 check in the code 😅.
So, I think all we can do here is to remove that particular test for PHP 5.
But that also means that some code using (namespaced) type declarations in string-form callbacks doesn't work as expected in 5.6.
Is that something you are OK with?
Thanks @tfrommen for the quick response.
I'm ok with that. There are other tests that are skipped for 5.6.
The fact that there's a difference in functionality should be documented.
If you have will and time, could you also do that? If not, I'll docs as soon as I can.
I will remove the test, but I don't have time for adding documentation soon. This is mainly because I would have to understand where this (i.e., string-form callbacks) is being used. I assume it's local to expectations around hooks usage, but that would have to be confirmed.
Thanks @tfrommen, I've merged your last PR. I'll add docs as soon as I can and then make a release.
@gmazzap @tfrommen I was just looking at this fix and why the PHP 5.6 tests were removed. It actually quite possible to make the code in the ClosureParamStringForm::fromReflectionParameter method compatible with PHP 5.6 and after all, class name based, including FQN type declarations are supported in PHP 5.6.
Would you be interested in a fix for this ?