Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VarExporter][DoctrineBundle] Accessing unknown properties on lazy ghost objects generates notices instead exceptions #48970

Closed
kiler129 opened this issue Jan 13, 2023 · 7 comments

Comments

@kiler129
Copy link
Contributor

Symfony version(s) affected

6.2.4

Description

Initially this bug looked like an issue with EasyAdmin Bundle, as it is triggered by its code. The simplest way to do it is to add a field which doesn't exist on an entity, e.g.:

        yield TextField::new('type.thresholds_matches', 'Thresholds')
                       ->setVirtual(true)
                       ->setTemplatePath('admin/field/measurement_threshold.html.twig');

When enable_lazy_ghost_objects is disabled this works correctly and simply renders an empty field/custom template. However, the issue isn't actually in EAB but in LazyGhostTrait breaking the PropertyAccessorInterface promise:

* @throws Exception\InvalidArgumentException If the property path is invalid
* @throws Exception\AccessException If a property/index does not exist or is not public
* @throws Exception\UnexpectedTypeException If a value within the path is neither object
* nor array
*/
public function getValue(object|array $objectOrArray, string|PropertyPathInterface $propertyPath): mixed;

->getValue() should produce an exception when a given property doesn't exist, which EAB relies on: https://github.com/EasyCorp/EasyAdminBundle/blob/4db1860871809ca0be610ba5dab9be26ba165351/src/Field/Configurator/CommonPreConfigurator.php#L49-L53

How to reproduce

PoC reproducer, pick your own entity of course. In my case the type field is configured as:

    #[ORM\ManyToOne]
    #[ORM\JoinColumn(nullable: false)]
    private ?MeasurementType $type = null;

The test code I used:

<php declare(strict_types=1);

#[AsCommand(name: 'app:sandbox')]
class SandboxCommand extends Command
{
    public function __construct(private ManagerRegistry $doctrine, private PropertyAccessorInterface $propertyAccessor)
    {
        parent::__construct();
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $repo = $this->doctrine->getRepository(InanimateObjectMeasurement::class);
        $ent = $repo->findOneBy([]);

        dump($ent);
        dump($this->propertyAccessor->getValue($ent, 'value'));
        dump($this->propertyAccessor->getValue($ent, 'type.thresholds_matches'));
        
        return 0;
    }

With enable_lazy_ghost_objects=false:

 % bin/console app:sandbox -v
"183/88"

In PropertyAccessor.php line 453:
                                                                                                                               
  [Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException]                                                         
  Can't get a way to read the property "thresholds_matches" in class "Proxies\__CG__\App\Entity\Measurement\MeasurementType". 

With enable_lazy_ghost_objects=true:

 % bin/console app:sandbox -v
"183/88"

In LazyGhostTrait.php line 191:
                                                                                                                                                                                                                                       
  [ErrorException]                                                                                                                                                                                                                     
  User Notice: Undefined property: Proxies\__CG__\App\Entity\Measurement\MeasurementType::$thresholds_matches in /app/vendor/symfony/property-access/PropertyAccessor.php on line 428  
                                                                                                                                                                                                                                       

Possible Solution

No response

Additional Context

No response

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 13, 2023

LazyGhostTrait cannot break the contract of PropertyAccessorInterface because it is not bound by the contract.
The issue is either in PropertyAccessor being too confident in having a __get meaning the property is reachable, or in easy admin relying on it while it shouldn't (why does a virtual field get fetched on a property?)

Anyway, can you please provide a repository with the working reproducer? That'd help a lot. Thanks.

@nicolas-grekas
Copy link
Member

/cc @javiereguiluz WDYT of my previous comment about easy admin?

@nickygerritsen
Copy link

nickygerritsen commented Apr 1, 2023

We also run into this if you call $propertyAccessor->isReadable on a property that doesn't exist on a 'ghosted' Doctrine entity, you get this error. Not sure if it happens in general, but at least it happens here: https://github.com/DOMjudge/domjudge/blob/symfony-62/webapp/src/Controller/Jury/TeamController.php#L122 (note that on that branch lazy ghost objects are disabled since this would break our CI).

This is the backtrace for that error by the way:

ErrorException:
User Notice: Undefined property: Proxies\__CG__\App\Entity\Team::$num_contests in /domjudge/lib/vendor/symfony/property-access/PropertyAccessor.php on line 428

