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

[9.x] Universal HigherOrderWhenProxy #37632

Merged
merged 3 commits into from
Jun 10, 2021

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Jun 8, 2021

This PR picks up the work in #37504 and #37561 to unify the behavior of when() and unless() across every implementation by:

  • Expanding HigherOrderWhenProxy to support any proxied object, not just Enumerables
  • Adding support for HigherOrderWhenProxy in the Conditionable trait
  • Replacing the implementation of when() and unless() in EnumeratesValues with Conditionable

It's necessary to implement these changes in 9.x because:

  1. There were slight signature differences between the when() method on Enumerable and the when() method on Conditionable (related to type hints)
  2. The signature of HigherOrderWhenProxy::__construct needs to be changed slightly to allow for any type of proxied object
  3. The return value of EnumeratesValues::when was not defaulted to $this like it is in Conditionable

I want to call out that third item because it is a subtle change.

In Laravel 8.x, the following code will return null. After this PR, the same code will return the Collection with "c" appended to it. I think that is preferable, and means that when() behaves the same way everywhere in the framework.

$results = collect(['a', 'b'])
  ->when(true, function($collection) {
      $collection->push('c');
  });

$results === null; // In Laravel 8
$results instanceof Collection; // In Laravel 9 after this PR

Copy link
Member

@JosephSilber JosephSilber left a comment

Choose a reason for hiding this comment

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

This looks great 👍

Was actually my first thought when I saw the new Conditionable trait.


My only gripe is that it feels weird having this now-totally-generic HigherOrderWhenProxy living under the Collections package, but I don't have a better solution ATM.

tests/Support/SupportCollectionTest.php Outdated Show resolved Hide resolved
tests/Support/SupportConditionableTest.php Outdated Show resolved Hide resolved
@driesvints
Copy link
Member

@inxilpro this PR seems to have coupled the Support component to the Collections component again because Conditionable is now used in the Collections component. We splitted the Collections out of the Support package for the exact reason to never do this. We'll either need to revert this PR or think of a way to do the conditionable features from this PR differently.

@inxilpro
Copy link
Contributor Author

Hm. Give me a bit to see if I can come up with something.

@JosephSilber
Copy link
Member

JosephSilber commented Aug 18, 2021

Can't Conditionable move under Collections, just like the HigherOrderWhenProxy?

@inxilpro
Copy link
Contributor Author

Can't Conditionable move under Collections, just like the HigherOrderWhenProxy?

Yeah, that would work. See my comment on #38389 — the only issue is that it doesn't exactly "belong" in the collections package, since it's more general than that, but moving it into that package is the simplest solution.

@franzliedke
Copy link
Contributor

Would it be possible to execute the tests for each package in isolation?

That could help noticing such hidden dependencies more quickly. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants