Solve the mess with array and ArrayCollection in Model/Entities.

[VincentLanglet]

There is multiple method which are typehinted with Foo[] but returns a Collection.

Issue 1: BlockInterface::children https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/BaseBlock.php#L33 and we can find a method to disable the lazy loading here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Model/Block.php#L55 which are called here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/BlockInteractor.php#L150-L155

But the getChildren() method is coming from BlockBundle, which is typehinted to array in 4.x https://github.com/sonata-project/SonataBlockBundle/blob/4.x/src/Model/BlockInterface.php#L144

Issue 2: PageInterface::children There is the same issue https://github.com/sonata-project/SonataPageBundle/search?q=ArrayCollection And I found a call to array_unshift on $page->getParents() in PageExtension.

Issue 3: Transformer::children There is also something wrong with the Transformer: We set an arrayCollection here as a value https://github.com/sonata-project/SonataPageBundle/blob/9343568991c81a787e41dd3a02079227b7d2d7d9/src/Entity/Transformer.php#L232 but the property is considered as an array of BlockInterface

Issue 4: SnapshotChildrenCollection implementation This class is doing multiple calls like $this->collection->offsetUnset when collection is an array. Which doesn't work. It seem like the collection property was meant to be a Collection, but it does

$this->collection = $this->transformer->getChildren($this->page);

when TransformerInterface::getChildren() returns an array according to the phpdoc

Issue 5: SnapshotChildrenCollection usage We're passing this class to Page::setChildren https://github.com/sonata-project/SonataPageBundle/blob/9343568991c81a787e41dd3a02079227b7d2d7d9/src/Model/SnapshotPageProxy.php#L109-L115 which is supposed to be an array (or Collection cf issue 2). But it's not.

Do you use those code @eerison ?

This issue will be a blocker for the 4.0 version.

[eerison]

Hey @VincentLanglet

Issue 2: PageInterface::children There is the same issue

No, what I use from PageInterface is getId() only this

Issue 3: Transformer::children There is also something wrong with the Transformer: We set an arrayCollection here as a value

I used this transform but indirectly here, I didn't touch in that code

I jus got the same code from Consumers

[Hanmac]

imo for my projects i let it return Collection but typehint it with Type[]|Collection

but i don't know what is your best idea

[VincentLanglet]

For Blocks, using Collection is not a bad idea, but

• I don't like have the need for a disableAutoloading method https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Model/Block.php#L55 which are not in the Interface. Dunno if we can avoid it.
• The Interface from BlockBundle ask for an array, so we should return this (or update the interface to iterable and restrict to Collection in this bundle).

For Pages, using Collection seems nice also, but

• I don't like have the need for a disableAutoloading method again. Dunno if we can avoid it.
• array_unshift call should be changed

For Transformer I assume there is a bug somewhere to fix since the call

$this->children[$page->getId()] = new ArrayCollection($pages); 

is not consistent with the others values set.

For SnapshotChildrenCollection, I think we need to fix both the implementation and the usage.

[eerison]

@VincentLanglet I was wondering if this SonataPageBundle/src/Model/SnapshotPageProxy.php is really necessary, do you know what is the advantage to use this proxy?

I guess the main idea is for who wants to implement something outside of SonataPage and wants intercept the SnapshotPage 😕 , But I don't know if it's used, there ins't anything into the doc explain how to use this.

and this proxy is really huge

[VincentLanglet]

@VincentLanglet I was wondering if this SonataPageBundle/src/Model/SnapshotPageProxy.php is really necessary, do you know what is the advantage to use this proxy?

I guess the main idea is for who wants to implement something outside of SonataPage and wants intercept the SnapshotPage 😕 , But I don't know if it's used, there ins't anything into the doc explain how to use this.

and this proxy is really huge

I have absolutely no idea since I dont use this bundle.

Do you want to try a POC of all the code which could be removed ?

[eerison]

Do you want to try a POC of all the code which could be removed ?

what is POC? 😄

anyway let me try to undestand the issues that you found, maybe it can be a start.

There is multiple method which are typehinted with Foo[] but returns a Collection.

I didn't find Foo[], where do is it? into any tests?

Issue 1: BlockInterface::children https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/BaseBlock.php#L33 and we can find a method to disable the lazy loading here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Model/Block.php#L55 which are called here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/BlockInteractor.php#L150-L155

do you know why do wee need this lazy loading? was it used in other bundle or just here?

Edit 1: and why is it a blocker for 4.0 release? just because it's a mess or it's block something to support symfony 5 and sonata 4?

[VincentLanglet]

Do you want to try a POC of all the code which could be removed ?

what is POC? 😄

Proof of Concept. You can open a PR on 4.x with all the code this would allow to remove (even without fixing tests) (and try this branch on your project). This would be easier to discuss about it ; and if we decide that we can remove this code, it just require to fix the tests and do a PR to deprecate this code on 3.x.

anyway let me try to undestand the issues that you found, maybe it can be a start.

There is multiple method which are typehinted with Foo[] but returns a Collection.

I didn't find Foo[], where do is it? into any tests?

I wrote Foo for "any", it's sometimes BlockInterface, sometimes PageBlockInterface, ...

For instance there is already one discussion here: https://github.com/sonata-project/SonataPageBundle/pull/1469#discussion_r923635402

Issue 1: BlockInterface::children https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/BaseBlock.php#L33 and we can find a method to disable the lazy loading here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Model/Block.php#L55 which are called here: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Entity/BlockInteractor.php#L150-L155

do you know why do wee need this lazy loading? is it used in other bundle or just here?

The lazy load was already removed by @jordisala1991 in one of his PHPStan PR. And I think he has/will solve of the issue I put in this post when he has/will bump PHPStan level. So I would recommend to take another issue

Edit 1: and why is it a blocker for 4.0 release? just because it's a mess or it's block something to support symfony 5 and sonata 4?

Because we're bumping phpstan and we'll add typehint.

[eerison]

Because we're bumping phpstan and we'll add typehint.

Yeah I imagined that would be a problem to bump phpstan, I saw this here #1460

[eerison]

Hey @VincentLanglet is it missing something in this issue?

[VincentLanglet]

I will close this since it will be solved when bumping the phpstan levels anyway and I already done https://github.com/sonata-project/SonataBlockBundle/pull/1087 (i just need a release)