Skip to content

Conversation

@dlundgren
Copy link

This should fix #1535.

@Ocramius Ocramius added this to the 6.66.0 milestone Oct 27, 2025
@Ocramius
Copy link
Member

I think the patch is fine, but I'm not 100% sure if we should return closures at all, as described in #1535.

WDYT, @asgrim, @kukulich?

Should closures be part of the iterated-upon sources? Is this going to cause downstream regressions? 🤔

@asgrim
Copy link
Member

asgrim commented Oct 27, 2025

IMO think it should be fine; the only thing I'd ask for is a test to show closures/arrow functions within a class method, or even within another function, are returned. Definitely would want to check @kukulich 's thoughts too in case this impacts phpstan however.

@dlundgren
Copy link
Author

Thanks for reviewing this.

Since I hadn't thought about adding a test from within a class, when I added the following tests, they passed...

<?php class foo { function foo() { fn() => "foo"; } }
<?php class foo { function foo() { $this->test(fn() => "foo"); } }
<?php class foo { function foo() { $this->test(function() { return "foo"; }); } }

However, when I ran the change against the directory I'm working with, the ReflectionFunction had no idea that the closure was within a class, or even a method parameter. Without knowing this information, at least for my use case, I'm not sure if this change makes sense.

I'm happy to add the tests in this comment if this is still wanted.

@ondrejmirtes
Copy link
Contributor

Hi, what is this going to change for users of BR? How is this going to change what Reflector returns from reflectFunction? What is the name of these anonymous functions? Is it even unique?

@Ocramius
Copy link
Member

Ocramius commented Nov 4, 2025

I'm on the same page with @ondrejmirtes here - need to IMO step back a second and look at this from a use-case perspective :D

@Ocramius Ocramius removed this from the 6.66.0 milestone Nov 4, 2025
@dlundgren
Copy link
Author

To answer @ondrejmirtes questions:

  • Users would get a wonderful (j/k) array of functions that have no real names to reference them, at least as-is.
  • reflectFunction probably won't work reliably,
  • The names are definitely not unique, which is part of the complication, and I realized this after I had submitted this.

simple test:

$a = fn() => 'kakaw';
$b = fn() => 'test';

function test($cb) {}

test(fn() => 'kakaw');
test(function () {return 'kakaw';});

From reflectAllFunctions the names end up as, the only useful one being test

{closure}
{closure}
test
{closure}
{closure}

@Ocramius I took that that step back, and it helped me realize this doesn't make sense even for my use-case. I thought about allowing functions to be returned from a require/include, but that is ambiguous when I need the certainty of an interface in what is being returned.

Thanks for the questions and review on this, I'm going to close it as it's not a feature complete solution, and as-is will cause more problems for those using BR than it's likely to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FindReflectionsInTree doesn't find closures

4 participants