Maintenance: Towards PHP8, by adding more type hints. Align PDO::errorCode() with specification.

[amotl]

About

Running the standalone example programs added with GH-145 revealed lots of deprecation warnings and subsequent type errors.

Details

1. Maintenance: Towards PHP8, by adding more type hints.
2. Maintenance: Align PDO::errorCode() with official PDO specification ¹.
   According to the specification, the function should return either an error code as string type, or null. Its signature is:

   public PDO::errorCode(): ?string
   

   On the other hand, our implementation returned int error codes. This changed now, and has been advertised properly as breaking change. Personally, I don't think it will have too much impact, as most error handling in userspace programs will be handled by exceptions anyway, as far as I could see, and within CrateConst.php, it looks like only ERR_INVALID_SQL had internal references at all. Please advise if you think this flaw should not be fixed, or differently.

Footnotes

1. https://www.php.net/manual/en/pdo.errorcode.php ↩

[seut]

👍

According to the specification, the function should return either an error code as string type, or null

error code as string, unbelievable 🙄

[seut]

Suggested change

	Breaking changes
	Breaking Changes

[matriv]

LGTM. Thank you!

[amotl]

Error code as string?

PDO::errorCode()'s offical documentation states that it

returns an SQLSTATE, a five characters alphanumeric identifier defined in the ANSI SQL-92 standard.

So, I think PHP is correct on this detail. Whether we made a mistake in the past, by defining the error codes within CrateConst.php not quite correctly, and just forwarding CrateDB's error code 1:1, which originally appears to be in integer format ¹, is beyond what I am currently able to grasp, i.e. I've not investigated it yet, and will be happy to hear back from you about it.

If you think we are up to something here, and the error code handling would need further adjustments, for example by mapping internal CrateDB error codes defined in CrateConst.php to the ones defined by ANSI SQL-92's SQLSTATE, and whether you think we need to go further here, please let me know. Now, it's the right time to fix such eventual shortcomings of previous versions.

Footnotes

1. You can inspect at ², that the code will be casting this to a string now, using $this->errorCode = strval($result['code']). ↩

2. https://github.com/crate/crate-pdo/pull/146/files#diff-d212dd2b5bd10a50c66f0b18298062aaebd573a232d2031daeded268e45d1e76 ↩

[seut]

If you think we are up to something here, and the error code handling would need further adjustments, for example by mapping internal CrateDB error codes defined in CrateConst.php to the ones defined by ANSI SQL-92's SQLSTATE, and whether you think we need to go further here, please let me know. Now, it's the right time to fix such eventual shortcomings of previous versions.

CrateDB's error codes are currently somehow aligned with HTTP error codes instead of codes defined by the SQLSTATE. I think CrateDB itself should move on to SQLSTATE eventually for SQL Standard compatibility.
I'm not sure if the situation really improves if we map CrateDB's own codes to the SQLSTATE at one client only, I'd tend to leave out the mapping for now.

[amotl]

It took a few more adjustments to satisfy PHP's type checker wrt. PHP7 vs. PHP8 balancing, Codecov's patch coverage rules, and Scrutinizer's quality assessment. It looks like every validation step succeeded now.

-- https://scrutinizer-ci.com/g/crate/crate-pdo/inspections/dfe6c8e8-a803-438d-9027-516419fb7ac0