Skip to content

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Feb 19, 2022

Summary of Changes

Injects the database instance into the model. So we don't have to rely on the global db anymore. Additionally it deprecates the use of the database in the model constructor and uses the aware interface and trait from the db package.

Testing Instructions

  • Browse around in the back end and front end of Joomla
  • Open the back end user list view
  • Open the back end modules list view

Actual result BEFORE applying this Pull Request

All works.

Expected result AFTER applying this Pull Request

All works.

@wilsonge
Copy link
Contributor

This looks wrong to me. The only place that should be database aware is the model. All controller and views database queries should be eventually moved into the model. not be made database aware

@joeforjoomla
Copy link
Contributor

joeforjoomla commented Feb 22, 2022

This looks wrong to me. The only place that should be database aware is the model. All controller and views database queries should be eventually moved into the model. not be made database aware

Totally agree with @wilsonge

@laoneo
Copy link
Member Author

laoneo commented Feb 23, 2022

This looks wrong to me. The only place that should be database aware is the model. All controller and views database queries should be eventually moved into the model. not be made database aware

I agree too, and the problem is that there are queries which do not actually fit into a model, for example against the extensions table. I mean I can move all of them to the model of the view, but would be nicer to have them just from the beginning on the right place.

@laoneo laoneo marked this pull request as ready for review February 23, 2022 05:19
@wilsonge
Copy link
Contributor

This looks better. I don't think that factory should be databaseaware. Just the model. But definitely better than what we had before.

@laoneo
Copy link
Member Author

laoneo commented Feb 25, 2022

True, removed the interface.

@BPBlueprint
Copy link

I have tested this item ✅ successfully on 67cdd83


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37095.

@laoneo laoneo requested review from wilsonge March 19, 2022 08:11
@roland-d roland-d added this to the Joomla 4.2.0 milestone Apr 14, 2022
@roland-d roland-d merged commit 7e21799 into joomla:4.2-dev Apr 14, 2022
@roland-d
Copy link
Contributor

Thank you everybody

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.

7 participants