search

laravel-doctrine / orm

An integration library for Laravel and Doctrine ORM
MIT License
829 stars 177 forks source link

[BUG] Registry returns manager for unsupported classes if it has one manager #578

Closed Hermaniandr closed 3 weeks ago

Hermaniandr commented 3 months ago

If there is only one manager, there is no check that it supports entity - it was removed in this PR https://github.com/laravel-doctrine/orm/pull/288/files As a result, if method called with some random class (e.g. \DateTime) - manager will be returned, even though it doesn't support this class.

Package version, Laravel version

laravel-doctrine/orm: 2.1.0 laravel/framework: 11.9.2

Expected behaviour

LaravelDoctrine\ORM\IlluminateRegistry::getManagerForClass returns null for non-Entities if it has one manager.

Actual behaviour

LaravelDoctrine\ORM\IlluminateRegistry::getManagerForClass returns manager for non-entities if it has one manager.

Steps to reproduce the behaviour

Registry::getManagerForClass(\DateTime::class)

TomHAnderson commented 1 month ago

Why should it return null? Doesn't an exception make more sense than checking the return value every time.

TomHAnderson commented 1 month ago

I've addressed this so an exception is returned if class is not found in any entity manager.

https://github.com/laravel-doctrine/orm/pull/628/commits/ac404a93a60cf3b92ca80ae6565a3711228094fd#diff-3fd23ed6f1b7b5448c5407dfea9909c80d6f42dcdfb44fc8dc2f473a55365848R338

Hermaniandr commented 3 weeks ago

@TomHAnderson Doctrine doesn't throw exception in case manager not found - it just returns null, and some packages rely on this behavior. As an example - https://github.com/zenstruck/foundry, when it goes through Entity properties, it tries to find managed if property is object - if manager found, it treats property as Entity (persist & some other actions), otherwise - as simple Object. Here link where it happens - link - internally it calls following function, so throwing exception in getManagerForClass() won't allow to use package.

I faced this issue when integrating zenstruck/foundry. Fortunately, I have two connections, so with second connection configured it was working as expected. But as far as I see with new changes it won't work :anguished:
I'm pretty sure there might be some other doctrine-related packages that rely on null, and throwing exception won't allow to use them. For me also makes sense to have a method that guarantees that Manager will be returned (via throwing exception), but probably better to follow doctrine's behavior, so we won't lose possibility to integrate doctrine-related packages.

TomHAnderson commented 3 weeks ago

https://github.com/laravel-doctrine/orm/pull/644

@Hermaniandr Please inspect this change. I believe it covers what was and what we're both looking for.

Hermaniandr commented 3 weeks ago

@TomHAnderson looks great, left a minor comment regarding return annotation. Thanks!

TomHAnderson commented 3 weeks ago

https://github.com/laravel-doctrine/orm/releases/tag/3.0.3