add AbstractCollection abstract class

[drupol]

This PR has been created while discussing with @AlexandruGG

This is an alternate way to provide an abstract collection class:

• without depending on it
• without altering what's already existing
• without repeating code all over the place.

Minimal Working Example:

class AAA extends AbstractCollection
{
}

$c = AAA::unfold(static fn (int $a = 0, int $b = 1): array => [$b, $a + $b]);
print_r($c->limit(10)->all());

This PR:

• Provide an alternate way to have an abstract class in the library
• Provide ...
• It breaks backward compatibility
• Is covered by unit tests
• Has static analysis tests (psalm, phpstan)
• Has documentation
• Is an experimental thing

Related to #268

[drupol]

@BusterNeece What do you think of this option ?

[BusterNeece]

@drupol I don't mind it, but I think you may have combined two of the methodologies I was talking about in a way that isn't particularly necessary.

Right now, this PR has both an AbstractCollection and wraps an internal collection that's tracked separately. Only one of those things needs to happen, not necessarily both.

Since I'd imagine the static analysis tool will have a fit over the current setup, what I'd propose would be something else:

• Revert the main Collection class to how it was, with no abstraction; this would be the main class to use if you aren't modifying anything, and it can stay final
• Make a new, extensible CollectionDecorator class that isn't final, that has this same "wrappedCollection" variable syntax. Almost exactly the way the current abstract is implemented.

This is how Doctrine does it, and it works extremely well for both ensuring the main classes are final, while giving developers an easy branching-off-of point for their own implementations: https://github.com/doctrine/orm/blob/2.13.x/lib/Doctrine/ORM/Decorator/EntityManagerDecorator.php

It also forces that anyone implementing their own won't reference your Collection class directly in their code, but rather will use the CollectionInterface instead.

[BusterNeece]

@drupol Also, upon further investigation, I'm not sure that the wrappable class should return instances of the interface, but rather something like static. Otherwise, once you do a single operation in your extended class, the returned item is hinted as being a different type than the original. I believe as long as the abstract class extends the interface, this is still considered a subset of the interface's return type.

[drupol]

@drupol Also, upon further investigation, I'm not sure that the wrappable class should return instances of the interface, but rather something like static. Otherwise, once you do a single operation in your extended class, the returned item is hinted as being a different type than the original. I believe as long as the abstract class extends the interface, this is still considered a subset of the interface's return type.

Definitely, but AFAIK returning static is not yet doable in PHP :(

[BusterNeece]

@drupol static as a return type hint was introduced with PHP 8.

[drupol]

Right, then it would be only available for PHP 8 :)

I'll do the change later in the evening I think.

[drupol]

I updated the PR, now the Minimal Working Example is:

class AAA extends CollectionDecorator
{
}

$decorated = Collection::unfold(static fn (int $a = 0, int $b = 1): array => [$b, $a + $b]);

$c = (new AAA($decorated))->limit(10);

print_r($c->all());

[BusterNeece]

@drupol For immutability's sake, you should probably return new static($this->wrapped->doThing()) all of those operations, right? So it matches the expected pattern of the current class.

[drupol]

Oooh you're right, going to make the changes.

[BusterNeece]

@drupol Come to think of it, that would prevent anyone from injecting their own constructors into the extended class...may be worthwhile to make a newInstance function that does the return new static($thing), so you can replace it with whatever other stuff you want to pass in the constructor. Just a thought.

[AlexandruGG]

I like this approach better than the one in #268!

That being said, I'm still not sure this is actually needed, for the following reason: if anyone wants to add new, useful operations to Collection, then these would be best placed directly in the library and the main class, rather than a custom one. That way everyone benefits from it.

If the argument is that "well, this is a very specific business logic operation and doesn't fit into the library itself", then I would say that it shouldn't be in Collection at all (including an extension of it).

Also this feels like peak irony: https://github.com/loophp/collection/pull/270/files#diff-d82dd67de25323d8122521b137f07152b743665fe3e8b718f99136cc90c88fa3R24. Feels wrong to introduce something new as "deprecated" and tell people not to use it.

[drupol]

I like this approach too, but there's a couple of things to discuss:

1. How to deal with static constructors? I removed them from the Collection class
2. Should we add a constructor?
3. What should be the visibility of the default Collection constructor?
4. Fix the hundreds of new static analysis issues

[drupol]

@AlexandruGG I think it's definitely ready. WDYT?

[AlexandruGG]

Nice work!

[BusterNeece]

@drupol Thanks for your persistence in working on this. Glad to see this kind of extensibility in the library :)

[drupol]

Thanks you guys ! :) Hope this is going to help some people !