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

Explicitly register the model classes #1951

Merged
merged 3 commits into from Jul 21, 2020

Conversation

leofeyer
Copy link
Member

I have realized that using Model::getRelated() still relies on the old Contao 3 class autoloader to find the related model class. This is because Model::getClassFromTable() only converts tl_page to PageModel and not to Contao\PageModel and therefore we still have "composerized" classes in Contao 4.10 when we really should not anymore. 😄

This PR explicitly registers the fully qualified model class names.

@leofeyer leofeyer added the bug label Jul 21, 2020
@leofeyer leofeyer added this to the 4.10 milestone Jul 21, 2020
@leofeyer leofeyer requested a review from a team July 21, 2020 09:37
@leofeyer leofeyer self-assigned this Jul 21, 2020
Toflar
Toflar previously approved these changes Jul 21, 2020
bytehead
bytehead previously approved these changes Jul 21, 2020
fritzmg
fritzmg previously approved these changes Jul 21, 2020
@ausi
Copy link
Member

ausi commented Jul 21, 2020

Couldn’t we instead adjust the getClassFromTable method and return the class name with the Contao\ prefix if it exists?

@bytehead
Copy link
Member

Couldn’t we instead adjust the getClassFromTable method and return the class name with the Contao\ prefix if it exists?

Thought about the same, but I'm not sure about potential side effects?

@fritzmg
Copy link
Contributor

fritzmg commented Jul 21, 2020

imho we should deprecate that functionality

@leofeyer
Copy link
Member Author

leofeyer commented Jul 21, 2020

Couldn’t we instead adjust the getClassFromTable method and return the class name with the Contao\ prefix if it exists?

Besides the potential side-effects, this would also be a BC break. Someone might have a class Vendor\PageModel and use $class = 'Vendor\\'.Model::getClassNameFromTable('tl_page').

@leofeyer
Copy link
Member Author

imho we should deprecate that functionality

We can only deprecate it once we have an alternative. And Model::getRelated() somehow needs to determine the class name.

@fritzmg
Copy link
Contributor

fritzmg commented Jul 21, 2020

We can only deprecate it once we have an alternative. And Model::getRelated() somehow needs to determine the class name.

I only meant deprecating the automatic inference of the model class according to the table name instead of looking it up in TL_MODELS.

Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt you want to override $GLOBALS['TL_MODELS'] in every config file? 😂

@bytehead
Copy link
Member

@ausi
Copy link
Member

ausi commented Jul 21, 2020

Besides the potential side-effects, this would also be a BC break. Someone might have a class Vendor\PageModel and use $class = 'Vendor\\'.Model::getClassNameFromTable('tl_page').

This would already fail if in the application config a custom class is registered via $GLOBALS['TL_MODELS']. IMO this is not a BC break, but if this method really gets used in such a way it might be better to not change it.

@leofeyer
Copy link
Member Author

but if this method really gets used in such a way

It certainly was not intended to be used this way! But you know…

@leofeyer leofeyer dismissed stale reviews from fritzmg, bytehead, and Toflar via 2016f09 July 21, 2020 13:43
@leofeyer
Copy link
Member Author

I doubt you want to override $GLOBALS['TL_MODELS'] in every config file? 😂

Fixed in 2016f09.

fritzmg
fritzmg previously approved these changes Jul 21, 2020
@ausi
Copy link
Member

ausi commented Jul 21, 2020

$class = 'Vendor\\'.Model::getClassFromTable('tl_page') would also break with this version of the pull request right?

aschempp
aschempp previously approved these changes Jul 21, 2020
@aschempp
Copy link
Member

$class = 'Vendor\\'.Model::getClassFromTable('tl_page') would also break with this version of the pull request right?

I think it would. But that would be a bug in the extension code, which is easily fixable and fully backwards compatible.

@ausi
Copy link
Member

ausi commented Jul 21, 2020

But I agree, explicitly setting the class names should still be preferred. How about adding a deprecation notice here?

(I think @fritzmg suggested something like that?)

@fritzmg
Copy link
Contributor

fritzmg commented Jul 21, 2020

Yep. Adding a deprecation notice there is what I meant :)

@leofeyer
Copy link
Member Author

Deprecation added in c88a355.

@leofeyer leofeyer requested review from ausi and fritzmg and removed request for Toflar and bytehead July 21, 2020 15:26
@leofeyer leofeyer merged commit e159caa into contao:master Jul 21, 2020
@leofeyer leofeyer deleted the fix/register-models branch July 21, 2020 15:39
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this pull request Apr 6, 2021
Description
-----------

I have realized that using `Model::getRelated()` still relies on the old Contao 3 class autoloader to find the related model class. This is because `Model::getClassFromTable()` only converts `tl_page` to `PageModel` and not to `Contao\PageModel` and therefore we still have "composerized" classes in Contao 4.10 when we really should not anymore. 😄

<img width="724" alt="" src="https://user-images.githubusercontent.com/1192057/88038330-5c9a8080-cb46-11ea-9976-6441d9ab906a.png">

This PR explicitly registers the fully qualified model class names.

Commits
-------

9857c38 Explicitly register the model classes
3cb436f Append to TL_MODELS instead of overwriting it
c88a355 Trigger a deprecation for unregistered tables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants