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

Implement PDO based DB drivers for MySQL and PostgreSQL #4193

Merged
merged 24 commits into from Apr 14, 2021

Conversation

euantorano
Copy link
Member

@euantorano euantorano commented Dec 13, 2020

This PR adds two new database drivers for MySQL and PostgreSQL that make use of PDO.

It also amends the SQLite driver to not just die if the database file cannot be opened.

It also fixes #4192.

{
// first, check for an IPv6 address - IPv6 addresses always start with `[`
$openingSquareBracket = strpos($hostname, '[');
if ($openingSquareBracket === 0) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This logic is not yet tested. I believe that other drivers doing much the same thing would never have worked with IPv6 addressed DB hosts...

return false;
}

if (preg_match('/^\\s*SELECT\\b/i', $query->queryString) === 1) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: using num_rows with PDO is not optimal at all. We need to review usages within the core and see when/why we use num-rows and if they can be replaced so that we don't hit this path too often.

This works better with the old mysql_ type functions as those functions buffer results internally, which PDO does not do.

return $fieldstring;
}

public function __set($name, $value)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This special handling is important to ensure compatibility.

@euantorano euantorano marked this pull request as ready for review December 13, 2020 20:04
@euantorano
Copy link
Member Author

I haven't yet tested the MySQL PDO driver, but have tested the Postgres one. I'd like to land this in 1.8.25 but not make too much noise about it. Then during the cycle for 1.8.26 I plan to add some more methods to all of the DB drivers to support prepared statements (which will be emulated for some drivers).

Enahnce the dbpdoEngine class in a few ways:

- Disable emulated prepares.
- Rather than dying when failing to connect, throw an exception.
- Make some properties private where possible.
- Add comments to properties.
- Add `query_prepared` method.
- Use object hash to identify queries, rather than assigning an ID property.
- Default name of query for `insert_id` to `NULL` rather than an empty string. This matches the underlying function.
- Set visibility for functions.
Implement `insert_query`, `insert_query_multiple`, `update_query`.
Use heredoc to format HTML for query explanations, which makes the HTML more readable.
Moved some methods to the postgres specific PDO class as they have different requirements (such as rewriting LIMITs).
Escaping is handled slightly differently to the standard pgsql driver
@euantorano
Copy link
Member Author

I've now done some testing of this with MySQL too and ran through an install and a few key pages.

I'd very much appreciate it if others could test this PR. Upgraded boards can opt-in to the PDO based drivers by modifying inc/config.php to alter the DB engine to one of the following options:

$config['database']['type'] = 'mysql_pdo';
$config['database']['type'] = 'pgsql_pdo';

Once altered, there shouldn't be any visible change if all goes well.

@euantorano euantorano mentioned this pull request Dec 22, 2020
10 tasks
@lairdshaw
Copy link
Contributor

I'd very much appreciate it if others could test this PR.

I haven't done any systematic testing, but have been using the mysql_pdo driver for about two weeks on my local dev board whilst working on a plugin, and have had no problems so far. Seems good.

@euantorano
Copy link
Member Author

euantorano commented Feb 13, 2021 via email

inc/AbstractPdoDbDriver.php Outdated Show resolved Hide resolved
Previous incarnation missed the closing `]` for IPv6 addresses, and had some questionable logic.
inc/db_pgsql.php Show resolved Hide resolved
@euantorano
Copy link
Member Author

euantorano commented Mar 25, 2021 via email

@euantorano
Copy link
Member Author

I'm going ahead and merging this, it's had a fair bit of testing now I believe.

@euantorano euantorano merged commit 67f1ee8 into mybb:feature Apr 14, 2021
@euantorano euantorano deleted the develop/pdo-pgsql branch April 14, 2021 16:44
lairdshaw pushed a commit to lairdshaw/mybb that referenced this pull request Oct 11, 2021
[Rebased for 1.9 by Laird]

* dbpdoEngine enhancements

Enahnce the dbpdoEngine class in a few ways:

- Disable emulated prepares.
- Rather than dying when failing to connect, throw an exception.
- Make some properties private where possible.
- Add comments to properties.
- Add `query_prepared` method.
- Use object hash to identify queries, rather than assigning an ID property.
- Default name of query for `insert_id` to `NULL` rather than an empty string. This matches the underlying function.
- Set visibility for functions.

* Working on PDO versions of MySQL and PostgreSQL DB

* Implement further postgres PDO methods.

* Fix spelling, implement some more methods

* Improve SELECT query detection.

Implement `insert_query`, `insert_query_multiple`, `update_query`.
Use heredoc to format HTML for query explanations, which makes the HTML more readable.

* Implement `show_fields_from`.

* Implement more DB methods for postgres PDO driver

* Move database methods for Postgres PDO connector

* Start working in integrating Postgres PDO driver

Moved some methods to the postgres specific PDO class as they have different requirements (such as rewriting LIMITs).

* Handle escaping binary and decoding binary

Escaping is handled slightly differently to the standard pgsql driver

* Implement missing methods. Handl DB type property.

* Starting work on mySQL PDO driver.

* Implement more mYSQL PDO methods

* Fix mybb#4192 - set default when modifying columns

* Drop the query_prepared function for now.

* Handle exception opening SQLite DB.

* Implement all MySQL functions.

* Handle mysql_pdo DB type.

* Prefer string inteprolation over concatenation.

* Check for MySQL in supported PDO drivers.

* Make explain property public and fix formatting of explain. Fix list_tables method.

* Close cursor for query in free_result.

* Update function to parse hostname

Previous incarnation missed the closing `]` for IPv6 addresses, and had some questionable logic.

* Fix unreachable statement
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.

DB_MySQLi::modify_column does not set default value
4 participants