8000 [Security] Do not use First Class Callable Syntax for listeners by javer · Pull Request #46260 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Comments

[Security] Do not use First Class Callable Syntax for listeners#46260

Closed
javer wants to merge 1 commit intosymfony:6.1from
javer:no-first-class-callable-for-listeners
Closed

[Security] Do not use First Class Callable Syntax for listeners#46260
javer wants to merge 1 commit intosymfony:6.1from
javer:no-first-class-callable-for-listeners

Conversation

@javer
Copy link
Contributor
@javer javer commented May 5, 2022
Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR fixes usage of First Class Callable Syntax for registering listeners which are intended to be unregistered later.

I'm targeting 6.1 branch because the issue was introduced only in this branch via b0217c6

The reason of this change is clear from the following example:

<?php

class Foo {
    public function bar() {}
}

$foo = new Foo();
var_dump($foo->bar(...) === $foo->bar(...)); // false
var_dump([$foo, 'bar'] === [$foo, 'bar']);   // true

In other words, it's not safe to use First Class Callable Syntax in the cases when we need to refer to the same function to remove it from somewhere, because each method(...) returns a new function.

@javer javer requested review from chalasr and wouterj as code owners May 5, 2022 11:15
8000
@carsonbot carsonbot added this to the 6.1 milestone May 5, 2022
@carsonbot
Copy link
carsonbot commented May 5, 2022

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@javer
Copy link
Contributor Author
javer commented May 5, 2022

Also it seems that CI is broken:

Can't contact LDAP server (-1)
Uncaught UnexpectedValueException: Intersection types not yet supported
Symfony\Bundle\WebProfilerBundle\Tests\Resources\IconTest::testIconFileContents with data set #22516

and so on.

nicolas-grekas
nicolas-grekas reviewed May 5, 2022
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good catch. I don't think this is the patch we want to merge. Instead, remove listener should be able to work with first-class callables. I'm going to submit a PR doing so, so that we can discuss about it.

@nicolas-grekas nicolas-grekas mentioned this pull request May 5, 2022
Merged
@nicolas-grekas
Copy link
Member
nicolas-grekas commented May 5, 2022

See #46262

@chalasr
Copy link
Member
chalasr commented May 5, 2022

Closing in favor of #46262 which reuses the test cases you contributed. Thank you.

@chalasr chalasr closed this May 5, 2022
nicolas-grekas added a commit that referenced this pull request May 5, 2022
@nicolas-grekas
bug #46262 [EventDispatcher] Fix removing listeners when using first-…
b368ce1 
…class callable syntax (javer)

This PR was merged into the 4.4 branch.

Discussion
----------

[EventDispatcher] Fix removing listeners when using first-class callable syntax

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46260
| License       | MIT
| Doc PR        | -

Closures should be compared using non-strict comparison to account for the first-class callable syntax.
See https://github.com/php/php-src/blob/9a90bd705483004c2ef408ee9e9bb0902beade3f/Zend/zend_closures.c#L387-L423

Commits
-------

df4c003 [EventDispatcher] Fix removing listeners when using first-class callable syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof approved these changes

@wouterj wouterj Awaiting requested review from wouterj wouterj is a code owner

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees

No one assigned

Labels

Bug Security Status: Reviewed

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

5 participants

@javer @carsonbot @nicolas-grekas @chalasr @stof
0