Project

General

Profile

Actions

Bug #92357

closed

Extbase / Symfony: Domain Object Validation with Lazy Loading ignores Getters

Added by Till Wimmer over 3 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Extbase
Target version:
-
Start date:
2020-09-21
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
10
PHP Version:
7.4
Tags:
extbase symfony validation lazyloading
Complexity:
Is Regression:
Sprint Focus:

Description

There is an issue with model validation in V10 when lazy loading is involved.

In such a case we get the PHP error:

Cannot access protected property Myvend\Myext\Domain\Model\Myclass::$property

I.e. it tries to access the (protected) property instead of using the getter.

The reason is that Symfony analyzes the LazyLoadingProxy object instead of the real class and then gets the ACCESS_TYPE_PROPERTY (/var/www/typo3_src-10.4.8/vendor/symfony/property-access/PropertyAccessor.php line 398):

                    throw $e;
                }
            } elseif (self::ACCESS_TYPE_PROPERTY === $access[self::ACCESS_TYPE]) {
                $result[self::VALUE] = $object->{$access[self::ACCESS_NAME]};

                if ($access[self::ACCESS_REF] && isset($zval[self::REF])) {
                    $result[self::REF] = &$object->{$access[self::ACCESS_NAME]};
                }

This happens in Symfony\Component\PropertyAccess::readProperty() by calling getReadAccessInfo().

My workaround is adding those lines starting at 377, readProperty():

    private function readProperty(array $zval, string $property, bool $ignoreInvalidProperty = false): array
    {
        if (!\is_object($zval[self::VALUE])) {
            throw new NoSuchPropertyException(sprintf('Cannot read property "%s" from an array. Maybe you intended to write the property path as "[%1$s]" instead.', $property));
        }

377:    if ($zval[self::VALUE] instanceof \TYPO3\CMS\Extbase\Persistence\Generic\LazyLoadingProxy) {
378:        $zval[self::VALUE] = $zval[self::VALUE]->_loadRealInstance();
379:    }
Actions #1

Updated by Till Wimmer over 3 years ago

  • Subject changed from Extbase / Symfony: Domain Object Validation with Lazy Lodaing ignores Getters to Extbase / Symfony: Domain Object Validation with Lazy Loading ignores Getters
Actions #2

Updated by Till Wimmer over 3 years ago

Two hours later:

The fix should rather go to /extbase/Classes/Validation/Validator/GenericObjectValidator.php:

    protected function getPropertyValue($object, $propertyName)
    {
        // @todo add support for lazy loading proxies, if needed
        if (ObjectAccess::isPropertyGettable($object, $propertyName)) {
            return ObjectAccess::getProperty($object, $propertyName);
        }
        throw new \RuntimeException(
            sprintf(
                'Could not get value of property "%s::%s", make sure the property is either public or has a getter get%3$s(), a hasser has%3$s() or an isser is%3$s().',
                get_class($object),
                $propertyName,
                ucfirst($propertyName)
            ),
            1546632293
        );
    }
Actions #3

Updated by Simon Schaufelberger over 3 years ago

I'm having the exact same bug and I'm totally stuck as I have a lazy dependency to static_countries and extbase tries to validate $countryZones in the Country model which will crash because of this.

How to reproduce:

Create a model A with a lazy dependency $country to static_countries and validate model A.

Workaround: make all affected properties public.

Actions #4

Updated by Xavier Perseguers almost 3 years ago

  • Status changed from New to Accepted

Confirmed.

Actions #5

Updated by Johannes Rebhan over 2 years ago

Same problem. Workaround to make the property in question public worked. But going by the call trace this will hurt performance as Symfony triggers _loadRealIstance for single reference lazy loaded properties. A fix would be greatly appreciated.

Actions #6

Updated by Stefan Neufeind about 2 years ago

I experienced a case which might be related here. Had a list of objects "a" and using fluid I wanted to access a property nested some hierarchies deeper (here: d). So I simply tried to output the variable {a.b.c.d}. That resulted in "null" since c wasn't yet loaded (marked as Lazy in the relation b to c). First doing a debug-output for {a.b.c} triggered the loading and then d was accessible as expected.

Discussed this with Mathias Brodala who agreed that "LazyLoadingProxy" might be part of the issue here. He mentioned probably dropping LazyLoadingProxy and "[...] ATM this could only be implemented using generated proxy classes like News or Doctrine ORM does".

Actions #7

Updated by Johannes Seipelt about 2 years ago

Stumbled upon the same error but in my case setting a default value (null) for the property fixed the issue for me (v10, php 7.4).

Actions #8

Updated by Alexander Schnitzler about 2 years ago

I stumbled upon this as well and I wonder how that ever worked in the past but I have the feeling that Extbase simply opened the property via reflection which symfony/property-access does not do.

I would propose the following quick fix:

// LazyLoadingProxy.php

public function __get($propertyName)
{
    $realInstance = $this->_loadRealInstance();

    if ($realInstance instanceof AbstractDomainObject) {
        return $realInstance->_getProperty($propertyName);
    }
    return $realInstance->{$propertyName};
}

It's not a real solution but it prevents errors that didn't occur in the past.

Actions #9

Updated by Torben Hansen about 2 years ago

Verified that the patch Alex suggested works and I think this is a better solution than making properties public.

I "solved" the issue locally by XClassing \TYPO3\CMS\Extbase\Persistence\Generic\LazyLoadingProxy and overwriting __get() as Alex suggested.

Actions #10

Updated by Till Wimmer about 2 years ago

Torben Hansen wrote in #note-9:

Verified that the patch Alex suggested works and I think this is a better solution than making properties public.

I "solved" the issue locally by XClassing \TYPO3\CMS\Extbase\Persistence\Generic\LazyLoadingProxy and overwriting __get() as Alex suggested.

Not sure why my initial approach to fix it was never accepted....

Actions #11

Updated by Simon Schaufelberger about 2 years ago

@Till: Maybe because you never created an actual code change in gerrit? Just by commenting here, the code does not find its way into the core :D

Actions #12

Updated by Gerrit Code Review about 2 years ago

  • Status changed from Accepted to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74072

Actions #13

Updated by Gerrit Code Review about 2 years ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74072

Actions #14

Updated by Simon Schaufelberger about 2 years ago

Quick poll: should this fix be backported for v10 or only for v11? There are fears of breaking something in v10 installations where people come up with other solutions which are incompatible with Alex's solution

Actions #15

Updated by Simon Schaufelberger about 2 years ago

  • % Done changed from 0 to 100
Actions #16

Updated by Gerrit Code Review about 2 years ago

Patch set 1 for branch 11.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74117

Actions #17

Updated by Anonymous about 2 years ago

  • Status changed from Under Review to Resolved
Actions #18

Updated by Stefan Neufeind about 2 years ago

@Simon: I'd be in favor of having it backported. For us the patch in some cases was to trigger loading the real instance first thus working around lazyloaded objects.

Actions #19

Updated by Christian Kuhn about 2 years ago

We decided to not backport to v10 anymore - v10 instances that really need this probably established a workaround already or could pick that patch on their own.

Actions #20

Updated by Till Wimmer about 2 years ago

PLEASE backport it! I finally want to pull upgrades... I reported this issue 1.5 years ago...

Actions #21

Updated by Simon Schaufelberger about 2 years ago

I tried my best to convince Christian, but he was too scared to back port it to v10 as some people might have come up with other solutions, and they might then get results with the fix which are breaking. So the best solution for v10 is to Xclass the class or make the property public or apply the patch via composer.

Actions #22

Updated by Till Wimmer about 2 years ago

I can't understand this strategy. If they have their own fix, then they have to patch the code each time they get an update. How is this more convenient than dropping their own solution?

And if there are many people who had an issue with this, then why only a few reported it? I reported the issue and hold back all our clients updates for 18 months, for nothing... Strange to care more for the few people who might have an issue than to care for people who obviously have an issue and reported it.

By the way: I wonder how this fix could negatively interfere with other solutions...

Actions #23

Updated by Christian Kuhn about 2 years ago

Hey Till.

I understand your frustration this has not been fixed anymore in v10.

The current state of v10 is 'old-stable': Only security fixes and critical issues like browser crashes are still fixed in v10.

As a maintainer, my main goal is to absolutely not break anything in v10 anymore - a regression at this point in v10 lifetime would be really ugly.

This bug is for most people not considered critical. Instances with this issue probably worked around it already, and it's not a bug that we introduced recently - it is not a patch level regression. The patch itself is just a couple of lines, so instances that really need it, could apply the patch on their own and test if everything is still fine. I assume that the majority of instances is not affected by the bug in the first place. But the patch is in a low-level area of the extbase framework and it's really hard to be 100% sure it can not break something. This patch is far away from a no-brainer, and as such risky.
I picked up the patch and took responsibility by merging it. Considering the above points I don't feel good enough to take the additional responsibility for a v10 merge. Maybe some other core maintainer is more relaxed and can be convinced to additionally backport this to v10, I'm not.

I'm aware users, devs and maintainers have different views on this topic. It's not always easy to balance these things and I hope you understand my decision, although you may disagree in this case.

Actions #24

Updated by Benni Mack over 1 year ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF