Skip to content

[DomCrawler] Callable type hint #14293

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[DomCrawler] Callable type hint #14293

wants to merge 4 commits into from

Conversation

dosten
Copy link
Contributor

@dosten dosten commented Apr 10, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

As of PHP 5.4 exists the callable type hint. Example:

// With the Closure type hint we can only create callbacks in this way...
$crawler->each(function ($node, $i) {});

// With the callable type hint this works too
$obj = new MyClass();
$crawler->each(array($obj, 'myCallbackMethod'));

nicolas-grekas
nicolas-grekas reviewed Apr 10, 2015
View reviewed changes
src/Symfony/Component/DomCrawler/CHANGELOG.md
3.0.0
-----

* The first argument of `Crawler::each()` and `Crawler::reduce()` are type hinted with `callback` instead of `Closure`.
Copy link
Member

Choose a reason for hiding this comment

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

callable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

stof
stof reviewed Apr 10, 2015
View reviewed changes
src/Symfony/Component/DomCrawler/Crawler.php
{
$nodes = array();
foreach ($this as $i => $node) {
if (false !== $closure(new static($node, $this->uri, $this->baseHref), $i)) {
if (false !== $fn(new static($node, $this->uri, $this->baseHref), $i)) {
Copy link
Member

Choose a reason for hiding this comment

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

this does not work for all types of callables. There is still 1 type of callable passing the typehint but failing for the direct call

Copy link
Member

Choose a reason for hiding this comment

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

have you tested the 5 examples in your PR description btw ?

@dosten
Copy link
Contributor Author

dosten commented Apr 11, 2015

After review it again with some tests i can see that only the object method callback type works, so i will update the PR description.

@mpajunen
Copy link
Contributor

All callback types that pass the callable type hint should work, except for a static method string like 'Foo::bar'. For example: http://3v4l.org/r9Tlt

Should the typehints be changed from \Closure to callable elsewhere in Symfony as well? I can provide a PR if this is wanted for 3.0. (At least I would find it useful.)

@stof
Copy link
Member

stof commented Apr 12, 2015

@mpajunen AFAIK, we don't have lots of Closure typehints. We support callables almost everywhere (and we should probably convert the is_callable checks to typehints indeed). And some places where we check for Closure explicitly are intended (in the OptionsResolver for instance).
The fact that DomCrawler restricts the callable being usable here looks like a design mistake for the code, and we should indeed change it for 3.0 (but we should accept any callable, including static method strings, to be consistent with other parts of Symfony)

@mpajunen mpajunen mentioned this pull request Apr 12, 2015
Closed
4 tasks
@dosten dosten mentioned this pull request Apr 12, 2015
Merged
@dosten
Copy link
Contributor Author

dosten commented Apr 14, 2015

Replaced by #14330

@dosten dosten closed this Apr 14, 2015
@dosten dosten deleted the callable-type-hint branch April 15, 2015 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dosten @mpajunen @stof @nicolas-grekas