[5.4] Add tap method to collection

[arjasco]

I noticed the tap function in helpers and thought it would be pretty useful in collections. The purpose is to "tap into" method chains without modifying the result of the previous call, we seem to do a lot of chaining with collections.

Creating a new collection to pass to the callback because there are some methods which if called on the collection will change the items. eg. the pop method.

[taylorotwell]

Interesting. Can you share the real use case you came across for this?

[GrahamCampbell]

cs

[taylorotwell]

Nobody knows what "cs" means.

[arjasco]

My use case for this was to help with debugging a huge series of collections without breaking them apart, reassigning them another var. This also means i can perform some extra logic without affecting the next result

Quick example maybe:

collect()
    ->sort(...)
    ->tap(function ($collection) {
        Log::info($collection->toArray()));
    })
    ->groupBy(...)
    ->tap(function ($collection) {
        dd($collection);
    })

You could potentially tap into a collection chain and fire events with the data at the current point.

collect($memebers)
    ->where('active', 1)
    ->tap(function ($collection) {
       $this->dispatch(new ActiveMembersEvent($collection));
    })
    ->where('plan', 1)
    ->tap(function ($collection) {
       $this->dispatch(new Plan1SpecifcMembersEvent($collection));
    })

Testing, making some assertions at various stages could also be a use.

@JosephSilber Good point, I did look at this but wasn't sure exactly if this was correct. There is actually a bit of a divide across what tap should actually do. Some say it should tap into a method chain so you can perform some action with the data at that specific point in the chain without affecting the result of the previous method as it gets passed onto the next. Yet some say it should be able to modify the result before it's passed onto the next.. which sounds more like a pipeline to me.

The use of the tap helper confuses me when used in some areas of the framework anyway.
https://github.com/laravel/framework/blob/5.4/src/Illuminate/Foundation/AliasLoader.php#L89

loadFacade doesn't actually return anything (says bool), if it did it would actually be a string (the path to the facade) so why do we need tap? I've seen some others that the callback doesn't even use the value thats passed to it...

I believe some of it's other use is just to tidy up on not having var assignments and then returning..

[sebdesign]

I'm here just to say that ->tap('dump')-> has been so useful lately!

[JosephSilber]

I came here to weep 😢

Creating a new collection to pass to the callback...

This is just soooo wrong 😞

It's counter-intuitive, and contrary to all other taps.

...because there are some methods which if called on the collection will change the items. eg. the pop method.

Exactly. So if you call a mutating method within tap, you expect the original collection to be mutated.

And if you don't want to mutate the original, you can always clone the collection within your tap.

---

I just lost over 2 hours tearing my hair out on this. So frustrating 😥

After figuring out what's happening, I had to switch to using pipe, and returning the original instance:

framework/tests/Support/SupportLazyCollectionTest.php

Lines 167 to 180 in 5e918ed

	->pipe(function ($collection) use ($mock, $timeout) {
	tap($collection)
	->mockery_init($mock->mockery_getContainer())
	->shouldAllowMockingProtectedMethods()
	->shouldReceive('now')
	->times(3)
	->andReturn(
	(clone $timeout)->sub(2, 'minute')->getTimestamp(),
	(clone $timeout)->sub(1, 'minute')->getTimestamp(),
	$timeout->getTimestamp()
	);
	return $collection;
	})

Really not what you'd expect!

---

We should change this in the next major version, to pass the collection itself as is.

[driesvints]

@JosephSilber think I agree. Would you be up for sending in a PR?

[arjasco]

Ooooh, this is old 😀

@JosephSilber I agree, at the time of the implementation my thought process was "The ability to tap into the collection without mutating the original in a chain of method calls". You are right to say this isn't inline with the other "tap" methods or the original Ruby implementation as a matter of fact. I can't say I have used this since submitting the PR, it was useful at the time debugging and partially filtering a large chained collection in an adopted code base, I rarely use plain collections these days.

Surprised it wasn't picked up on quicker 😅