[BC] Conflict with Laravel Collection part 2 !

[mathieutu]

Hi,
Here a conflict with the Arrayable interface.

I have a Laravel Collection with several Tightenco collection inside.

When using toArray() on the main Laravel Collection, it doesn't touch the Tightenco ones... 😞

It's because of the \Illuminate\Contracts\Support\Arrayable and \Tightenco\Collect\Contracts\Support\Arrayable Interfaces.

Because of these sort of bugs, packages that use Tightenco/collect can't be used with Laravel anymore... 😥

[Isinlor]

I'm pretty sure that @taylorotwell could extract contracts that collections are depending on to a separate package without affecting Laravel.

BTW - There was also an old proposal to deal with problem like that on PHP level: https://wiki.php.net/rfc/protocol_type_hinting

In short, rather than checking the type-tree as traditional type-hinting works, this only checks to see that the public APIs match (that if you added “implements $interface” to the class definition, if it would pass)…

But it was withdrawn.

[mattstauffer]

@mathieutu I think we have an open issue about this--I'll take a look ASAP. We're trying to figure out a good solution, but it's definitely tough.

@Isinlor Please feel free to read the link I gave you in #105 to see the history, but in short, we need to imagine situations in which different versions of Collect and Laravel are in the same project. They need to be able to be separate because the API may have changed between those versions, so any time the two interact with each other and rely on reflection, we're going to feel a pain like this. That doesn't mean it's not fixable, but it's not just as simple as "Taylor could extract these." We've already tried that, working with Taylor, before we split out Collect. It was messy. We (together with Taylor) decided against it.

[mattstauffer]

(nope, the one I saw was your first one.)

We dug into a bit at our company on-site a few weeks ago and we fixed all of the problem except these namespace issues. Thanks for bring it back up!

[Isinlor]

@mattstauffer Yes, thanks for the link. It's a good read.

Do you have some link to the efforts you took with Taylor? I would be interested to read these too if they are available.

I understand that extracting concrete implementations may lead to mess in Laravel, since there seems to be quite a lot cross dependencies.

But I think it could be a solution in the case of contracts required by this package and avoid pitfalls that you linked too.

Based on this file, the three contracts in question are:

contracts=(
        'Contracts/Support/Arrayable'
        'Contracts/Support/Jsonable'
        'Contracts/Support/Htmlable'
)

The interfaces seem to be pretty stable and standalone. Last substantial change was 4 years ago involving renaming. There isn't really that much that can change.

• Arrayable
• Htmlable
• Jsonable

I would think that extracting these - it doesn't even need to be done by Laravel - and implementing them next to the current ones should just work. It wouldn't be even a braking change, would it?

[mattstauffer]

@Isinlor No link, sorry.

And, frankly, there's no compelling reason for Laravel to extract these packages or to maintain references to copies someone else extract just for the sake of our library working. It's a nice thought, but it's just not going to happen. We even had a single line in Laravel's composer.json referencing Collect as a replacement and deeply regretted it later. The conclusion from that was: Laravel shouldn't have anything in its codebase that exists for the purpose of other projects like Collect.

[mathieutu]

For this particular issue, it could be possible to check if the Laravel interfaces are available and make some class aliases, or replacements?

Maybe we can even make a service provider that will be registered only in Laravel projects, and execute inside some processes which permit to avoid all the conflicts?

[mathieutu]

@mattstauffer just a little up on this one to ask for your thoughts about the ideas in my last message.

[mattstauffer]

@mathieutu I'm very happy to consider that idea--I think we went down that road and hadn't yet come up with a concrete description of exactly what actions should be performed in that instance. But I love the idea and I hope that's the root of our solution.

[mathieutu]

Hey @mattstauffer (and @antonioribeiro).
Now the PR is aborted, what do you think about a simple class_alias in a service provider? I've tested it in some projects, and it seems to be working pretty well.

It'll completely replace Laravel collection with tigtenco ones. We could let an optout via the config, and so it will solve all the problem we have.

What are your thoughts about that guys?

[mattstauffer]

It's been a long time since I've really thought of that. So I can't say for sure.

My quick response is that I'd be worried about what that does when a library imported might expect to be using Collect, or your internal code is expecting to be using Illuminate Collections, and you have different versions of Collect and Illuminate Support.

[LarryBarker]

I have been trying desperately to install okta/sdk, which requires this package, on a Laravel 5.5 application but to no avail. Okta says they have fixed it by updating to your latest release; however, this does not have seemed to resolve the issue. Does anyone have any suggestions on how I can address the issue? Any feedback is appreciated. Thanks much!

[mattstauffer]

@LArbearrr Please open a new issue. Thanks!

[mathieutu]

@mattstauffer unfortunately it can be the same issue, as tightenco/collect isn't fully compatible with Laravel anymore.

[mattstauffer]

@mathieutu

1. it could be the same issue but, as you can see in the issue he opened (Unable to use with Laravel 5.5 through okta/sdk #176) I needed a lot of info from him that i didn't want to clutter this issue with those

2. I'm not sure what you mean it's not fully compatible?

Thanks!

[mathieutu]

@mattstauffer

1. Ok, it's not the same pb.

2. Because of both issues I opened a while ago on this repo, there are still issues when using collect (in a package installed) in a laravel project.

The conflicts between Illuminate and Tightenco collections leads to real bugs in Laravel projects.

[mattstauffer]

@mathietu we have years of back and forth and at least one failed major PR to the framework. If you are still concerned with the current state I’d love for you to close the existing issues and create new fresh ones with the state of the issue as it exists today with links to the originals. Thanks for your help!

[mathieutu]

@mattstauffer The issues are still exactly the same than before. I can close and reopen them, but they will be totally the same.

[mattstauffer]

@mathieutu You obviously have more time and energy to consider these things than I do. If you want to take some work to collect the A) problem and B) solutions I would be more than happy to look at it immediately. If you need me to read through every idea and every comment and figure out which are and aren't relevant for me to make decisions today then I'm not going to do that and we're going to stay in exactly the same situation we're in.

This is an open source package. I am making time out of my day and taking brain space out of my head. If you want to help me help you I would appreciate the help. If you don't then the situation will stay exactly as it is.

[stevebauman]

Reading over the PR's and issues about this, maybe I'm misunderstanding something, but I don't think Laravel collections and Tightenco collections should be mixed together or treated identically / similarly. If you treat them as separate packages then there isn't an issue.

In my opinion, I don't think class aliases should be used to auto-magically use the Illuminate namespace to create Tightenco collections.

@mathieutu If a package you are using is creating Tightenco collections, you should be creating a Tightenco collection to contain Tightenco collections. That, or creating your own Collection class in your application that extends the Illuminate one, and overriding the getArrayableItems method to support converting Tightenco collections into Laravel ones. There is no way around this.

If you don't treat them independently then of course you'll run into bugs and problems.