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.4] Replace table _db with DatabaseAwareTrait #45165

Open
wants to merge 11 commits into
base: 5.4-dev
Choose a base branch
from

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

Main reason for this switch is to use the DatabaseInterface and constant developer experience.
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR-5.4-dev RTC This Pull Request is Ready To Commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants