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

[Lock] fix PDO SQLSRV throws for method_exists() #43316

Closed
wants to merge 1 commit into from
Closed

[Lock] fix PDO SQLSRV throws for method_exists() #43316

wants to merge 1 commit into from

Conversation

GDmac
Copy link
Contributor

@GDmac GDmac commented Oct 4, 2021

pdo_sqlsrv driver 5.9.0 (on mac) always throws an exception for any call on method_exists().
(see microsoft/msphpsql/issues/1306)

executeStatement() is a DBAL-method, and exec() is PDO.
fix by excluding method_exists() check for PDO type of connections

Q A
Branch? 4.4 (and later)
Bug fix? yes
New feature? no
Deprecations? no
License MIT

pdo_sqlsrv driver 5.9.0 on mac throws an exception for any call on method_exists().
(see microsoft/msphpsql/issues/1306)
executeStatement() is a DBAL-method, exec() is PDO.
fix by excluding method_exists() check for PDO type of connections
@carsonbot
Copy link

It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you.

Cheers!

Carsonbot

@derrabus derrabus added the Lock label Oct 4, 2021
@carsonbot carsonbot changed the title fix PDO SQLSRV throws for method_exists() [Lock] fix PDO SQLSRV throws for method_exists() Oct 4, 2021
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can you please confirm there are no similar changes needed in any other components, and add them to this PR if not?

@GDmac
Copy link
Contributor Author

GDmac commented Oct 4, 2021

I'm not using any other components that use PDO in this way (e.g. http-foundation, process)
Looks like..., yes Cache, DoctrineBridge, Messenger have the same issue probably.

I was hoping for a hot-fix so we could deploy, since the prune command crashed and made locks hang.
I'll close the pull request for now and move the fork to my personal account.
Do you want me to search and replace other PDO instances i can find ?

@GDmac GDmac closed this Oct 4, 2021
@nicolas-grekas
Copy link
Member

Do you want me to search and replace other PDO instances i can find ?

would be nice yes 🙏

@GDmac
Copy link
Contributor Author

GDmac commented Oct 5, 2021

For now i have updated our Fork of the lock component, so we could deploy.

Can you help with the pull request?
In my opinion the (not instanceof \PDO) is not so nice.
also, seems like DBAL Connection class for SQLSRV also has an exec() method.
can we check for DBAL instead?

                if ($conn instanceof Connection && method_exists($conn, 'executeStatement')) {
                    $conn->executeStatement($sql);
                } else {
                    $conn->exec($sql);
                } else {

@derrabus
Copy link
Member

derrabus commented Oct 5, 2021

seems like DBAL Connection class for SQLSRV also has an exec() method.

Correct, but it has been deprecated and removed in later DBAL versions. This is why we have that code: If our connection is a DBAL connection that already implements executeStatement(), we want to call that method. Otherwise, we fall back to exec().

can we check for DBAL instead?

Yes please! Your proposed condition is the way to go, imho:

if ($conn instanceof Connection && method_exists($conn, 'executeStatement')) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants