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

RemoveElement return type nullable? #413

Open
bOmBeLq opened this issue Apr 19, 2024 · 2 comments
Open

RemoveElement return type nullable? #413

bOmBeLq opened this issue Apr 19, 2024 · 2 comments

Comments

@bOmBeLq
Copy link

bOmBeLq commented Apr 19, 2024

Hello,
so here is issue in doctrine with LazyCollections doctrine/orm#8739 (comment)
Long story short if collection is lazy and we call remove() ideally we don't want to load whole collection into memory.
This can be implemented but there is issue with return type of remove(). When calling it on lazy collection we don't know if element is within collection or not. Therefore cannot return bool true/false.
So ideally I'd change Collection::removeElement : bool to Collection::removeElement : bool|null with description that null is returned if removal cannot be confirmed (eg. lazy collection)
Does that make sense?

@derrabus
Copy link
Member

I don't think I want to widen the return type for this. Would it be a problem if your lazy implementation would always return true as long as it hasn't been initialized?

@bOmBeLq
Copy link
Author

bOmBeLq commented Jan 16, 2025

Sorry got consumed in other stuff but I see issue is still open.
Returning always true can be misleading. And a silent BC break so I'm not sure if that would be good idea.

if($cart->somCollection->remove($someObject)) {
    $cart->totalCost -= $someObject->cost;
}

If lazy collection returns true this condition suddenly changes behaviour. Probably normally a contains() will be called beforehead but you cannot be sure.
I guess eventuallyRemove or LazyRemove to LazyCollection could be way to go.

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

No branches or pull requests

2 participants