Skip to content

Comments

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

Merged
Ocramius merged 5 commits intomasterfrom
unknown repository
Oct 15, 2018
Merged

Allow ArrayCollection#filter() to filter by key and also value#167
Ocramius merged 5 commits intomasterfrom
unknown repository

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
return is_numeric($v) && $k % 2 === 0;
});
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