  at /domjudge/lib/vendor/symfony/var-exporter/LazyGhostTrait.php:193
  at Proxies\__CG__\App\Entity\Team->__get()
     (/domjudge/lib/vendor/symfony/property-access/PropertyAccessor.php:428)
  at Symfony\Component\PropertyAccess\PropertyAccessor->readProperty()
     (/domjudge/lib/vendor/symfony/property-access/PropertyAccessor.php:330)
  at Symfony\Component\PropertyAccess\PropertyAccessor->readPropertiesUntil()
     (/domjudge/lib/vendor/symfony/property-access/PropertyAccessor.php:227)
  at Symfony\Component\PropertyAccess\PropertyAccessor->isReadable()
     (src/Controller/Jury/TeamController.php:122)
  at App\Controller\Jury\TeamController->indexAction()
     (/domjudge/lib/vendor/symfony/http-kernel/HttpKernel.php:163)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()
     (/domjudge/lib/vendor/symfony/http-kernel/HttpKernel.php:74)
  at Symfony\Component\HttpKernel\HttpKernel->handle()
     (/domjudge/lib/vendor/symfony/http-kernel/Kernel.php:184)
  at Symfony\Component\HttpKernel\Kernel->handle()
     (/domjudge/lib/vendor/symfony/runtime/Runner/Symfony/HttpKernelRunner.php:35)
  at Symfony\Component\Runtime\Runner\Symfony\HttpKernelRunner->run()
     (/domjudge/lib/vendor/autoload_runtime.php:30)
  at require_once('/domjudge/lib/vendor/autoload_runtime.php')
     (public/index.php:5)  

@delboy1978uk
Copy link
Contributor

We are upgrading a site to Symfony 6.3, we were using reflection in one of our classes to access the Connection class of an EntityManager, but now that fails because we do not have the real object and have this uninitialised lazy ghost instead. Any ideas?

@nicolas-grekas
Copy link
Member

@kiler129 could you please wrap your reproducer into a repository to help me bootstrap this quickly?

@nickygerritsen
Copy link

I have a very minimal repo to reproduce this for my case. It comes down to:

        $p    = PropertyAccess::createPropertyAccessor();
        $item = new \Proxies\__CG__\App\Entity\Item();
        $item->setName('test');
        dump($item);
        dump($p->isReadable($item, 'name')); // <-- Works
        dump($p->isReadable($item, 'missing')); // <-- Gives a notice

Repo can be found here.

Switching lazy ghost objects off makes this code work without a notice.

Let me know if you need anything else from me. My guess is this issue also resolves the original one.

@nicolas-grekas
Copy link
Member

Should be fixed by #54194

nicolas-grekas added a commit that referenced this issue Mar 13, 2024
…las-grekas)

This PR was merged into the 6.4 branch.

Discussion
----------

[PropertyAccess] Fix checking for missing properties

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #48970, fix #54194, fix #51919
| License       | MIT

PropertyAccess and PropertyInfo make a few assumptions when magic methods are involved.
Namely they assume that if a `__get` method is defined, then any property name is accessible.
This assumption is generally wrong, and this becomes more evident with the introduction of lazy objects.
The linked issue gives some more details.

In this PR, I tweak these assumptions in order to make less of them.
Note that there is no bullet-proof way to decide if a *virtual* property exists in PHP. We're missing an `__exists()` magic method for that. Because of this, I'm submitting this PR to 6.4 and not 5.4. Let 5.4 end its life quietly and ensure 6.4 works at its best with lazy ghosts, where they're mainstream.

Commits
-------

9610a7c [PropertyAccess] Fix checking for missing properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants