loophp / collection

A (memory) friendly, easy, lazy and modular collection class.
https://loophp-collection.rtfd.io/
MIT License
721 stars 35 forks source link

Issue with cache and fromCallable #264

Closed jazithedev closed 2 years ago

jazithedev commented 2 years ago

Hey 👋. I have this code:

$test = Collection::fromCallable(static function () {
    yield 100 => 'a';
    yield 200 => 'b';
    yield 300 => 'c';
    yield 400 => 'd';
})->cache();

$result1 = $test->get(100)->current();
$result2 = $test->get(200)->current();
$result3 = $test->get(300)->current();
$result4 = $test->get(400)->current();

With each run, the result is:

image

Using the latest loophp/collection (6.0.3) and PHP 8.1. Is it a bug, or am I doing something wrong 😉?

drupol commented 2 years ago

Hi !

Interesting, let me check this out locally with version 6, master and I'll get back to you.

jazithedev commented 2 years ago

Thank you for VERY fast response @drupol 🍻

drupol commented 2 years ago

You're welcome.

I cannot reproduce the issue with the master branch, everything seems to be ok there.

Going to investigate the issue on version ^6...

drupol commented 2 years ago

This is the commit who fixed the issue a69acec

jazithedev commented 2 years ago

Thanks! I suppose it will be soon pushed on the next library version?

drupol commented 2 years ago

I'm still investigating which lines fixed the issue. I need to provide a fix and release a version 6.0.4 with the fix only.

jazithedev commented 2 years ago

Ok, no worries. I will wait for it then 🙂. Good luck 🤞!

drupol commented 2 years ago

Ok, got the reason.

That commit is actually one of the most important commit. This is mostly the reason why there will be a major version bump.

The sad news is that I'm afraid this won't be fixable in v6 :( it would require to rewrite almost every operations.

The good news is that now I need to hurry and release version 7. I took a bit of delay to finish it. There's still #256 to finish and we're good to go.

jazithedev commented 2 years ago

I see. It is ok. I managed to make a workaround on my side by casting the collection to an array (unfortunately), and getting the value by a key at that point. Keep up the good work, and can't wait for v7 🙂.

jazithedev commented 2 years ago

And, of course, thank you for your help and fast reaction once again 👍️.

drupol commented 2 years ago

I just added your tests in the codebase to make sure it doesn't happen again in v7 :)

Thanks for reporting it !

rela589n commented 11 months ago

Hi there!

Does the cache really work in this case?

$test = Collection::fromCallable(static function () {
    var_dump('data call');

    yield 100 => 'a';
})->cache();

$result1 = $test->all();
$result2 = $test->all();

var_dump($result1, $result2);

This code outputs following result:

string(9) "data call"
string(9) "data call"
array(1) {
  [0]=>
  string(1) "a"
}
array(1) {
  [0]=>
  string(1) "a"
}

It doesn't seem to work correctly, since it calls for data every time :thinking:

drupol commented 11 months ago

This is weird indeed, but this is an expected behavior due to the immutable nature of the Collection.

To make it work "as expected", add the squash operation, use just use it without cache().

However, this issue gaves me an idea to add a new operation memorize that would do that... I just did in 5 minutes, I might probably finalize it next week (see https://github.com/loophp/collection/pull/316).