[4.0] Get the dbtype from the DatabaseDriver Instance

[twister65]

Pull Request for Issue in Comment #25507 (comment) .

Summary of Changes

In Joomla! 3.x and earlier the mysql type was used for the ext/mysql PHP extension, which is no longer supported. The pdomysql type represented the PDO MySQL adapter. With the Framework's package in use, the PDO MySQL adapter is now the mysql type.
Also the PDO PostgreSQL is only supported.

The configuration file is saved according to these requirements.

Testing Instructions

Install Joomla 3.9.x with postgresql or pdomysql driver.
Update Joomla with the build of this PR.
Check Global Configuration -> Server -> Database Type.
Save to update the configuration file with the correct dbtype.

Expected result

postgresql driver is replaced by pgsql (PDO driver).
pdomysql driver is replaced by mysql (PDO driver).

Actual result

As expected.

Documentation Changes Required

[richard67]

I have tested this item ✅ successfully on 13470be

Code review ok.
PostgreSQL native to PDO: ok.
MySQL PDO: ok.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25729.}

[mbabker]

This isn't really necessary, https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Service/Provider/Database.php handles the re-mapping on-the-fly. TBH I would be pretty cautious about having code that arbitrarily changes configuration.php on each update.

[richard67]

Hmm, valid point from Michael.

[HLeithner]

Also this could doesn't has a proper check if the new database driver really exists. And I think this change should be done in j3 update checker?

[mbabker]

After reading the conversation that prompted this, I'd say just close the pull request. The code in the service provider has to be there to make sure the app can boot up right, and this code that changes the configuration file runs too late in the request cycle to rely on it to make the change. Just use the code that handles it in the bootup process and don't worry about the configuration file not being a 1-for-1 match here, it is not harming anything.

[mbabker]

And I think this change should be done in j3 update checker?

You can't. 3.x and 4.x are incompatible with what the configuration file identifies as the "mysql" driver, if you change it on 3.x you are instructing the platform to use the deprecated driver for ext/mysql support, whereas on 4.x that name is used for the PDO MySQL driver.

As for the PostgreSQL change, as noted in the service provider, it is arbitrarily done because we know the user has already configured their environment for PostgreSQL usage, core only provides one of those drivers in 4.0. If their environment doesn't support PDO PostgreSQL (the driver used in 4.0) then they will get an error noting as much thanks to the service provider mapping, instead of getting an error about an unknown database driver.

There is a reason the changes were made in the way they were. Please review the pull requests and the inline code comments. The approach was the most sane for everyone involved, don't go overengineering things for the sake of added complexity. Unless it can be proven that not rewriting the configuration file when updating 3.x to 4.x is problematic (as in fatal error type of problematic), I see no reason to pursue this idea any further.

[richard67]

The problem is that when you update a staging which uses the native PostgreSQL driver to a 4.0-Alpha-11-dev (without this PR) and then goto server tab of global configuration in backend, the database type which is shown is "MySQL (PDO)", and that is definitely wrong and confusing.

[mbabker]

Then put some code in com_config so that it reads the actual database driver in use instead of the value from the configuration file. Same should apply to 3.x because that too has some auto mapping logic.

[richard67]

Then put some code in com_config so that it reads the actual database driver in use instead of the value from the configuration file. Same should apply to 3.x because that too has some auto mapping logic.

Sounds reasonable. @twister65 Will you do that? If not I can try my best, but I am not sure when I will find time, can be next weekend.

[mbabker]

(Either way I don't think having the update script arbitrarily rewrite the configuration file on every update is the way to go here, especially because I worry someone might accept this pull request then propose to remove the mapping code at the service layer thinking the upgrade will automatically sort everything out without actually understanding why doing that will FUBAR upgrades for anyone not using MySQLi or PDO PostgreSQL, and if you don't understand the order of operations in the update cycle then you wouldn't see the issues in doing so)

[richard67]

@mbabker Do I assume right that when we change as you proposed and then some admin in backend saves global configuration, it will be correct in the config file after that anyway? If so, it would be ok for me.

[mbabker]

If you fix com_config correctly then it will both show the right info in the form when viewing the global config as well as saving the right value when the file is updated.

Long and short, I would not rely on reading the database type from the config file anywhere but that service provider code because of the auto mapping logic, anything beyond that I would use Joomla\CMS\Factory::getDbo()->getName() so you have the correct driver at runtime.

[richard67]

If you fix com_config correctly then it will both show the right info in the form when viewing the global config as well as saving the right value when the file is updated.

That would be my preferred solution then.

[richard67]

Well my test result here is still good because this PR works as described, regardless whether it's the best way or not to fix the issue. Shall I set it back to "not tested"?

[twister65]

So what is the purpose of $dbtype in the configuration file ?

[twister65]

This file will be updated once and for all with the right driver.

[mbabker]

To set the database type that is the default one to be used in an environment. But the service layer has code to change the type at runtime if need be for a number of reasons. So yes, it is preferable that is right, but this PR is IMO just adding extra code for the sake of it.

IMO the “right” way to do it is to make sure com_config shows the right value and updates the file correctly when that is saved. This PR doesn’t account for other scenarios where the driver might change at runtime and to handle all of those adds a lot of complexity.

What is the real error being solved with this pull request, other than the driver in config is not the driver at runtime because of a mapping layer?

[twister65]

This PR should solved the global configuration interface:
Global Configuration -> Server -> Database Type is MySQL (PDO) when $dbtype in the configuration file is no longer supported. Previously, It displayed the first element by default. Luckily, it displayed the MySQL (PDO) with the pdomysql driver.

[richard67]

@twister65 I know it solves that, so I gave it a good test result. But Michael tries to convince you that it's the wrong way to solve it. For me either way is ok, Michael's is a bit preferred.

[mbabker]

And that’s why I suggested a better fix is reading the value from the driver at runtime and not making this fix. There are other reasons the driver can change at runtime that aren’t accounted for (PHP config change where PDO isn’t installed so you go from PDO MySQL to MySQLi). This PR solves the mapping logic for changes between major versions but that is all.

[twister65]

Alright, I modify the com_config component to read the DBO driver at runtime.

[richard67]

Here in this PR or in other? Let me know when ready for test, I still have time today.

[richard67]

@twister65 Now you should update the title of this PR so it describes the right thing. It also needs to change the last sentence in the summary of changes.

[richard67]

@twister65 You should not include the update package zip in that PR.

[richard67]

I have tested this item ✅ successfully on 8e0dda9

Code review ok.

Update staging to patched 4.0.0-alpha11-dev:

• PostgreSQL native to PostgreSQL (PDO): ok.
• PdoMySQL to MySQL (PDO): ok.

Verified that the modified update package contains only the change from this PR: ok.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25729.}

[richard67]

@twister65 I just remember now that @mbabker recommended to do that in staging, too. @wilsonge Should this PR be done for staging instead of 4 and be later then merged into 4?

[wilsonge]

I think this is fine for J4. I mean there's no reason we can't put it in staging - but neither is anything going to change in staging that makes this change necessary. In 3.10 we also need to put something in Joomla Update to try and persuade people to change their db driver over before they upgrade.

[mbabker]

I mean there's no reason we can't put it in staging - but neither is anything going to change in staging that makes this change necessary.

There is a lighter version of the mapping code that deals with the ext/mysql deprecation and trying to move people onto MySQLi or PDO depending on what the environment supports. So the same issue of "the global configuration screen doesn't show the actual database type in use" is present there to a lesser degree.

In 3.10 we also need to put something in Joomla Update to try and persuade people to change their db driver over before they upgrade.

The 3.x to 4.x migration is going to realistically impact two groups:

• PostgreSQL users (who can make the update in 3.x to switch from the ext/pgsql driver to the PDO driver)
• PDO MySQL users (who cannot make the update in 3.x because the code level identifier for "mysql" is the deprecated ext/mysql driver)

So, probably not worth the effort. The mapping code is more than adequate for everyone and with this fix you don't end up saving a completely FUBAR database config.

[richard67]

Second tester please.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25729.}

[alikon]

I have tested this item ✅ successfully on 8e0dda9

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25729.}

[alikon]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25729.}

[HLeithner]

Thanks for finding the real db.