[WIP] Use callable typehints #14330

mpajunen · 2015-04-12T20:55:59Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Initially discussed in #14293, now includes those changes as well.

The callable typehint is used where applicable. Two types of changes are included:

Replace \Closure typehints
Replace is_callable() checks

The PHP bug in the handling of callables of the 'Foo::bar' type (https://bugs.php.net/bug.php?id=68475) causes some issues. To support every possible callable, call_user_func($func, ...) needs to be used instead of using $func(...) directly. Besides the more verbose syntax, call_user_func is also slower. There are a few options on how to deal with this:

Use call_user_func.
Don't support callables of the static string type. Possibly add an extra check and throw an exception when such a callback is passed.
Don't change typehints where this is an issue, but continue to use \Closure.

Even option 2 doesn't sound entirely unworkable to me, static callback strings are very unwieldy -- especially with namespaces. That the error is thrown at call time is less than ideal of course.

In these cases using call_user_func doesn't look like a too bad option either. But in other places noticeable performance improvements could be possible if existing code could be converted to use direct calls. So the decision may have implications beyond just this PR.

In addition to handling the static strings, I still want to make sure that none of the \Closure typehints that were replaced were intentionally strict. The one typehint in OptionsResolver mentioned by @stof in #14293 was already skipped.

Implement support for callables of 'Foo::bar' type.
Make sure all \Closure to callable changes are valid.
Resolve [Config] Renamings to allow all callback types #14364
Add method signature changes to CHANGELOG / UPGRADE files.

stof · 2015-04-13T10:54:39Z

We should support all callable types (which is already the case in most places in Symfony). Restricting the callables being allowed would be both a BC break and a WTF for users

stof · 2015-04-13T10:56:25Z

src/Symfony/Component/HttpKernel/Event/FilterControllerEvent.php

        $this->controller = $controller;
    }
-
-    private function varToString($var)


this unused method should actually be removed in older branches

ah no sorry, it was used in the exception message

Also removes tests checking the exceptions thrown from the removed is_callable checks.

mpajunen · 2015-04-14T19:42:28Z

Related changes to support all callable types are now included.
The Console component changes include renaming functions and attributes. If these changes are too extensive, I can remove or rework them.
Changes from [DomCrawler] Callable type hint #14293 are also now included in this PR.

stof · 2015-04-14T21:08:20Z

src/Symfony/Component/Config/Definition/BaseNode.php

     */
-    public function setNormalizationClosures(array $closures)
+    public function setNormalizationCallbacks(array $callbacks)


Renaming the method is a BC break

@stof this is for 3.0 and all these changes in this pr are bc breaks.

but in this case here (which is just about renaming without changed typehint) it is probably better to deprecate the method in 2.8 first and provide the new method. so there is an upgrade path. and then remove the old one in 3.0

@Tobion the typehint changes are BC breaks only for people extending the classes, because of the E_STRICT (and for most places, they probably don't need to extend it). The method renaming is the only BC break affecting the usage of the class.

To make it perfectly clear, we don't want to break BC without providing an upgrade path. In this case, we should introduce the new method in 2.8, deprecate the old one in 2.8, and remove the deprecated on in 3.0.

Ok, I can open a separate PR with the naming changes and BC layer for 2.8.

All Config component changes now in #14364.

fabpot · 2015-10-05T14:12:21Z

This involves a lot of signature changes and so many BC breaks for which we cannot really warn the user beforehand, not sure this is worth it.

nicolas-grekas · 2015-10-05T14:24:58Z

👎 from me: this should be changed on a case by case basis, when someone has an actual use case imo. Doing a global change is more a theoretical standpoint than a practical one.

nicolas-grekas · 2015-10-05T15:03:10Z

See #16125 for the callable type hint

…nicolas-grekas) This PR was merged into the 3.0-dev branch. Discussion ---------- Replace is_callable checks with type hints | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14330 | License | MIT | Doc PR | - Also removes tests checking the exceptions thrown from the removed is_callable checks. Commits ------- 7685cdd Add more callable type hints 4e0c6e1 Replace is_callable checks with type hints

dosten mentioned this pull request Apr 12, 2015

Replaced Closure type hint in the DomCrawler component mpajunen/symfony#1

Merged

stof reviewed Apr 13, 2015
View reviewed changes

Replace is_callable checks with type hints

5356ee3

Also removes tests checking the exceptions thrown from the removed is_callable checks.

mpajunen force-pushed the typehint-callable branch 2 times, most recently from dd38335 to 2425766 Compare April 14, 2015 19:22

stof reviewed Apr 14, 2015
View reviewed changes

dosten mentioned this pull request Apr 14, 2015

[DomCrawler] Callable type hint #14293

Closed

mpajunen mentioned this pull request Apr 15, 2015

[Config] Renamings to allow all callback types #14364

Closed

Replaced Closure type hint in the DomCrawler component

dda1075

mpajunen force-pushed the typehint-callable branch from 2425766 to 2b47634 Compare April 15, 2015 18:13

Change type hints from \Closure to callable

3f7e512

mpajunen force-pushed the typehint-callable branch from 2b47634 to 3f7e512 Compare April 15, 2015 20:35

dosten mentioned this pull request May 20, 2015

[DOMCrawler] Replace \Closure by callable on each and reduce function #14709

Closed

Tobion mentioned this pull request Aug 20, 2015

Callable typehint twigphp/Twig#1779

Merged

fabpot closed this Oct 5, 2015

nicolas-grekas mentioned this pull request Oct 5, 2015

Replace is_callable checks with type hints #16125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use callable typehints #14330

[WIP] Use callable typehints #14330

mpajunen commented Apr 12, 2015

stof commented Apr 13, 2015

stof Apr 13, 2015

stof Apr 13, 2015

mpajunen commented Apr 14, 2015

stof Apr 14, 2015

Tobion Apr 14, 2015

Tobion Apr 14, 2015

stof Apr 14, 2015

fabpot Apr 15, 2015

mpajunen Apr 15, 2015

mpajunen Apr 15, 2015

fabpot commented Oct 5, 2015

nicolas-grekas commented Oct 5, 2015

nicolas-grekas commented Oct 5, 2015

[WIP] Use callable typehints #14330

[WIP] Use callable typehints #14330

Conversation

mpajunen commented Apr 12, 2015

stof commented Apr 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpajunen commented Apr 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 5, 2015

nicolas-grekas commented Oct 5, 2015

nicolas-grekas commented Oct 5, 2015