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

Public readonly properties are unset in Proxy's constructor #9431

Closed
wants to merge 1 commit into from
Closed

Public readonly properties are unset in Proxy's constructor #9431

wants to merge 1 commit into from

Conversation

solcik
Copy link
Sponsor

@solcik solcik commented Jan 27, 2022

Failing Test

Q A
BC Break no
Version 2.11.0

Summary

Public readonly properties are unset in Proxy's constructor, which results in error.

Current behavior

Cannot unset readonly property \Entity\SimpleBook::$title from scope Proxies\__CG__\Entity\SimpleBook

How to reproduce

#[Entity, Table]
class SimpleBook
{
    #[Column, Id, GeneratedValue]
    private readonly int $id;

    #[Column]
    public readonly string $title;

    #[ManyToOne, JoinColumn(nullable: false)]
    private readonly Author $author;

    public function getId(): int
    {
        return $this->id;
    }

    public function getTitle(): string
    {
        return $this->title;
    }

    public function getAuthor(): Author
    {
        return $this->author;
    }
}

class SimpleBook extends \Entity\SimpleBook implements \Doctrine\ORM\Proxy\Proxy
{
    // ...

    public function __construct(?\Closure $initializer = null, ?\Closure $cloner = null)
    {
        unset($this->title);

        $this->__initializer__ = $initializer;
        $this->__cloner__      = $cloner;
    }
}

Expected behavior

Code that generates Proxy's constructor

I dug into ProxyGenerator and here is generation of these unsets:

public readonly properties are not tested as I see in tests.

@greg0ire
Copy link
Member

The failing test does not seem to fail…

@solcik
Copy link
Sponsor Author

solcik commented Jan 27, 2022

Yeah, I saw, and did investigate it now on my local. Sorry, after investigation the problem is with public readonly properties, not nullable ones as I thought. Hopefully updated test case is going to show that. I also updated the description.

@solcik solcik changed the title Nullable readonly properties unset in constructor Public readonly properties unset in Proxy's constructor Jan 27, 2022
@greg0ire
Copy link
Member

Same player try again :P

@solcik
Copy link
Sponsor Author

solcik commented Jan 27, 2022

Sorry for the test not failing./ It is my first attempt for such a large project.) I am doing it blindly, based on Proxies generated in my local project. I do not have local setup to run tests. I am going to do that, but just now leaving for vacation. So you can expect it around 2022-02-08

I put more info into the description. Each public property is unset in Proxy's constructor.

I dig into ProxyGenerator and here is generation of these unsets:

public readonly properties are not tested as I see...

I am going to do failing tests after vacation.

@solcik solcik changed the title Public readonly properties unset in Proxy's constructor Public readonly properties are unset in Proxy's constructor Jan 27, 2022
@beberlei
Copy link
Member

pretty sure the problem is that SimpleBook is not used as a proxy in the test code. You can modify the tests to use $book = $entityManager->getReference(SimpleBook, $id); and work on the proxy then it should trigger this error.

@edipoReboucas
Copy link

edipoReboucas commented Feb 23, 2022

Aparrently the issue is on \Doctrine\Common\Proxy\ProxyGenerator::getLazyLoadedPublicPropertiesNames.

The strategy of unset the property to made it lazyloadable will not work on public readonly property and should be ignored.

@solcik
Copy link
Sponsor Author

solcik commented Apr 5, 2022

@greg0ire sorry for the delay, but eventually I got back to it.

@beberlei Thank you for the hint.

I added the test-case, which is failing now locally - hopefully it is going to fail also here .)

@christian-kolb
Copy link

@greg0ire I'm using only public in entities and this is preventing me from using it entirely. Is there something specific I can do to support you on this ticket? If it's with some kind of implementation, can you point me to the relevant point in the codebase and documentation for the context? 🙂
Thanks a lot in any case.

@greg0ire
Copy link
Member

@christian-kolb I believe there is something you could do that would help: everyone here is pointing to doctrine/common as a culprit, yet this PR is on doctrine/orm. Maybe you could find a way to create another failing test that reproduces the bug solely with the doctrine/common APIs, and contribute it in a PR to doctrine/common? Something that would also help would be to find out why these calls to unset exist in the first place? What purpose do they serve?

@beberlei
Copy link
Member

I believe public readonly properties cannot be supported for entities that are used as proxy. At least i dont see a way we can implement that in the engine, because it requires unset and __get

@christian-kolb
Copy link

@greg0ire That is what's happening here, isn't it? doctrine/common#957
@beberlei Would that be an option?

Perhaps it makes sense to close this pull request here. At the moment there are 2 issues and 2 pull requests over both packages. I haven't even seen the other pull request until I dived into it further 🙂

@beberlei
Copy link
Member

It makes the error go away but breaks lazy loading, so it's not an option

@christian-kolb
Copy link

@beberlei Too bad. So this means you don't think Doctrine will ever be able to offer public readonly properties?
I think this is something that should be added to this article https://www.doctrine-project.org/2022/01/11/orm-2.11.html. The example is done with private properties (I don't know if that was on purpose or just out of habit). But I wouldn't have guessed that it's only possible with private properties.

@beberlei
Copy link
Member

It depends if we find a way in the engine to hack around this

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants