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

Is_callable throws an exception if PDOStatement method does not exist. #1258

Closed
Beakerboy opened this issue May 12, 2021 · 6 comments
Closed

Comments

@Beakerboy
Copy link

Beakerboy commented May 12, 2021

PHP Driver version or file name

  • latest using pecl install sqlsrv pdo_sqlsrv on 2021-05-12

SQL Server version

  • 2019

Client operating system

  • Linux

PHP version

  • 8.0.2

Microsoft ODBC Driver version

  • msodbcsql17

Table schema

  • None

Problem description

  • calling is_callable() on the sqlsrv version of a PDOStatement throws an exception if the method does not exist. PDOException: SQLSTATE[IMSSP]: This function is not implemented by this driver.

Expected behavior and actual behavior

  • it should return false

Repro code or steps to reproduce

$pdo = new \PDO("sqlsrv:Server=localhost;Database=mydrupalsite", "sa", "Password12!");
$pdoStatement = $pdo->prepare("SELECT id FROM test");

// This Works
$functionExists = is_callable([$pdoStatement, 'bindParam']);
$this->assertTrue($functionExists);

// This throws an exception: 
$functionExists = is_callable([$pdoStatement, 'boo']);
$this->assertFalse($functionExists); 
@yitam
Copy link
Contributor

yitam commented May 12, 2021

Thank you @Beakerboy for bringing this to our attention. While I understand it may not be an expected behavior to throw an exception, it's been like this since version 3.0 for pdo_sqlsrv. We will get back to you on this.

@Beakerboy
Copy link
Author

The following was recently added to Drupal core:

public function __call($method, $arguments) {
    if (is_callable([$this->getClientStatement(), $method])) {
      @trigger_error("StatementWrapper::{$method} should not be called in drupal:9.1.0 and will error in drupal:10.0.0. Access the client-level statement object via ::getClientStatement(). See https://www.drupal.org/node/3177488", E_USER_DEPRECATED);
      return call_user_func_array([$this->getClientStatement(), $method], $arguments);
    }
    throw new \BadMethodCallException($method);
  }

It successfully runs on Postgres, MySQL, and SQLite...potentially others as well. The GOOD thing is that in this particular case, an exception would be thrown regardless, but of a different type.

@yitam
Copy link
Contributor

yitam commented May 12, 2021

Thanks @Beakerboy so it's a matter of throwing different exceptions?
I mean we can easily change pdo_sqlsrv to return false instead of throwing an exception, but there might be users out there who have relied on the exceptions

@Beakerboy
Copy link
Author

Beakerboy commented May 12, 2021

From a semantics perspective, it would make sense for a function named is_callable() to return true or false, not true or throw and exception, no? If PDO aims to abstract away specific database behavior to make it easier to develop universal applications, matching the behavior between implementations makes the most sense. If other drivers do it the way I propose, AND it makes more syntactic sense, you would only be helping people by making more consistent behavior.

@yitam
Copy link
Contributor

yitam commented May 18, 2021

Hi @Beakerboy
I've fixed this and you should see the change in the next preview release.

@yitam
Copy link
Contributor

yitam commented Sep 8, 2021

Closing this issue as per 5.10.0-beta1

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

No branches or pull requests

2 participants