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

Allow ArrayCollection#filter() to filter by key and also value #167

Merged
merged 5 commits into from Oct 15, 2018
Merged

Allow ArrayCollection#filter() to filter by key and also value #167

merged 5 commits into from Oct 15, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 15, 2018

Uses ARRAY_FILTER_USE_BOTH in the underlying array_filter function. Does
not cause BC Break.

Any examples where how to run benchmarks against this?

#165

Uses ARRAY_FILTER_USE_BOTH in the underlying array_filter function. Does
not cause BC Break
});
self::assertEquals([0 => 1, 2 => 3], $res->toArray());
self::assertEquals([0 => 1, 2 => 3, 4 => 5], $res->toArray());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do add a test: don't modify existing tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

@Ocramius
Copy link
Member

Also to be checked: preservation of order, preservation of key and value types (use self::assertSame() rather than self::assertEquals())

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Ocramius Ocramius changed the title Allow ArrayCollection#filter to filter by key or value. Allow ArrayCollection#filter() to filter by key and also value Oct 15, 2018
@Ocramius Ocramius added this to the 1.6.0 milestone Oct 15, 2018
@Ocramius Ocramius self-assigned this Oct 15, 2018
@Ocramius Ocramius merged commit 92ab93e into doctrine:master Oct 15, 2018
@Ocramius
Copy link
Member

Thanks @0x13A!

@ghost
Copy link
Author

ghost commented Oct 15, 2018

cheers

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

Successfully merging this pull request may close these issues.

2 participants