Fix #51499 PDO: add mysql-specific warning count function

[danielbeardsley]

MySQL supplies the count of warnings along with the result set and
provides a handy function to retrieve it.

Without this, the only way to discover if there has been warnings
in the last query is to run another query: "SHOW WARNINGS" OR
"SELECT @@warning_count". This requires another round-trip across
the network which makes warning discovery or reporting a drag on
performance.

Instead, we use the existing get_driver_methods function that others
(postgres, sqlite) use to provide custom functionality outside the
standard PDO interface.

Thinking about the PDO interface, I looked into several other PDO
drivers but couldn't find an analog for mysql_warning_count(). Thus I
determined that this may well stay mysql-specific.

Fixes php bug: #51499 (https://bugs.php.net/bug.php?id=51499)

[kamil-tekiela]

I feel like this should go through RFC first. I am not a fan of adding driver specific methods. Isn't there any other way we could implement this?

[danielbeardsley]

I feel like this should go through RFC first.

I'll happily do that if you still think it warrants it

Isn't there any other way we could implement this?

I feel like the options are:

1. This way, given the precedent in pgsql and sqllite
   • PDO already has support for drivers having extra functionality and postgres and sqlite make use of this
   • This mysql C function hasn't changed in decades so maintenance cost of this code is nil
2. Add to the common interface ($pdo->getWarningCount()) and let some drivers opt out
   • The problem is that in my brief survey, libraries behind drivers just don't have this feature, so I think it'd still be mysql-specific
3. Expose the underlying DB handle ($pdo->getHandle() or something so the C call mysql_warning_count() can be handled in user-land (or a php extension)
   • This seems like a more-dangerous precedent

[danielbeardsley]

Update: Here's the RFC: https://wiki.php.net/rfc/pdo-mysql-get-warning-count

[kamil-tekiela]

Can you rebase it against the master, please?

[danielbeardsley]

Ok, rebased onto master!

[danielbeardsley]

Looks like that CI failure doesn't have anything to do with this pull and is happening across many other pulls.

[cmb69]

Note that PHP 8.1 feature freeze is on July 20th.

[danielbeardsley]

I never got any negative feedback with my RFC, just a few kind folks privately saying "I like your solution, but some others likely don't" and have no idea how to initiate voting.

A tiny function to get the number of warnings in the last query. Without this, there is no sensible way in production to see if your PDO query generated warnings. Yes, it's possible without this change, but doubling the number of queries and thus network round trips is not sensible.

[cmb69]

See https://wiki.php.net/rfc/howto for details about the RFC process. The problem with this PR is not the functionality per se (which appears to be useful), but rather that many people are opposed to adding driver specific methods to the general PDO objects.

[janmartenjongerius]

Sorry for being late to the conversation. I do see your point about being vendor specific, but even still I would prefer not to have to know about this being a vendor specific feature.

If the method would have the signature: public getWarningCount(): ?int, then the method could check if an implementation exists for the current driver. If not, it returns null and if so, it returns the count.

Then, to check this in userland, whether the driver supports it or not, the following expression always works:

if ($stmt->getWarningCount() > 0) {
  // Handle warnings
}

Using this same approach, the vendor specific driver could implement a method public getWarnings(int|null $fetchMode = null, /*etc*/): false|PDOStatement. (See also PDO::query)

That way there is a generic interface, regardless of vendor, as what PDO should be all about.

If the code is really cool about it, getWarnings() automatically checks against getWarningCount and returns false if it isn't 1 or more, leaving the option to do:

if ($warnings = $stmt->getWarnings()) {
    // Handle warnings.
}

[danielbeardsley]

If the method would have the signature: public getWarningCount(): ?int, then the method could check if an implementation exists for the current driver. If not, it returns null and if so, it returns the count.

I'm all for that idea. I chose a smaller scale implementation because I thought a generic function on the PDO interface (and the maintenance that entails) and only one driver that supports it would be a harder pill to swallow.

[kamil-tekiela]

@johmanx10 I would be against implementing a function like getWarnings(). MySQL API doesn't have any function to return the warnings, so you would have to emulate a normal query, which is pointless, as you can do that easily with PDO::query().

Getting the warning count from the connection status is much easier, but I don't know how useful that is in real life. A standard PDO method would be a better choice, and it could be implemented for other drivers too. So +1 for getWarningCount()

[janmartenjongerius]

@johmanx10 I would be against implementing a function like getWarnings(). MySQL API doesn't have any function to return the warnings, so you would have to emulate a normal query, which is pointless, as you can do that easily with PDO::query().

@kamil-tekiela I understand there is less use for a method like getWarnings(), but to call it pointless does seem incorrect to me. There is a point, namely, abstraction.

If a new vendor is introduced, with the warnings functionality, or an existing vendor gets this feature, but it doesn't implement it the same as MySQL does, then using PDO::query would have to look something like:

if ($pdo->getWarningCount() > 0) {
    $query = match($pdo->getAttribute(PDO::ATTR_DRIVER_NAME)) {
        'mysql' => 'SHOW WARNINGS;',
        'acme' => 'SELECT Severity as Level, Code, Description as Message FROM acme.warnings;'
        default => null
    };
    
    if ($query !== null && $warnings = $pdo->query($query)) {
        // Handle warnings
    }
}

Versus:

if ($warnings = $pdo->getWarnings()) {
    // Handle warnings.
}

So this mostly benefits the complexity and maintenance in userland.

Again, I don't think that feature will be added, but I also don't think there is no point to it.

[janmartenjongerius]

And after thinking over my previous comments a bit, the default signature of the getWarnings method could be:

public function getWarnings(int|null $fetchMode = PDO::FETCH_CLASS, string $class = PDOWarning): false|PDOStatement;

So that the following class could be the default:

final class PDOWarning
{
    public function __construct(
        public readonly string $level,
        public readonly int $code,
        public readonly string $message
    ) {}
}

[cmb69]

Since the respective RFC has been declined, I'm closing this PR.