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

[4.2] Change the db calls back to the getDbo #38506

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Aug 17, 2022

Pull Request for Issue #38505.

Summary of Changes

When an extension is overriding the getDbo function in a model and returning it's own database instance, then the parent classes are not using it anymore as they use getDatabase now. Correct way would be to inject the database instance through the argument in the constructor config or setDbo/setDatabase.

The following code snippet shows the code issue:

class MyModel extends ListModel {
  private $mydb;
  public function __construct(..) {
    parent::__construct(...);
    $ẗhis->mydb = MyHelper::getGridDB();
  }
  
  public function getDbo() {
    return $this->mydb;
  }
}

Correctly you would write:

class MyModel extends ListModel {
  public function __construct(..) {
    parent::__construct(...);
    $this->setDbo(MyHelper::getGridDB());
  }
}

To maintain full backwards compatibility, this pr changes back the usage to getDbo.

Testing Instructions

Use the extension from issue #38505.

Actual result BEFORE applying this Pull Request

It throws an error.

Expected result AFTER applying this Pull Request

It works as expected.

@roland-d
Copy link
Contributor

@FoTo50 Can you test this pull request please?

@roland-d
Copy link
Contributor

I have tested this item ✅ successfully on f5b8086

After applying the patch the external database connection is being used which it did not use before the patch.


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

@FoTo50
Copy link

FoTo50 commented Aug 18, 2022

@FoTo50 Can you test this pull request please?

Thanks very much for this good explanation. This now works just as expected, in 4.1.5 and in 4.2.0

I think this would be a good example to modify https://docs.joomla.org/Connecting_to_an_external_database with since this page is probably for most the starting point when asking Google for it

@laoneo
Copy link
Member Author

laoneo commented Aug 18, 2022

Even when this article still uses the old code, it shows you how to do it the correct way, by using the setDbo function and not overriding getDbo. It is not safe to rely on the base class, that it uses all the time getDbo when there is even a protected variable $_db in the BaseDatabaseModel class. So I suggest you adapt your code, as this change will be reverted in 5.0.

Anyway, happy that the pr works also for you now and the issue is fixed.

@roland-d
Copy link
Contributor

@FoTo50 Great to hear it is working again.

Could you please mark your test on our issue tracker as well? All you need to do is:

  1. Go to https://issues.joomla.org/tracker/joomla-cms/38506
  2. Click Login with Github
  3. After you are logged in you can click the Test this blue button
  4. Write in the box that it works as expected
  5. Click on the Submit test result

This will then be marked here as a human test result and we can get this merged. Thank you.

@FoTo50
Copy link

FoTo50 commented Aug 18, 2022

I have tested this item ✅ successfully on f5b8086

works as expected, thnx


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

@chmst
Copy link
Contributor

chmst commented Aug 18, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 18, 2022
@fancyFranci fancyFranci merged commit 1ce74dc into joomla:4.2-dev Aug 18, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 18, 2022
@fancyFranci
Copy link
Contributor

Thank you very much!

@fancyFranci fancyFranci added this to the Joomla 4.2.1 milestone Aug 18, 2022
@laoneo laoneo deleted the model/dbo branch August 18, 2022 09:10
laoneo added a commit to Digital-Peak/joomla-cms that referenced this pull request Sep 3, 2022
HLeithner pushed a commit that referenced this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants