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

Static closures resolving #3067

Closed
wants to merge 2 commits into from
Closed

Static closures resolving #3067

wants to merge 2 commits into from

Conversation

ElisDN
Copy link

@ElisDN ElisDN commented Mar 27, 2021

Fixes #1636 PHP Warning, wich still actual in all PHP versions.

@coveralls
Copy link

coveralls commented Mar 27, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling ddc85e9 on ElisDN:static-closure into 090498e on slimphp:4.x.

@ElisDN ElisDN changed the title Static closures resolving for PHP 8.0 Static closures resolving for PHP 8 Mar 27, 2021
@ElisDN ElisDN force-pushed the static-closure branch 2 times, most recently from f46298d to cc67e40 Compare March 27, 2021 15:01
@ElisDN ElisDN changed the title Static closures resolving for PHP 8 Static closures resolving Mar 27, 2021
@ElisDN ElisDN changed the title Static closures resolving Static closures resolving tests Mar 27, 2021
@ElisDN ElisDN force-pushed the static-closure branch 4 times, most recently from 0221dc4 to 6dc3f36 Compare March 27, 2021 16:18
@ElisDN ElisDN changed the title Static closures resolving tests Static closures resolving Mar 28, 2021
/** @var Closure $callable */
$callable = $callable->bindTo($this->container);
/** @var Closure $bound */
$bound = @$callable->bindTo($this->container);
Copy link
Member

Choose a reason for hiding this comment

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

We don't suppress errors in this codebase. This isn't a fix.

Copy link
Author

Choose a reason for hiding this comment

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

I know. But there is no legal way to check that Closure is static.

Copy link
Member

Choose a reason for hiding this comment

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

Until there is a way to do this without suppressing errors, it will not be merged unfortunately. There is no upside to allowing static closures. This is a micro optimization.

@l0gicgate
Copy link
Member

See #3067 (comment)

@l0gicgate l0gicgate closed this Apr 19, 2021
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.

PHP Warning: Cannot bind an instance to a static closure in RouteGroup.php on line 42
3 participants