[8.x] Throwable error code can only be an integer

[lucacri]

While testing the automatic retrying of a transaction for concurrency issues, I noticed that it only worked if the message contained any of the strings. The error code could never match the '40001' since the $e->getCode() always returns an INT.

[taylorotwell]

Eh, maybe we should check both to be safe? I think this would be reported much more often?

[derekmd]

There's an explicit comment about the test double override that forces a string $code value:

framework/tests/Database/DatabaseConnectionTest.php

Lines 460 to 463 in 994cf4c

	/**
	* Overrides Exception::__construct, which casts $code to integer, so that we can create
	* an exception with a string $code consistent with the real PDOException behavior.
	*

Maybe @jansennerd10 can remember exactly why that was done in #29067 two years ago?

---

https://www.php.net/manual/en/class.pdoexception.php documents:

 class PDOException extends RuntimeException {
    /* Inherited methods */
    final public Exception::getCode(): int

But getCode() doesn't have a runtime type check on the return value like the other Exception methods, which permits string return values.

https://github.com/php/php-src/blob/8168d312f95fca63b638f3fab8dc987fb172b592/Zend/zend_exceptions.stub.php#L48-L51

Only methods __construct() & __wakeup() perform a type check. e.g,

https://github.com/php/php-src/blob/8bc7b1a0c036bff6f573c1939ee3a19711bfa88a/Zend/zend_exceptions.c#L320-L323

The PDO extension source contains many alphanumeric SQLSTATE codes and stores the string in a char[5] C array:

https://github.com/php/php-src/blob/63c4e8b5ab0cb47eaf167e38fa1e2dd0d3a7a330/ext/pdo/pdo_sqlstate.c#L27-L30

So AFAIK the extension raises PDOException with a string $code value?

https://github.com/php/php-src/blob/570d9b63e91ad42c7d7b4513e0072f907dc1c72e/ext/pdo/pdo_dbh.c#L107

[jansennerd10]

So AFAIK the extension raises PDOException with a string $code value?

I remember this being (bafflingly) the case... love PHP for its (lack of) consistency. Glad someone looked into the C code to figure out how it's happening.

I think this would be reported much more often?

Well... I thought the same about the original issue fixed in #29067. My guess is very few people actually use transaction modes that can auto-rollback on commit, so it's an edge case that doesn't get a lot of attention.

The error code could never match the '40001' since the $e->getCode() always returns an INT.

@lucacri can you provide more details about your setup? If you're trying to test the behavior with your own synthetic PDOException, you'll have to create the exception using a hack similar to what is in the test code, because PHP won't normally let you create one with a string code even though the real PDOExceptions have them.

If on the other hand you're observing this problem with a "live" PDOException thrown from a real database connection... what version of PHP are you using, and what database driver?

[GrahamCampbell]

This is not necessary. It will always be a string.

[GrahamCampbell]

The information in the docs is wrong unfortunately. If you look at the C code, you will see PDO exception codes are strings (I see you did, and it is a string... i don't understand the PR then?).

[lucacri]

@lucacri can you provide more details about your setup? If you're trying to test the behavior with your own synthetic PDOException, you'll have to create the exception using a hack similar to what is in the test code, because PHP won't normally let you create one with a string code even though the real PDOExceptions have them.

If on the other hand you're observing this problem with a "live" PDOException thrown from a real database connection... what version of PHP are you using, and what database driver?

You are right, I'm trying to synthetically test that the transaction would retry if an exception with the code 40001 is thrown.

Using PHP 8.0.5.

My setup is a bit different than usual since I'm using CockroachDB in production, and there are a few error messages that all have the code 40001, but the message itself is different than the ones we already have in the DetectsConcurrencyErrors.php.

The final goal would be to have that every single query passing through \DB or Eloquent would retry if it's receiving a 40001. For the time being I created a simple static function:

	public static function run(Closure $query, $attempt = 0) {
		try {
			return $query();
		} catch (Throwable $e) {
			$attempt++;
			if ($attempt >= self::MAX_ATTEMPTS) {
				throw $e;
			}

			if ($e instanceof PDOException && $e->getCode() === 40001) {
				// RETRY it
				return self::run($query, $attempt);
			}
			throw $e;
		}
	}

I had to manually check the PDOException instead of using DetectsConcurrencyErrors because of this issue.

I was testing it with (snippet):

RetryableQuery::run(function () {
			$this->testAttempt++;
			if ($this->testAttempt === 1) {
				throw new PDOException('Deadlock found', 40001);
			} else {
				return Team::first();
			}
		});

In the test, if I try to pass '40001' instead of the integer, it won't work.

I understand this might be one of PHP's funkiness, and I think that @taylorotwell edit might just be the safest approach

[GrahamCampbell]

In the test, if I try to pass '40001' instead of the integer, it won't work.

You can't pass it in the exception constructor, but you can set it directly on the exception after it's constructed.