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

Extending Collection causes infinite loop #17483

Open
LordSimal opened this issue Dec 11, 2023 · 5 comments
Open

Extending Collection causes infinite loop #17483

LordSimal opened this issue Dec 11, 2023 · 5 comments

Comments

@LordSimal
Copy link
Contributor

Description

Original Thread: https://discourse.cakephp.org/t/extended-collection-class-not-behaving-the-same-as-base-collection-class/11734

As can be seen in the mentioned forum link (thanks @Zuluru) if one extends a collection it behaves differently (in this specific case it causes an infinite loop)

I'll just copy/paste the example in here:

<?php
namespace App\Test\TestCase;

use Cake\Collection\Collection;
use PHPUnit\Framework\TestCase;

class MyCollection extends Collection { }

class CollectionTest extends TestCase
{
    public function testIterators(): void {
        $records = [
            ['field' => true],
            ['field' => false],
            ['field' => true],
        ];

        $collection = new MyCollection($records);
        $count = 0;
        foreach ($collection as $investment) {
            $count ++;
            if ($count > 20) {
                break;
            }
            if ($collection->every(fn ($record) => $record['field'] === true)) {
                // Nothing, really, just need to execute the every() call
            }
        }

        $this->assertEquals(3, $count);
    }
}

if you change new MyCollection to new Collection it works just fine.

My guess is related to this line: https://github.com/cakephp/cakephp/blob/5.x/src/Collection/CollectionTrait.php#L937

But I don't use collections and have not yet doven into this black magic.

CakePHP Version

5.0.3

PHP Version

8.2

@LordSimal LordSimal added this to the 5.0.4 milestone Dec 11, 2023
@othercorey
Copy link
Member

othercorey commented Dec 15, 2023

Looks like that assumes Collection. I'm not a big fan of these traits, but think we can fix this.

@othercorey othercorey modified the milestones: 5.0.4, 4.5.3 Dec 15, 2023
@othercorey othercorey self-assigned this Dec 15, 2023
@othercorey
Copy link
Member

Extending Collection outside of cake is not possible because we have logic that work sonly for Collection and only for the custom iterators that extend Collection like ExtractIterator.

Because ExtractIterator overrides, unwrap(), we cannot call getInnerIterator() on it.

@Zuluru
Copy link
Contributor

Zuluru commented Dec 18, 2023

Well, that's unfortunate. :-( I'll have to try to think of a way to encapsulate it instead of extending, I guess. The class should probably be marked as final?

@othercorey
Copy link
Member

We have to change the iterators to not extend Collection to mark it final.

Will keep this open while we look into if the interfaces or inheritance can change in 5.1.

@othercorey othercorey modified the milestones: 4.5.3, 5.1.0, 6.0 Dec 21, 2023
@othercorey othercorey removed their assignment Dec 27, 2023
Copy link

This issue is stale because it has been open for 120 days with no activity. Remove the stale label or comment or this will be closed in 15 days

@github-actions github-actions bot added the stale label Apr 30, 2024
@othercorey othercorey added pinned and removed stale labels Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants