search

doctrine / DoctrineORMModule

Doctrine ORM Module for Laminas
https://www.doctrine-project.org/projects/doctrine-orm-module.html
MIT License
439 stars 229 forks source link

Property generateProxies must be integer #665

Closed pbs-dg closed 3 years ago

pbs-dg commented 3 years ago

GenerateProxies can have different autogenerate levels as defined in Doctrine\Common\Proxy so it must be an integer.

TomHAnderson commented 3 years ago

I've found a bug that has roots going back 10 years. https://github.com/doctrine/DoctrineORMModule/blame/d71f07c62df65b8f03b086a8abf0a2493341284b/config/module.config.php#L46 This is when the first commit was made to the module configuration regarding proxy generation. Note the value was a boolean. https://github.com/doctrine/DoctrineORMModule/blame/4.0.x/config/module.config.php#L64 9 years ago the config was changed to add generate_proxies as a configuration option. And the value has never changed since. https://github.com/doctrine/common/blame/ae862a3e8c18cd3a7a527b39bd99b36d1f7c8d02/lib/Doctrine/Common/Proxy/AbstractProxyFactory.php#L32 8 years ago the AbstractProxyFactory introduced constants for integer generate_proxy values. However, as noted, DoctrineORMModule was never changed. So, the correct fix for this is to use the constants from Doctrine\ORM\Proxy\ProxyFactory (extended from AbstractProxyFactory) for all the configuration and to change the Options classes to expect an int instead of a bool. I'm fine with that. But... This is a change that will affect everyone who has ever configured their ORM through DoctrineORMModule. It is my opinion that the module should receive a major version change and go ahead with the correction. I would appreciate a second opinion. (edited) 7 replies

alcaeus 7 days ago Nice archeology there! Just a heads up, you should leave the option as a bool: ORM is eventually going to use ProxyManager to generate proxies, which again only understands two options, not everything else

Tom H Anderson 7 days ago Keep it! Not the answer I was expecting at all, but it sure simplifies things. I'll need to quote you in the commit :dart:

alcaeus 7 days ago Haha :smile:

Christophe Coevoet 6 days ago well, the question is what true and false mean in the new system in ORM 3.0 (which is not there yet)

Christophe Coevoet 6 days ago compared to the existing values

Christophe Coevoet 6 days ago in 2.x, the "best" solutions for dev and prod might not be the one mapped for true and false (otherwise, new behaviors would not have been introduced in minor versions)

Tom H Anderson 6 days ago Thanks for the feedback, Christophe. Fortunately, the values for 0&1, false & true, map well to the ProxyFactory as-is. I'll take Alcaeus' advice and continue with the usage as-is and 'fix' the unit test which uses the integer constant.

blaineam commented 3 years ago

I'm having an issue with this same bug, however I really could use the ability to use one of the other constants in the AbstractProxy class like 2 or 3 to solve a read write race condition on macos docker when using a Docker ORM laminas module. We use a the doctrine option generate_proxies for that module that passes it on to doctrine orm module. Because it is casted to a bool here I don't think setting a 2 or 3 makes it past this section as those basically are the same as a 1 or true. Is there any possibility of an update to this module?

TomHAnderson commented 3 years ago

The bug addressed here was in the unit tests for this repository. Try assigning one of the integer constants in your config and there's a chance it'll work.

There is no change of an update to this module.

blaineam commented 3 years ago

If I recreate this code change the option passes through to the AbstractProxy class as I expected however if I stick to using main it does not work 3 turns into a 1 because it is casting a 3 to a bool and 3 is truthy. I will also add that it appears to solve the proxy generation race condition on macos as I suspected. were using an older than 4.x build anyways so I will look into other options.