Fatal error: Uncaught Error: Non-static method PDO::__construct()

Bugs are fixed in the Git branch code, which is used to create the development branch.
A version (which is created from a Git tag) cannot be fixed, once released; only the next version and the development snapshot can be fixed.

Apologies in advance. I'm probably being super dumb. Kiamlaluno states that the fix is available on the 7.x.-2x-dev branch.
I've cloned the repo via ssh as an authenticated user 'taylormadeapps' but I cannot see this branch available on the origin.
Thinking that maybe I have to be a project member to access?

I stated that bugs are fixed in the Git branch, which is used to create the development snapshot.
On Drupal.org, version 7.x-2.6 will never be fixed, as it's been already published.

The link for the 7.x-2.x development snapshot is given in the project page.

So, the 7.x-2.x development snapshot hasn't been updated since the 9th of March, and doesn't currently contain any differences between the 2.6 release.
Can we assume that we are awaiting a fix to be published at this time.
Can I also assume, in the git repo, any changes will be available on the 7.x-2.x branch?
Thanks in advance for your patience.

Hi, I am having the same issue. Can you let me know how can I fix this issue please, Thanks

I know all the project members are selflessly contributing their own time an effort on this project so I do not want to seem pushy or ungrateful. I would humbly like to enquire if anybody is working on this or intends to. We're in a situation that all Drupal 7 instances that use MsSQL are unable to be patched beyond Drupal 7.78 - so are exposing highly critical vulnerabilities out there in the wild. It would be for the common good, and of course my own selfish ends, if this issue were addressed promptly or at least the community given some time lines on what to expect. I'm a php noob at best so would contribute a full fix if I could. I've got the driver working in part with v7.80 through trial and error and a good dose of guess work, but still got a few issues that I can't clear up not knowing the driver architecture - but if I can help in anyway with moving this along please let me know.

Hi all, I'm not a heavy contributor or even a PHP expert, but maybe somebody who is can look and see if this works and maybe create a patch if they agree that it does. Here is what I did to fix the Fatal Error in sqlserv\database.inc based on what I saw in the sqlite file and mysql file.

Line 145: change PDO::__construct to parent::__construct
Line 843:
Remove \PDO::beginTransaction();
Add

if (!$this->inTransaction()) {
	$this->connection->beginTransaction();
}

Line 896: change if (!PDO::commit()) to if (!$this->connection->commit())

I am concerned that if \PDO::rollBack(); on line 780 gets called it will also throw a Non static method error, but haven't quite figured out how that one should be changed yet.

Apologies if this isn't the proper way to provide contributions.

I tried submitting a comment with a way to possibly patch the issue but I guess I got marked as spam. I have (from what I can tell so far) fixed the issue locally but I'm not sure how to provide the information here since I do not seem to be a confirmed user.

Edit: I've been confirmed, ignore this post

Thanks jkaufman, those changes make a lot of sense to me from my initial foray into solving this. I couldn't quite work out the transaction stuff though, so I had a site that renders, but could not save any content :)
I think if we can solve the rollback issue then we have a workable fix.
I'll have a bit more of a play around today, now I have something to go on re the transactions.
Will report back if/when I make some progress.

Have pushed the proposed changes to the issue fork for review.
Still need an assist with the PDO::rollback statement.

Jkaufman, I'm thinking that:

\PDO::rollBack(); on line 780
should be replaced with
$this->connection->rollBack();

also perhaps?

none of the other drivers have this transactional stuff in as far as I can see, so I don't have an example to go by.

taylormadeapps, based on the PDO.rollback documentation and compared to beginTransaction that looks like the proper usage to me.

Hi taylormadeapps and jkaufman

Thanks a lot for contributing to the issue. It will be such a great help. Is it possible for you to please post all the code changes, for us to try out.

Many thanks,

Here is a patch file with the code changes.

sadarangans #16 and #20 have all of the code changes listed

rraney Yes, the patch and the fork that taylormadeapps submitted should be the same code. I just wasn't sure if having a patch would help the process so I went ahead and made one. Hopefully that didn't confuse things.

The 7.x branches of the sqlsrv module are no longer being maintained at this point. Sorry. I think the 7.x-2.x branches require a closed-source database abstraction layer, so I am unable to verify any fixes for that branch. Personally, I am completely unfamiliar with the 7.x codebase in general as well.

I don't think we're quite there, I've had my tester report a 500 error when trying to change a select field for their user account.

#0 F:\www\includes\database\sqlsrv\query.inc(33): DatabaseSchema_sqlsrv->queryColumnInformation('semaphore')
#1 F:\www\includes\lock.inc(134): InsertQuery_sqlsrv->execute()
#2 F:\www\includes\bootstrap.inc(438): lock_acquire('schema:runtime:...')
#3 F:\www\includes\bootstrap.inc(460): DrupalCacheArray->set(Array)
#4 [internal function]: DrupalCacheArray->__destruct()
#5 {main}
thrown in F:\www\includes\database\sqlsrv\schema.inc on line 141
[12-May-2021 16:57:47 Europe/London] PHP Fatal error: Uncaught DatabaseSchemaObjectDoesNotExistException: The table 'semaphore' does not exist. in F:\www\includes\database\sqlsrv\schema.inc:141

And that 'semaphore' table very much does exist. All works fine with Drupal v7.78 and the current sqlsvr driver.

Regarding the `empty($table)` use, it’s possible that at some point the `tableExists()` method would throw an exception if $table was some sort of empty value. This may have been a way to control the error handling. I suggest that if `tableExists()` handles that case in a way that is appropriate, there’s no reason to check it in `queryColumnInformation()`.

I maintain the D8/D9 version of the driver, and there was a lot of weird stuff I had to clean out from there to get it working correctly. I’m sure D7 needs housekeeping too. If you are able to run the core test suite with Sql Server you’ll probably find a lot of little problems.

Sounds like the patch has been reviewed, but not quite right. Please submit another patch for review.

@rraney, if you find a new issue, please file a new issue. It’s usually easier to review and approve them one at a time versus making one patch that covers all the issues.

Have checked in the fix suggested by Beakerboy to the issue fork. e.g. to remove the empty table check from scheme.inc
That's solved that problem for me. Still got some weirdness around ajax calls in views and content edit admin pages, but getting a lot closer.

I think there's two issues to be unpicked here.

Issue #1 D7 driver contains a lot of cruft - ideally needs refactoring. Obviously that's unrealistic as we all acknowledge that it's not worth the effort and there's never going to be new functionality that warrants this.

Issue #2# Drupal Core v7.79 change a few minor things in the database driver implementation. To my noob eyes the changes don't look that extensive. This has flushed out some issues with the SQLSvr D7 driver. Hopefully these can be tidied up with a little bit of effort without a major refactor.

D7 is going to be around for a long while to come yet, as used by NGOs who simply don't have the resources to brave the dreaded D8/9 upgrade path. I do believe it's for the common good to patch this old driver up, so it still works, and do our bit to keep sections of the internet secure that can't afford to migrate.

I'd consider contributing to a modest bounty/reward if someone who knows the D7 branch could champion this.

