contao / contao

Contao Open Source CMS
https://contao.org
GNU Lesser General Public License v3.0
350 stars 159 forks source link

The model class defined in $GLOBALS['TL_MODELS'] is not used in Controller::getContentElement() #298

Closed Defcon0 closed 5 years ago

Defcon0 commented 5 years ago

Affected version(s) 4.x (we're coding on 4.4.x)

Description
We're currently trying to get DC_Multilingual working for tl_content. The problem is that when content elements are generated in Controller::getContentElement(), the following call is made:

$objRow = \ContentModel::findByPk($intId);

But we need to use our own ContentModel inheriting from the class Multilingual. Therefore, we now had to override the whole ContentModel class using composer psr-4 which is not update safe :-(

But wouldn't it be correct to have it that way:

$strClass = $GLOBALS['TL_MODELS']['tl_content'];
$objRow = $strClass::findByPk($intId);

Thanks!

leofeyer commented 5 years ago

I guess it should be

$strClass = Model::getClassFromTable('tl_content');
$objRow = $strClass::findByPk($intId);

@contao/developers Do you agree? And is this a bug fix or a new feature?

Toflar commented 5 years ago

This is a bugfix.

Defcon0 commented 5 years ago

I guess there are many points in the code where this situation is given (especially old code). Maybe we should scan the code for these occurences, I mean directly called model classes?

dmolineus commented 5 years ago

I wonder if $GLOBALS['TL_MODELS'] is really designed to make model classes exchangeable? Then basically every single method call on a specific model would be wrong as @Defcon0 pointed out.

Afair the registry was introduced to allow custom namespaces for own models and not to exchange core models.

Defcon0 commented 5 years ago

Even if wasn't designed to do so it should in the future. Modules like DC_Multilingual wouldn't make any sense for existing DCAs if we couldn't replace the Model with the one inheriting from Multilingual.php (in this case).

leofeyer commented 5 years ago

I wonder if $GLOBALS['TL_MODELS'] is really designed to make model classes exchangeable?

Actually it is not. Its purpose is to allow the Model::getClassFromTable() method to find models that cannot be mapped by their class name.

@contao/developers WDYT about enhancing this in a future release?

Toflar commented 5 years ago

Thinking about it again, I don't think that should be done really. Models are not extensible by design. If we would do that, people would maybe start doing this in their extensions and as soon as two extensions do it for the same model, nothing will work anymore.

Defcon0 commented 5 years ago

Mhm, then https://github.com/terminal42/contao-DC_Multilingual would be invalid, i.e. unusable, no :-(? Because here I must override the core model in order to benefit from this module in core modules.

Toflar commented 5 years ago

Yeah, it's designed to be used in your own models. You cannot use it for any other model you have no control over.

Defcon0 commented 5 years ago

Mhm, this is sad. So no multilanguage support for events, news, content :-( I don't think this is a good decision for the future. Would've been better to have this functionality built into the core.

I understand the example with multiple extensions manipulation the model. I've had this issue in several older extensions. I solved it by taking the situation into account and building a Model that contains the changes both extensions contain. If a developer decides to have multiple extensions doing a similar task he must take this situation into account.

leofeyer commented 5 years ago

As discussed in Mumble on March 7th, the $GLOBALS['TL_MODELS'] array is not meant to be used this way, therefore we are not going to support this use case.

Defcon0 commented 5 years ago

Too bad :-( Maybe we can have such a feature in an upcoming rewrite of the models in doctrine (if planned) via service tag.