Add `each` method to the ArrayCollection

[taylankasap]

Is there a reason for not adding it?

[Ocramius]

BC compat, mostly. Collection interface is closed for modification.

[soullivaneuh]

Collection interface is closed for modification.

Why not start implementation and add the method to the interface on next major?

[Ocramius]

It would be interesting, but it would also have a high impact on the ecosystem, making upgrades very painful. The collections API as-is needs some sort of redesign also to get rid of this kind of major BC break bumps for every new feature that is introduced.

[soullivaneuh]

making upgrades very painful.

Why? Just one method to add for people implementing the interface directly, nothing to do for people using doctrine classes.

The collections API as-is needs some sort of redesign also to get rid of this kind of major BC break bumps for every new feature that is introduced.

Which kind of redesign?

BTW, if you don't want to create a new major right now, you can implement the each method on each abstract implementing the interface and add the method to the interface but commented with a remember comment.

Then when you will release the next major, uncomment the method.

Not a very beautiful method, but this is the best way we found on sonata projects for now (cc @greg0ire @core23).

[Ocramius]

Which kind of redesign?

We looked a lot at things like https://github.com/morrisonlevi/Ardent - the collection API needs some sort of differentiation between dictionaries, arrays, stacks, linked lists, etc. The reason behind that being that the current logic is all crammed together into a single object that is just PHP arrays (again, sigh).

BTW, if you don't want to create a new major right now, you can implement the each method on each abstract implementing the interface and add the method to the interface but commented with a remember comment.

If we do that, people will start relying on a concrete implementation rather than an abstraction, and that would be more harmful to the ecosystem

[greg0ire]

If we do that, people will start relying on a concrete implementation rather than an abstraction, and that would be more harmful to the ecosystem

Alternatively, you can add a new interface, and merge it with the other on next major (or keep it separate if it makes sense).

[stof]

@Soullivaneuh all the Doctrine O*M projects are containing their own implementations of the collection API, and when using them, you have no guarantee about which implementation of the interface you will get. So adding a new method only in ArrayCollection here would make it unusable (if you use it, it will break when the ORM uses its own implementation). And the different O*M projects are maintained by different teams, making it harder to enforce implicit contracts

[soullivaneuh]

And the different O*M projects are maintained by different teams, making it harder to enforce implicit contracts

So I see two solutions:

• Add a new Interface as @greg0ire suggested and deprecate the old one
• Just add the method directly on next major

[greg0ire]

Add a new Interface as @greg0ire suggested and deprecate the old one

I was Actually thinking of doing this on next minor

interface NewInterface
{
    public function newMethod();
}

class ArrayCollection implements NewInterface, OldInterface
{
}

And optionally do this on next major, if keeping several interfaces makes no sense (Does Interface Segregation Principle apply here ?)

interface OldInterface
{
    // other methods…
    public function newMethod();
}

class ArrayCollection implements OldInterface
{
}

I can see how this is not great because NewInterface would be sort of instantly deprecated

Client code would then look like this :

public function myMethod(OldInterface $collection)
{
   // do some stuff

   if ($collection instanceof NewInterface) { // this line is not forward-compatible :(
      // yaaaay, can use new method
   } else {
      // old method, to be removed on next major when we rely on doctrine/collection ^$nextMajor
   }
}

Problem : this is not very compatible with deprecation warnings and smooth upgrade, unless you deprecate NewInterface in next major instead of right now.

[justin-oh]

If an "each" method does get added, it might be worth adding other helper methods. For example:

// implementation
public function sum(Closure $p)
{
    $sum = 0;

    $this->each(function($key, $element) use ($p, &$sum) {
        $sum += $p($key, $element);
    });

    return $sum;
}

// usage
$sum = $this->collection->sum(function($key, $element) {
    return $element->getSomeValue();
});

Although, this "sum" helper is only a slight improvement over:

$sum = 0;

$this->collection->each(function ($key, $element) use (&$sum) {
    $sum += $element->getSomeValue();
});

[alexbonhomme]

Hey @taylankasap !

I think the method your looking for is name forAll()

The method should be named forEach() imho

[stof]

Any utility methods taking a closure as argument would be methods that cannot be implemented in optimized way by persistent collections (as the closure can do anything). And so having them in the Collection interface does not have much benefits over having a separate function taking the collection and the callable. But, it has drawbacks: adding methods in an interface requires a new major version because it is a BC break.

so I would not be in favor of adding such utilities in the Collection interface.

[VincentLanglet]

Any utility methods taking a closure as argument would be methods that cannot be implemented in optimized way by persistent collections (as the closure can do anything). And so having them in the Collection interface does not have much benefits over having a separate function taking the collection and the callable. so I would not be in favor of adding such utilities in the Collection interface.

