-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.4] Replace table _db with DatabaseAwareTrait #45165
base: 5.4-dev
Are you sure you want to change the base?
[5.4] Replace table _db with DatabaseAwareTrait #45165
Conversation
Main reason for this switch is to use the DatabaseInterface and constant developer experience.
@HLeithner might be useful. Unfortunately I use my PR in production already. Cannot afford to wait for Joomla to catch up and wait for Joomla 5.4. Nice work keep on your good work. |
You don't need to wait, just please test it. |
Co-authored-by: Richard Fath <[email protected]>
Tested successfully It works even with a driver different than the one of the main database. In this case I tried Here is a screenshot of data fetched from external database using an Table instantiated via anonymous class with injected external DatabaseInterface compatible from DatabaseFactory getDriver method. |
Hey @HLeithner #45165 (comment) Tested successfully. Enjoy your weekend Super Joomlers. |
@alexandreelise Can you open https://issues.joomla.org/tracker/joomla-cms/45165 and Login with your github-account and
![]() Now the test count as successfull. |
I have tested this item ✅ successfully on 0743884 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
Thanks @fgsw . Done. Enjoy your weekend. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
@alexandreelise The PR has received a change. Could you test again? Just a quick check if it still works would be sufficient. Thanks in advance. |
@richard67 Tested it and seems to still work as expected if I didn't do a mistake will testing. For me it still works fine just as the previous screenshot I showed. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: Tuan Pham Ngoc <[email protected]>
I have tested this item ✅ successfully on d91c66d This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
@alexandreelise Sorry to bother you again, but the PR has again received changes. But I think now it's ready. Could you test again with the latest version and if ok, submit your test result with the issue tracker? Thanks in advance. |
I have tested this item ✅ successfully on 6cdbf1b This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
1 similar comment
I have tested this item ✅ successfully on 6cdbf1b This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
@richard67 Hey Richard, test done again with new changes. Seems to still work as expected. Same outcome. Tested successfully. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45165. |
When a developer does setup a database in the constructor and assigns it with "$this->_db" then we have a difference here which might lead to unintended divergence. This should be handled somehow, that's why I used magic methods in #37095. |
That's the reason I don't use The reason I didn't use the magic function is, that if someone uses this on it's own, we would have the same issue again. Using the |
PR fixes Issue in joomla-framework/database#326
Summary of Changes
Forward compatibility PR to use DatabaseAwareTrait which allows us to set a database driver which implements only the
DatabaseInterface
and does not need to extend theDatabaseDriver
.This pr introduces
setDatabase()
andgetDatabase()
in a b/c way until the_db
variable andsetDbo()
andgetDbo()
gets removed.@alexandreelise please test this pr for your use case and mark it as success if it fixes your issue.
Testing Instructions
Use Joomla, use any 3rd party extension which uses the Table class.
Actual result BEFORE applying this Pull Request
Works
Expected result AFTER applying this Pull Request
Works
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: TBD
No documentation changes for manual.joomla.org needed