#89 support closures/anonymous functions #174
Conversation
|
OK, I think we should be all set here. Let me know if there's anything else I can do. |
| unset( $filter ); | ||
|
|
||
| return $filters; | ||
| } |
There was a problem hiding this comment.
looks like this portion of the file was accidentally formatted perhaps by your editor- we plan to introduce some codestyle / phpcs automation to avoid style inconsistencies in the future - would you mind restoring the lines that have no code/logic change please?
| return $value; | ||
| } elseif ( ''.$value === '<Closure>' ) { | ||
| return '__CLOSURE__'; |
There was a problem hiding this comment.
hey @acobster may I know why the concatenation with ''.$value here? shouldn't you check is_string( $value ) && $value === '<Closure>' instead? is this expected to have a toString method? or isn't this expected to be $value instanceof \Closure? Just double checking, I've read your previous explanation in your PR description.
On the other hand, if it's a string instead, wouldn't it return true for the previous condition is_scalar()?
sorry if I missed anything here
There was a problem hiding this comment.
The code in trunk has a new case to check for instances of AnyInstance in this method.
Maybe this PR can check for instances of Mockery\Matcher\Type, which is what we will receive when WP_Mock\Functions::type('callable') is passed as the expected callback?
There was a problem hiding this comment.
@wvega-godaddy I've updated the code with your feedback in a separate branch feature/closures-bis mind checking if that's what you had in mind?
| \WP_Mock::expectActionAdded( 'save_post', \WP_Mock\Functions::type( 'closure' ) ); | ||
| \WP_Mock::expectFilterAdded( 'the_content', \WP_Mock\Functions::type( 'closure' ) ); |
There was a problem hiding this comment.
Should these use type( 'callable' ) instead?
| \WP_Mock::expectActionAdded( 'save_post', \WP_Mock\Functions::type( 'closure' ) ); | |
| \WP_Mock::expectFilterAdded( 'the_content', \WP_Mock\Functions::type( 'closure' ) ); | |
| \WP_Mock::expectActionAdded( 'save_post', \WP_Mock\Functions::type( 'callable' ) ); | |
| \WP_Mock::expectFilterAdded( 'the_content', \WP_Mock\Functions::type( 'callable' ) ); |
There was a problem hiding this comment.
Or are we trying to match instances of the Closure class? In that case I think using Clousure::class is more accurate:
| \WP_Mock::expectActionAdded( 'save_post', \WP_Mock\Functions::type( 'closure' ) ); | |
| \WP_Mock::expectFilterAdded( 'the_content', \WP_Mock\Functions::type( 'closure' ) ); | |
| \WP_Mock::expectActionAdded( 'save_post', \WP_Mock\Functions::type( Closure::class ) ); | |
| \WP_Mock::expectFilterAdded( 'the_content', \WP_Mock\Functions::type( Closure::class ) ); |
\WP_Mock\Functions::type( 'closure' ) works because is_class( 'closure' ) returns true as class names are not case sensitive, but it took me a lot of digging to understand that.
Initially I thought there was some special handling for the closure string, but we are passing that string to Mockery::type(); and it is used as a class name.
If we are targeting the class name, I believe using Closure::class better reflects what's happening under the hood and makes the example more readable.
There was a problem hiding this comment.
@wvega-godaddy I have applied some of your suggestions in feature/closures-bis
There was a problem hiding this comment.
@wvega-godaddy see #183
|
@unfulvio-godaddy @wvega-godaddy I don't work on WP stuff with any regularity anymore, and since this isn't a super quick fix I don't anticipate having time to look into these in the foreseeable future. The suggested changes sound reasonable though. Thanks for your work! |
|
Thank you for letting us know @acobster, no worries, we'll try to apply the change this PR originally was meant for. @wvega-godaddy I have pushed a separate branch I think the recent addition of the If we want to support just the mapping of |
|
see #183 |
My apologies, even the PhpUnit test suite wasn't passing for me at the time. I think the API of AnyInstance (or anything really, since it's pre 1.0) is open to redesign. I definitely noticed the Closure issue as I wrote/waited for the AnyInstance PR. I think maybe ideally, both would be abstracted behind |
## Summary Reimplements #174 after implementing PhpStan and code styles in WP_Mock. Tests have been reworked in PhpUnit. Quote from OP: > WP_Mock\Functions::type('callable') was causing a mismatch between the safe_offset values returned for the Mockery matcher and the actual closure being passed to add_action. The matcher was getting spl_object_hash'd, while the Closure was getting `"__CLOSURE__"`. Now, safe_offset() treats Mockery matchers with type closure as `"__CLOSURE__"` too. ### Closes: #89 ## Test - [x] Unit tests pass ## Open questions I think we could rename `safe_offset()` into something more meaningful to make it easier to understand what it does. Seems this is used solely within the `Hook` abstract and Filter/Action implementations to get a string representations of the hook arguments. We could rename it using camel case and soft deprecate the former method. As predicted, PhpStan will start spitting errors as tests, return types and such start to change. I covered some of the issues in this PR, but had to update the baseline to fix several unrelated positives which we can address in subsequent PRs. Co-authored-by: Ashley Gibson <99189195+agibson-godaddy@users.noreply.github.com>
This fixes #89 and supsedes PRs #130 and #119.
The root of the issue turned out to be that using WP_Mock\Functions::type('callable') was causing a mismatch between the safe_offset values returned for the Mockery matcher and the actual closure being passed to add_action. The matcher was getting spl_object_hash'd, while the Closure was getting
"__CLOSURE__". Now, safe_offset() treats Mockery matchers with type closure as"__CLOSURE__"too.