Skip to content
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

chore: remove all references to callable, replace by closure #323

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

azjezz
Copy link
Owner

@azjezz azjezz commented Dec 21, 2021

Signed-off-by: azjezz azjezz@protonmail.com

Signed-off-by: azjezz <azjezz@protonmail.com>
@azjezz azjezz added Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: Enhancement Most issues will probably ask for additions or changes. Type: BC Break A change that will result in a backward compatibility break in the public API. labels Dec 21, 2021
@azjezz azjezz added this to the 2.0.0 milestone Dec 21, 2021
@azjezz azjezz self-assigned this Dec 21, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1608969450

  • 6 of 6 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0001%) to 99.881%

Totals Coverage Status
Change from base Build 1608848542: 0.0001%
Covered Lines: 3355
Relevant Lines: 3359

💛 - Coveralls

@azjezz azjezz merged commit f7eb6b5 into 2.0.x Dec 21, 2021
@azjezz azjezz deleted the chore/dict/remove-callable branch December 21, 2021 22:48
@veewee
Copy link
Collaborator

veewee commented Dec 22, 2021

Any specific reason for accepting a less broad set of callbacks?

@bendavies
Copy link

bendavies commented Jun 16, 2022

@azjezz any reason for this change? making upgrading quite a chore :)

@bendavies
Copy link

i guess it's because of first-class callables in 8.1

map($api->getAll($params), [PostingPeriod::class, 'fromArray']);

becomes

map($api->getAll($params), PostingPeriod::fromArray(...));

@bendavies
Copy link

bendavies commented Jun 16, 2022

although we did have some callable classes (with __invoke) which we now can't use so nicely.
that needs:

before:

map($value, $this->foo);

after:

map($value, Closure::fromCallable($this->foo));

or

map($value, $this->foo->__invoke(...));

because this doesn't work:

map($value, $this->foo(...));

not very nice

@bendavies
Copy link

i'm an idiot.
this works:

map($value, ($this->foo)(...));

@azjezz
Copy link
Owner Author

azjezz commented Jun 17, 2022

The reasoning behind this change is in revolt-php, which uses Closure for all callbacks within the event loop, we can use Closure in async related functions, but that would be an inconsistency that i would rather avoid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: Completed Nothing further to be done with this issue. Awaiting to be closed by the requestor out of politeness Type: BC Break A change that will result in a backward compatibility break in the public API. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants