Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[9.x] Split Conditionable into package #38457

Merged
merged 4 commits into from
Aug 24, 2021

Conversation

inxilpro
Copy link
Contributor

This is a potential solution to #38389.

Realistically, anything that illuminate/collections depends on (that's also used elsewhere in the framework) is going to bump into this same issue. It's the same reason the Macroable trait had to be moved into its own package.

The other approach would be to just move Conditionable into the Collections package, but that just feels wrong to me. In the end, I decided to PR this as a solution. If you decide it's not worth breaking out another package, I'll happily re-submit the other solution.

@JosephSilber
Copy link
Contributor

Where does it end though? The HigherOrderCollectionProxy also doesn't really have anything to do with collections. It should really be called HigherOrderProxy, and accept any generic type.

Wouldn't it make more sense to have some catch-all package, like Traits or Helpers or whatnot, where we can store these lower level things? So Macroable and Conditionable and HigherOrderProxy and HigherOrderWhenProxy can all live in the same package together?

@GrahamCampbell
Copy link
Member

illuminate/utils?

@inxilpro
Copy link
Contributor Author

I like that. But at what point are we just re-creating illuminate/support?

Would a better approach be to extract some things out of that package, and then make the collections package depend on it again?

It feels like illuminate/support is exactly the right place for this kind of code.

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Aug 23, 2021

I firmly believe illuminate/support should be a dependency for other packages. It's okay for illuminate/collection to require it. Otherwise, we may land on the circular dependency hell.

@driesvints
Copy link
Member

It's okay for illuminate/collection to require it.

The entire point of illuminate/collection was that it didn't depend on illuminate/support.

Honestly I'm beginning to think splitting illuminate/support was a mistake and we should just move everything back into one component.

@taylorotwell
Copy link
Member

This PR has conflicts.

@DarkGhostHunter
Copy link
Contributor

DarkGhostHunter commented Aug 23, 2021

At this point, for a code to have their own package, it should comply with the following:

  1. Does not depend on illuminate/support functionality.
  2. The dependent functionality can be replicated by just copy-pasting one or two methods.

For the case of Conditionable, if you replicate on Collection without the trait, it will break the trait check for conditions:

if (in_array(Conditionable::class, class_uses_recursive($object)) {
    $object->when(true, ...);
}

In that case, you're bound to create a Conditionable contract. Seems very over-engineered to me.

Again, if a single part of the code requires illuminate/support to work, is no sin, as long is big enough (like Collections). Conditionable is merely a trait, not a class comprised of enough code that can't fit one screen. To me, illuminate/support is like a global toolbox for small utilities that can be dropped-in my project. The Collection class grew enough to guarantee a package for itself.

@taylorotwell
Copy link
Member

@inxilpro can you resolve the conflicts on this?

@JosephSilber
Copy link
Contributor

@taylorotwell what are your thoughts on this #38457 (comment)?

I would really like to eventually rename HigherOrderCollectionProxy to HigherOrderProxy. I've had many generic use-cases for it in the past.

@inxilpro
Copy link
Contributor Author

I've resolved the conflicts. I think there are two viable paths:

  1. Bring everything back into illuminate/support
  2. Commit to "support" functionality getting split into smaller chunks over time. I don't think there's anything wrong with this, and it could be nice to have the option to just install illuminate/proxies and illuminate/macroable if that's the only "support" functionality that you need in your app.

After thinking about it a bit longer, it feels like moving away from a catch-all "support" package into more tightly scoped feature packages is a good thing. In fact, I think that in relationship to @JosephSilber's comment, it makes a lot of sense to introduce an illuminate/proxies package, too, that holds generic versions of various proxy objects (something like, HigherOrderMethodProxy, HigherOrderObjectProxy, HigherOrderEnumerableProxy, and HigherOrderConditionableProxy).

@taylorotwell
Copy link
Member

Yeah I don't see what the big controversy is with splitting this out into a package.

@taylorotwell taylorotwell merged commit 792e42c into laravel:master Aug 24, 2021
@driesvints
Copy link
Member

Thanks @inxilpro

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

Successfully merging this pull request may close these issues.

6 participants