There are however a filter or a map function.

You can write

$collection->map($callable)

But have to

array_reduce($collection->toArray(), $callable)

It's not consistent :/

But, it has drawbacks: adding methods in an interface requires a new major version because it is a BC break.

It's not like it would be a big major version.

[rogerzanelato]

Hey @taylankasap !

I think the method your looking for is name forAll()

The method should be named forEach() imho

I would just like to add a note for anyone who see this comment: forAll is not equivalent of PHP foreach.

forAll goal is to compare if all itens in the collection satisfy a given predicate. So, it will execute the Closure on each item while true is being returned, if the closure returns something false or void the loop breaks and false is returned, which is in agreement with the docs:

Tests whether the given predicate holds for all elements of this collection.

[VincentLanglet]

I feel like it's too bad to not create a new major version with useful method added to the collection interface.

There is a method contains, exists or filter and a method first, but no method find, which could be more performant.

It could also avoid some usage of the 'toArray' method. For example if we add a reduce method.

[greg0ire]

I feel like it's too bad to not create a new major version with useful method added to the collection interface.

That's an issue we have both in Doctrine and Sonata: major versions are a big deal. They wouldn't be if we had them more frequently (or at all, really).

[VincentLanglet]

That's an issue we have both in Doctrine and Sonata: major versions are a big deal. They wouldn't be if we had them more frequently (or at all, really).

I saw multiple PR on master, so I took all my hopes and create one too with the methods I find useful: https://github.com/doctrine/collections/pull/252.

I know doctrine/orm next major is a big deal, but I have trouble to see what is big for doctrine/collections.

To me doctrine could

• Add the methods to the Collection interface and the ArrayCollection
• Release a new major
• Add the implementation on doctrine/orm (and so on) to add support to the new major.

[greg0ire]

I have trouble to see what is big for doctrine/collections.

I don't see any blocker either :)

[stof]

forAll goal is to compare if all itens in the collection satisfy a given predicate. So, it will execute the Closure on each item while true is being returned, if the closure returns something false or void the loop breaks and false is returned, which is in agreement with the docs:

then, the naming is bad. forAll tells me that this executes the callback for all items, not that it checks that all items are accepted by the callback (the for part of the name is the culprit here)

[VincentLanglet]

forAll goal is to compare if all items in the collection satisfy a given predicate. So, it will execute the Closure on each item while true is being returned, if the closure returns something false or void the loop breaks and false is returned, which is in agreement with the docs:

then, the naming is bad. forAll tells me that this executes the callback for all items, not that it checks that all items are accepted by the callback (the for part of the name is the culprit here)

A lot of method could be improved.

The add method is taking an $element. The contains method too and there is a containsKey method.

But the remove method is taking a $key as argument and there is a removeElement method. There should be a remove method and a removeKey method instead.

The method forAll could be named every.

[stof]

@VincentLanglet changing existing methods requires BC breaks for packages consuming collections, which is much harder for the ecosystem than BC breaks for packages implementing collections (which is the kind of BC break for adding a new method). So cleaning the existing API would be quite hard. But that does not mean that any new method that would be added in a 2.x should not be careful about its naming and API.

[someniatko]

I wish PHP had something like extension methods from C#, or maybe the pipe operator would make a deal.

[aimeos]

The Doctrine collection API is completely frozen for years and there won't be any major improvements (even not in 2.0) due to the ecosystem and other Doctrine projects relying on exactly that API according to what @Ocramius, @stof and @greg0ire said.

If you want a feature-rich and extensible collection API, you can try the PHP Map package in addition. Converting a Doctrine collection into a Map is just a matter of:

$m = map( $collection );

and you have all named methods available (and 100+ more):

$v = $m->each(function($val, $key) {
   return -$val < 0;
})->reduce(fuction($result, $val) {
  return $result + $val;
});

// or use
$v = $m->every(function($val, $key) {
   return $val > 0;
})->sum();

You can also extend the Map objects dynamically by your own methods like @someniatko mentioned, e.g.

Map::method( 'strrev', function( $sep ) {
    return strrev( join( '-', $this->list ) );
} );
Map::from( ['a', 'b'] )->strrev( '-' );

You can even operate on all objects in a map at once:

// MyClass implements setId() (returning $this) and getCode() (initialized by constructor)
$map = Map::from( ['a' => new MyClass( 'x' ), 'b' => new MyClass( 'y' )] );
$map->setStatus( 1 )->getCode()->toArray();

The full documentation of the PHP Map package is available at https://php-map.org/