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

[5.3] Add missing MVCFactoryInterface into model constructors #44295

Open
wants to merge 7 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

Denitz
Copy link
Contributor

@Denitz Denitz commented Oct 17, 2024

Summary of Changes

There are models which don't unitilize the MVCFactoryInterface $factory constructor argument for calling parent's constructor.

This results to useless extra calls to Factory::getApplication()->bootComponent() in BaseDatabaseModel::__construct.

Testing Instructions

Apply patch, ensure no errors on actions with patched models.

Actual result BEFORE applying this Pull Request

Model constructors are missing MVCFactoryInterface $factory argument.

Expected result AFTER applying this Pull Request

All good.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@rdeutz
Copy link
Contributor

rdeutz commented Oct 17, 2024

Looks good to me, but I would make this for 5.3 and not for 5.2. Would give us some more time to test and be sure we don't have unexpected side effects.

@Denitz Denitz changed the base branch from 5.2-dev to 5.3-dev October 18, 2024 05:13
@Denitz Denitz requested a review from laoneo as a code owner October 18, 2024 05:13
@Denitz
Copy link
Contributor Author

Denitz commented Oct 18, 2024

@rdeutz Great, rebased to 5.3-dev

@Denitz Denitz changed the title Add missing MVCFactoryInterface into model constructors [5.3] Add missing MVCFactoryInterface into model constructors Oct 18, 2024
@rdeutz rdeutz removed the PR-5.2-dev label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants