Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
PHP 8.0 is now released, and we recently committed the MySQL 8 compatibility fixes to core. I waited for that issue before submitting this ticket.
A major change in PHP 8.0 is
Fatal errors on incompatible method signatures, and the first fatal error from the signature mismatch in \DatabaseConnection::runQuery($query, array $args = array(), $options = array()), that overrides \PDO::query($statement, $mode = PDO::ATTR_DEFAULT_FETCH_MODE, $arg3 = null, array $ctorargs = array()).
As you can see, the $args parameter is widely used within Drupal, but it not compatible with PDO::query, which now results in fatal error.
Steps to reproduce
- Update to PHP 8.0
- Open site.
Proposed resolution
Because we extend PDO class, rather than composing with it, it is not possible to seamlessly update this method.
Drupal fortunately encapsulates the \DatabaseConnection::query($query, array $args = array(), $options = array()) method, and use the classes that extend the DatabaseConnection class, so we can rename query method to a new one, and leave PDO::query method intact.
I went ahead and refactored DatabaseConnection::query() method to runQuery, and it worked!
This of course, is the solution I could come up with, and is not an elegant one. I'd love to hear any other alternative approaches.
Remaining tasks
None.
User interface changes
None.
API changes
- Communicate this change. DatabaseConnection::query is declared public (as all methods in Drupal 7 are), so we will need to communicate this change.
Data model changes
None.
Release notes snippet
Due to PHP 8.0's signature checks on LSP,
DatabaseConnection::querymethod was renamed toDatabaseConnection::runQuery. All database drivers will now need to rename to this method for PHP 8.0 compatibility.
Issue fork drupal-3185918
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
Ayesh CreditAttribution: Ayesh commentedComment #3
MustangGB CreditAttribution: MustangGB commentedSo
DatabaseConnection::query()isn't used in core, other than inincludes/database(and one test)?Which presumably means it's quite unlikely that the majority of contrib modules would be affected.
This seems like the most sensible solution, I guess the other option would be to not extend
PDO::query(), which if a contrib module is doing something funky I imagine would be an even bigger breaking change.Are there any well used contrib modules that might likely be using this that we could ensure have a heads-up?
Comment #4
MustangGB CreditAttribution: MustangGB commentedQuick edit of patch to see if it will apply.
Comment #7
MustangGB CreditAttribution: MustangGB commentedUpdated to fix test failure.
Comment #8
Ayesh CreditAttribution: Ayesh commentedHi @MustangGB thanks for the comments and opening a PR.
I really wish there was a way to do it without renaming the query method, but it appears to be the only way.
I was thinking Views module might do something with the database class directly, but it does not. However, database connection modules will almost certainly extend this method to work out the differences, so I'd expect them to break.
Comment #9
fgmAfter reading the discussion, I think this patch breaks our compatibility policy, since Database::query() is not marked as @internal, so it needs to remain compatible for the sake of contrib.
Changing the class inheritance to compose PDO instead of inheriting from it seems IMHO to be less intrusive, as inheritance from \PDO is not part of the API compatibility promise AIUI. Plus, this is how D8/D9 does it anyway, the PDO connection is not just a field on the Database class.
To minimize risk further, this new version of Database could also proxy PDO methods to reduce issues even further. As a reference point, D8/D9 doesn't care, and requires explicit access to the connection, so maybe this wouldn't be needed here either.
Comment #10
Ayesh CreditAttribution: Ayesh commentedThanks a lot for the great points,@fgm. You are right, this will be a BC break, and an ugly one at that.
I was thinking about composing PDO as opposed to inheriting too, like you said. It would be a BC break IMO, because any method that used `\PDO` in parameter/return/property types would then get a TypeError because `DatabaseConnectiin` no longer fulfils it.
If we were to proxy PDO, I think there will be a call forwarding overhead because now every method is called via proxy method. If we go with magic methods, we might also lose some IDE autocompletion goodness unless we provide stubs or create a full proxy with all methods.
Comment #11
joseph.olstad@TODO:
looks like we need to patch drush 8.x for PHP 8 like we did to get mysql 8 working
Feel free to open a new issue or find an existing issue for this and link it here, then open an issue upstream with the drush project.
Comment #14
Taran2LSo, I've added an alternative branch with composing the PDO object rather than extending it. There is a BC layer allowing to proxy any direct calls of the PDO methods to the actual connection.
The code is basically identical or very similar to the D8/9 implementation.
MySQL/SQLite are both happy. PostgreSQL is not, but afaik D7 is not being tested with PostgreSQL regularly anyway. See https://www.drupal.org/node/3060/qa
Please review the MR.
Tests on PHP8.0 won't even start before (at least) #3200407: [PHP8] ArgumentCountError: Too few arguments to function _drupal_error_handler() and friends is committed.
Comment #15
Taran2LComment #16
Taran2LAlright, PostgreSQL has well-known issues with D7, like #998898: Make sure that the identifiers are not more the 63 characters on PostgreSQL
Comment #17
MustangGB CreditAttribution: MustangGB commentedSeems like my concerns of no longer extending
PDOhave been covered by the addition of__call(), so should be good on that front.Comment #18
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, only concern is that there is no getPDO() or whatever method to get to the actual underlying object.
The __call however is great and will definitely help with BC here.
I think getPDO is not necessary overall and we can always add it quickly in case someone really needs the raw object and __call is not enough.
Comment #22
mcdruidThanks!
Comment #23
joseph.olstadVery cool, thanks to everyone above! One step closer.