Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds isStatic and isNotStatic #310

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

peterfox
Copy link

@peterfox peterfox commented Jan 12, 2025

Changes

  • Adds the assertions isStatic and isNotStatic.
  • Adds tests.
  • Updates the readme docs.

Why

This can be useful in some libraries where you want to make sure that the end user hasn't provided a static function as an argument.

Assert::isStatic($closure, $message = '');
Assert::isNotStatic($closure, $message = '');

@zerkms
Copy link
Contributor

zerkms commented Jan 12, 2025

you want to make sure that the end user hasn't provided a static function as an argument.

what would be a real life use case for this exactly? What property of a static function can break 3rd party code versus non-static function that simply does not use $this?

@peterfox
Copy link
Author

So if you're writing an API that accepts a Closure but contextually binds it later on it will stop the code from working.

$obj = new stdClass();
$function = static function () {
    return 'foo';
};
$function->bindTo($obj);

This would then throw an exception like 'Cannot bind an instance to a static closure'.

A good example of where I've seen this happen is with Pest where you provide closures where the Closure is then bound to a different object.

There's probably less of a need for isStatic than there is isNotStatic.

src/Assert.php Outdated
Comment on lines 1952 to 1953
Assert::isCallable($closure);
$reflection = new ReflectionFunction($closure);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not type safe, because:

  • The argument $closure is not typed, except with a docblock
  • The docblock says Closure, but is enforced with isCallable
  • The assertion isCallable uses is_callable(), which has type callable|mixed
  • But ReflectionFunction accepts Closure|string

End result, the input type is enforced as callable and then passed to a constructor that accepts Closure|string. I believe the correct solution here is to type cast:

```php
if (($closure instanceof Closure) === false) {
    $closure = Closure::fromCallable($closure);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by just type hinting as that made the most sense to me. It also then matches the throws assertion.

@peterfox peterfox requested a review from shadowhand January 13, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants