Bug #88464

LazyLoadingProxy does not resolve into \SplObjectStorage, resulting in infinite loop (same for `@var array`)

Added by Leonie Philine Bitto 5 months ago. Updated 4 months ago.

Status:
Accepted
Priority:
Should have
Assignee:
-
Category:
Extbase
Start date:
2019-05-30
Due date:
% Done:

0%

TYPO3 Version:
9
PHP Version:
7.2
Tags:
Complexity:
easy
Is Regression:
Sprint Focus:

Description

This report is about \TYPO3\CMS\Extbase\Persistence\Generic\Mapper\DataMapper::mapResultToPropertyValue() and the magic (__call() etc) and \Iterator interface methods of \TYPO3\CMS\Extbase\Persistence\Generic\LazyLoadingProxy:

Preface

\SplObjectStorage used to be buggy, which is why the Extbase ObjectStorage was created. In currently supported PHP versions \SplObjectStorage is no longer buggy and it would be time to shed the non-standard Extbase ObjectStorage.

The Problem

Code indicates that \SplObjectStorage is supported. However, LazyLoadingProxy fails to map fetched related objects onto an \SplObjectStorage:

it checks the following:

if (in_array($propertyMetaData['type'], ['array', 'ArrayObject', 'SplObjectStorage', Persistence\ObjectStorage::class], true)) {
    ...
}

(Note that array is supported as well, which will lead to the same infinite loop when iterating through the results!)

The method fetches the objects into an array $objects[]. For a target @var type ArrayObject or Extbase Persistence ObjectStorage, the objects array is boxed into the respective container.
There is no handling for \SplObjectStorage though, which causes a plain array to be set as the $parentObject's resolved formerly-lazy property.

When iterating through the \Iterator, e.g. with a for(... as ...) loop, the LazyLoadingProxy calls \TYPO3\CMS\Extbase\Persistence\Generic\LazyLoadingProxy::_loadRealInstance(), to then call current(), next() or reset() on the result.

This works well for object containers, but not for arrays, because PHP returns arrays by value, not by reference .

On each call to $this->_loadRealInstance(), a new copy of the fetched related objects is made, stored in $realInstance and current(), next() or reset() called on the copy. These calls do not at all influence the actual real instance.

Solution steps:

1) Reference assignments

In the LazyLoadingProxy magic methods (__call() etc) and \Iterator interface methods, use reference ("&") assignments:

$realInstance = &$this->_loadRealInstance();

This step will fix the redirect loop for \@var array, but we still need to fulfill the @var \SplObjectStorage<Vendor\Ext\Domain\Model\Something> annotation

2) Handling \SplObjectStorage property mapping targets

In \TYPO3\CMS\Extbase\Persistence\Generic\Mapper\DataMapper::mapResultToPropertyValue(), implement a section for \SplObjectStorage:

                } elseif ($propertyMetaData['type'] === \SplObjectStorage::class) {
                    $propertyValue = new \SplObjectStorage();
                    foreach ($objects as $object) {
                        $propertyValue->attach($object);
                    }
                    $propertyValue->_memorizeCleanState();
                }

This section should probably share code - except for the constructor - with the Extbase Persistence ObjectStorage section.

Note:
Long ago it was tried to base ObjectStorage on SplObjectStorage (https://forge.typo3.org/issues/63989), but that was abandoned: https://review.typo3.org/c/Packages/TYPO3.CMS/+/35585

I believe it is time to decide:

1)
Drop ObjectStorage and switch Extbase to the native SplObjectStorage

2)
Or: Drop support for SplObjectStorage and only support ObjectStorage (then what about ArrayObject and array?)

3)
Or: Properly support SplObjectStorage and array like ObjectStorage and ArrayObject are supported. (solution indicated above)

History

#1 Updated by Leonie Philine Bitto 5 months ago

Where I wrote "redirect loop", I meant "infinite loop". =)

#2 Updated by Alexander Schnitzler 5 months ago

I believe it is time to decide:

1)
Drop ObjectStorage and switch Extbase to the native SplObjectStorage

2)
Or: Drop support for SplObjectStorage and only support ObjectStorage (then what about ArrayObject and array?)

3)
Or: Properly support SplObjectStorage and array like ObjectStorage and ArrayObject are supported. (solution indicated above)

Well, yes. It's time to decide at a certain point but that time has not come yet.
We already have some patches in 10 that help with the decoupling of the ObjectStorage object of Extbase but there is also the question of how the persistence will look like in general and there is no decision from the persistence team yet.

I would be glad to allow/enable all kind of collections here but this topic is not that easy to cope with.

#3 Updated by Alexander Schnitzler 5 months ago

  • Target version changed from next-patchlevel to Candidate for Major Version
  • TYPO3 Version changed from 9 to 10

#4 Updated by Alexander Schnitzler 5 months ago

Just a short additional note:
TYPO3 9 should only receive bugfix and security patches, therefore such a big change can't make it into that version.

Extbase only supports its own ObjectStorage and therefore a change in this area might look like a bugfix but is more a feature.

#5 Updated by Georg Ringer 5 months ago

  • Status changed from New to Accepted

#6 Updated by Leonie Philine Bitto 4 months ago

  • Target version changed from Candidate for Major Version to Candidate for patchlevel
  • TYPO3 Version changed from 10 to 9

The problem is mainly a bug in the current software.

See "Solution steps".

You wrote: "Extbase only supports its own ObjectStorage"

This is not true - Extbase supports 'array', 'ArrayObject', 'SplObjectStorage', Persistence\ObjectStorage::class, but most of them are buggy and must be fixed.

The fix is not difficult. Just return references and we are good to go.

You wrote: "Well, yes. It's time to decide at a certain point but that time has not come yet."

The "I believe it is time to decide" part is just a side note for a broader perspective, but it is not THE bug report. I think you misread that.

It's not necessary to decide that to just fix the existing bugs as I proposed in the OP under "Solution Steps".

Since this is an easy complexity bugfix, it is indeed a Candidate for patchlevel.

I already provided code above.

If you disagree, then in fact LazyLoadingProxy is buggy by offering (broken) mapping to 'array', 'ArrayObject', 'SplObjectStorage'.
This must be removed and documentation must be updated to reflect your claim "Extbase only supports its own ObjectStorage".

Also available in: Atom PDF