contao / core-bundle

[READ-ONLY] Contao Core Bundle
GNU Lesser General Public License v3.0
123 stars 58 forks source link

Extend from real classes in 4.4 as well #1675

Closed m-vo closed 5 years ago

m-vo commented 6 years ago

Since https://github.com/contao/core-bundle/commit/1218f01f3888c7bc9279ba55c763b61b2a079c1f classes are extended from the real classes (instead from the aliased ones). :+1:

This change was targeted against the 4.6 branch, it should IMHO be done in the 4.4 branch as well because it breaks under some conditions.

Example: If you've got a doctine entity that - to interface with the rest of the ecosystem - uses a Contao Model as a return type like so ...

public function getFile() : \Contao\FilesModel { ... }

... doctrine's proxy generator will throw a FatalError on cache clear/warmup. In the given example during doctrine's generateMethods() the symfony class loader would be issued to load core-bundle/src/Resources/contao/models/FilesModel.php and fail with

PHP Fatal error: Class 'Model' not found in [...]/core-bundle/src/Resources/contao/models/FilesModel.php on line 8

aschempp commented 6 years ago

You should not have a entity method that returns another model. It means you are querying the database inside your entity, and that's wrong.

m-vo commented 6 years ago

Ideally the returned type would be another Entity and doctrine would resolve the reference. Otherwise every consumer needs to know about the static method calls and how to get the element. In the case of files they could for example be referenced by id or uuid - the consumer shouldn't care how it is implemented. Imo this is a valid tradeoff when mixing two ORMs.

aschempp commented 6 years ago

if you have real relations, they are loaded by Doctrine and everything works fine. Your edge case does not because you're mixing two ORMs. I don't think we can support that atm, and certainly not for 4.4 …

m-vo commented 6 years ago

Well it works for 4.6 (and of course works when omitting the return type). I'm fine with the workaround but I'm wondering if keeping the aliases breaks other things as well.

Is there a reason why this hasn't been done earlier? BC?

aschempp commented 6 years ago

yes, for BC reasons

m-vo commented 6 years ago

Could you elaborate? I don't see right now where this could break.

leofeyer commented 5 years ago

Since we don't know if the change will cause side-effects, we have deliberately implemented it in Contao 4.6 and not Contao 4.4. We should avoid such substantial changes in bugfixe releases whenever possible.