@taylormadeapps, I have a slack channel set up to discuss sqlsrv issues. By all means check in there and continue the conversation, or keep it here...I don’t care either way, but thought I’d throw that out if you like slack (#sql-server). We can discuss strategy if you would like.

@rraney, I think the only thing you can do is edit the comment and remove the content.

What do you mean by “working on this module”? If you mean actively maintaining it, then that number is zero.

@rraney, regarding your question in #26, maybe the driver needs to implement a new function that replaces driver() or getConnection() is allowed (or encouraged) to return a different object type that renders driver() pointless. Maybe driver() has been deprecated and is now removed from core.

I seem to remember a project in D8 whereby core would automatically know what the database driver was, and if it was a non-core driver, include the files automatically. This line may have been removed because core may have already included the files in an ideal world.

@rraney
Regarding comment #26, that line does not appear in the version of that file that was included in the official release. Did someone else in your team add it at some point?
https://git.drupalcode.org/project/drupal/-/blob/7.78/modules/system/sys...

Also, could someone provide what versions of Drupal, PHP, and pdo_sqlsrv, and the sqlsrv driver you are using with success? I’m seeing this fatal error all the way back to PHP7.0.

As an update to the empty() question...the reason this is there is because the sqlsrv implmentation of Schema::tableExists() includes a reference to $table[0], the first character in the table name string. If the table name is empty, this check will cause a fatal error. I don’t think removing the call to empty() would fix the semaphore issue. However, it probably does make sense to remove it for consistency, but the check should be moved to TableExists().

I figured this out. One month ago drupal core changed the DatabaseConnection class so that it no longer extends PDO. Because PDO was an ancestor of the sqlsrv connection class, PDO::__construct() was valid syntax. Now that it is not, we cannot call the constructor statically.

In case anyone here want to investigate this with CI tools, I have an Appveyor environment set up on this repository:
https://github.com/Beakerboy/sqlsrv/tree/7.x-2.x

It installs sql server 2016, PHP7.1.33, pdo_sqlsrv 5.6.1, Drupal 7.70, and the latest sqlsrv driver code, and runs the core test suite using scripts/run-tests.sh.

I have it set to just run the Database tests and there are a handful of failures. If anyone here wants to ensure that this driver works, and continues to work, this is how to do it.

The whole issue fork/GitLab integration is new to me, but that seems like it. Make sure you look at the correct branch of the issue fork.

To me, the patch looks like it should cover the bases, but without having an actual D7 site to test it against, I don’t want to vouch for it. When @taylormadeapps said he was still seeing errors, I had to take that as a vote against it. Now that I have a test environment in Appveyor that works, I can see how it affects everything.

The only change I might make is to change the line from parent::__construct(...) to $this->connection = new PDO(...). It seems like the constructor was a complete replacement of the parent, so this would keep the separation. I have not tested this, but it seem like it would be a more accurate replacement for that line.

I’ve started a new issue where I plan to list all the test failures in the driver. If you want to weigh in on what should be fixed.
#3215085: Drupal 7.78 core database failures.

You’re right..habit. Fixed.

Let me know if this is accurate. We have a module that “works” in one version of Drupal, but breaks upon core upgrade. We have a patch that appears to allow the site to upgrade, but new bugs are presented. Has anyone tested the patch under the original Drupal version? Does the patched driver operate successfully in Drupal 7.78?

If 7.78 is unaffected by the patch, and fixes the Fatal Error, my vote would be to apply the patch to the repository. If new errors come up after upgrading with the new driver version, these should be filed as a new issue and worked on. I don’t see where anyone has demonstrated that the patch causes these new regressions or if core changes cause the new regressions.
Has anyone compared calling new PDO() versus parent::__construct()?

Again, I’m don’t use this module and have not tested it. If @jkaufman made the patch, then @rraney and @taylormadeapps should chime in on their thoughts on it it’s been Reviewed And Tested.

The usual process is to specify which version of the patch you have tested, and your opinion, “needs work” or RTBC. A patch can alter multiple files. If you feel more changes need to be made then are in the provided patch, make the change and upload it, or change the issue fork and let us know. When everyone likes what’s in there, I’ll merge it.

/includes/database/sqlsrv/schema.inc Is not a core file, it’s in the driver...unless you are referring to something else.

You could check out the repository with git in a new location, apply the patch with git apply Foo.patch, then copy the files to your test site. Would that work on windows? With git installed, you could probably check out the issue fork as well and use those files.

Any time you are given an error message, please include that in your post.

There should be a way to construct the driver so that it works in both versions of core. The error is caused because calling a PHP object constructor is only valid if that object is an ancestor of the current object. When the parent of our class removed PDO as ITS parent, our call to PDO::__construct() was no longer valid. We should still be able to call the parent or PDO with new PDO().

Alternatively, we could add an if/then to look at the Drupal version and have two different versions of the constructor, and call the correct code in each case. This is probably to only way.

Can you try this:

if (VERSION <= 7.78) {
  PDO::__construct([stuff]);
} else {
 $this->connection = new PDO([stuff]);
}

Odd,

Call to a member function beginTransaction() on null would imply that $this->connection is null, but it is set to PDO by parent::__construct(). You’re using the code in the issue fork? You replaced the code in wwwroot\includes\sqlsrv as well as in wwwroot\sites\all\modules\sqlsrv ?

The ideal process would be to update the driver, browse the site looking for errors, flush cache, update drupal core, then browse again and flush cache again. If we can come up with code that all this can happen without errors, we will be golden.

Also, please be more descriptive with Drupal core versions and sqlsrv versions. When you say “revert to current” I assume you mean 7.x-2.6. If so, please say that so there’s no confusion. Was this latest update in core 7.78? If so, then the error makes sense. Core 7.78 does not define $this->connection in the parent. We might need another if/then

if (VERSION <= 7.78) {
    PDO::beginTransaction();
} else {
    $this->connection->beginTransaction();
}

And the rollback() change should be wrapped the same way.

if (VERSION <= 7.78) {
    PDO::rollback();
} else {
    $this->connection->rollback();
}

I've updated the issue fork with all three conditionals. Please test the changes against both 7.78 and 7.80

Have we verified that removing the empty() check is necessary?

I just added a fourth version check for the PDO::commit() change.

Again, What are the error messages? Is there anything different between the sites that might be causing a difference in AJAX errors?

Have you opened the separate issue? Why can't you copy and paste the AJAX errors? Is there nothing in the web browser console log, nothing in the drupal log? Can you at least describe what it is doing? There might be a way to force the page to print the error to the screen or the error log.

Are you saying that the code as-is in the issue fork is completely working successfully on one of two 7.80 sites, and it works fine on 7.78? Are the two 7.80 sites copies of each other, or different development sites? Do they have different modules? Different themes? Maybe a custom module that is incompatible with either the sqlsrv changes or D7.80?

What do you think you still need before you call this RTBC, and good to merge into the main codebase?

For anyone watching this issue, my plan is not to keep the if/else statements in the driver indefinitely.

#3215864: Mark new versions for D7.79+

My plan is to create one "bridge release" that works on pre 7.79 and 7.79+. I will immediately make a release that is only 7.79 and greater.

This will allow users to test and upgrade stepwise if they desire.

@taylormadeapps gave it an RTBC in slack, so I’ll merge this change.

Thanks for the input everyone.