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

[VarDumper] fix dump of closures created from callables #29054

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Member

nicolas-grekas commented Nov 1, 2018

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

We are missing displaying full information about closures created using ReflectionMethod::getClosure() or Closure::fromCallable().

This PR fixes it. For VarDumper but also other places where we have logic to display them.

@nicolas-grekas nicolas-grekas merged commit 1c1818b into symfony:3.4 Nov 6, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Nov 6, 2018

bug #29054 [VarDumper] fix dump of closures created from callables (n…
…icolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[VarDumper] fix dump of closures created from callables

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

We are missing displaying full information about closures created using `ReflectionMethod::getClosure()` or `Closure::fromCallable()`.

This PR fixes it. For VarDumper but also other places where we have logic to display them.

Commits
-------

1c1818b [VarDumper] fix dump of closures created from callables

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:vd-from-callables branch Nov 6, 2018

@stof

This comment has been minimized.

Member

stof commented Nov 6, 2018

the new behavior is tested in VarDumper, but not in other places

nicolas-grekas added a commit that referenced this pull request Nov 11, 2018

minor #29172 [Fwb][EventDispatcher][HttpKernel] Fix getClosureScopeCl…
…ass usage to describe callables (ogizanagi)

This PR was merged into the 3.4 branch.

Discussion
----------

[Fwb][EventDispatcher][HttpKernel] Fix getClosureScopeClass usage to describe callables

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29054   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

`\ReflectionFunctionAbstract::getClosureScopeClass` returns a `\ReflectionClass` instance, not the class name.

Before this patch:

```diff
--- Expected
+++ Actual
@@ @@
-'Symfony\Component\EventDispatcher\Tests\Debug\FooListener::listen'
+'Class [ <user> class Symfony\Component\EventDispatcher\Tests\Debug\FooListener ] {
+  @@ [...]/src/Symfony/Component/EventDispatcher/Tests/Debug/WrappedListenerTest.php 28-33
+
+  - Constants [0] {
+  }
+
+  - Static properties [0] {
+  }
+
+  - Static methods [0] {
+  }
+
+  - Properties [0] {
+  }
+
+  - Methods [1] {
+    Method [ <user> public method listen ] {
+      @@ [...]/src/Symfony/Component/EventDispatcher/Tests/Debug/WrappedListenerTest.php 30 - 32
+    }
+  }
+}
+::listen'
```

Commits
-------

61e4592 [Fwb][EventDispatcher][HttpKernel] Fix getClosureScopeClass usage to describe callables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment