Skip to content

[5.4] Replace table _db with DatabaseAwareTrait #45165

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

Merged
merged 10 commits into from
Mar 27, 2025

Conversation

HLeithner
Copy link
Member

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 the DatabaseDriver.

This pr introduces setDatabase() and getDatabase() in a b/c way until the _db variable and setDbo() and getDbo() 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

@alexandreelise
Copy link
Contributor

@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.

@HLeithner
Copy link
Member Author

You don't need to wait, just please test it.

@alexandreelise
Copy link
Contributor

You don't need to wait, just please test it.

Tested successfully

It works even with a driver different than the one of the main database.

In this case I tried mysql driver for the external database which is used for PDO Driver (but PDODriver is an abstract class) and the main joomla database untouched from your pr is using mysqli driver

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.

Screenshot from 2025-03-20 12-24-59

@alexandreelise
Copy link
Contributor

Hey @HLeithner #45165 (comment) Tested successfully. Enjoy your weekend Super Joomlers.

@fgsw
Copy link

fgsw commented Mar 20, 2025

@alexandreelise Can you open https://issues.joomla.org/tracker/joomla-cms/45165 and

Login with your github-account and

  1. click button "Test this"
  2. mark "Tested successfully"
  3. click button "Submit test result".
test-pr-submit

Now the test count as successfull.

@alexandreelise
Copy link
Contributor

I have tested this item ✅ successfully on 0743884

Please refer to https://issues.joomla.org/tracker/joomla-cms/45165#event-815648


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

@alexandreelise
Copy link
Contributor

Thanks @fgsw . Done. Enjoy your weekend.


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

@richard67
Copy link
Member

@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.

@alexandreelise
Copy link
Contributor

@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.

@joomdonation
Copy link
Contributor

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.

@richard67
Copy link
Member

@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.

@alexandreelise
Copy link
Contributor

I have tested this item ✅ successfully on 6cdbf1b

@richard67 Still working as expected. Same outcome than before.


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

1 similar comment
@alexandreelise
Copy link
Contributor

I have tested this item ✅ successfully on 6cdbf1b

@richard67 Still working as expected. Same outcome than before.


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

@alexandreelise
Copy link
Contributor

@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.

@richard67 richard67 removed the Feature label Mar 23, 2025
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 23, 2025
@laoneo
Copy link
Member

laoneo commented Mar 24, 2025

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.

@HLeithner
Copy link
Member Author

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 databaseAwareTraitDatabase write only. databaseAwareTraitDatabase also can't be read by any child class. It's only set for documentation purpose.

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 _db until we remove it, seems to be the better way for me.

@richard67
Copy link
Member

@laoneo Are you ok with Harald's answer to your comment?

@laoneo
Copy link
Member

laoneo commented Mar 26, 2025

I guess this is fine here. The hard bc break comes, when $this->_db get removed.

@muhme muhme force-pushed the 5.4/replace-table-setdatabase branch from 96aeed1 to c4e8039 Compare March 27, 2025 17:19
@richard67 richard67 merged commit d21e8be into joomla:5.4-dev Mar 27, 2025
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 27, 2025
@alexandreelise
Copy link
Contributor

Yooooohooooo!! Merged! Thanks for your hard work Super Joomlers! @richard67 @HLeithner @muhme @joomdonation and others

@richard67 richard67 added this to the Joomla! 5.4.0 milestone Mar 27, 2025
@richard67
Copy link
Member

Thanks

@alexandreelise
Copy link
Contributor

Thanks

My pleasure, Richard. Enjoy your weekend.

SharkyKZ added a commit to SharkyKZ/joomla-cms that referenced this pull request Apr 24, 2025
SharkyKZ added a commit to SharkyKZ/joomla-cms that referenced this pull request Apr 24, 2025
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