ENH Various fixes for PHP 8.1 compatibility

[emteknetnz]

Issue #10250

PHP 8.1 change the default behaviour of failed mysql statements from 'return false' to 'throw new mysqli_sql_exception()` https://php.watch/versions/8.1/mysqli-error-mode

Travis install failure on the --prefer-lowest job will be fixed by silverstripe/silverstripe-travis-shared#36

[GuySartorelli]

I gather the thinking here is just that we're swapping out the raw php exception to one more in line with the exceptions that are likely already being caught elsewhere in our code? This feels like something that at least needs a brief mention in changelogs, since we're now throwing an exception where previously we did not (developers would either just get a warning, or get a mysqli_sql_exception already depending on their mysqli error more config).

Also how does this react with things like pstgres or pdo?

I note the below:

This change is similar to PDO extension changing its error reporting mode to Exceptions in PHP 8.0.

Does this mean there's been an exception thrown by PDO in php 8.0 this whole time? Is it the same exception we're catching here, or a different one?

[emteknetnz]

I gather the thinking here is just that we're swapping out the raw php exception to one more in line with the exceptions that are likely already being caught elsewhere in our code?

Yes

Also how does this react with things like pstgres or pdo? / Does this mean there's been an exception thrown by PDO in php 8.0 this whole time? / Is it the same exception we're catching here, or a different one?

I don't know.

This particular issue surfaced when doing dev/build flush=1 on an empty database for kitchen-sink in CI, on graphql3 Controller::flush() - somewhere in this it tried to query a non-existent table. The false return meant it just continued, though the mysqli_sql_exception caused a hard to diagnose failure. This only happened on recipe-kitchen-sink in CI, it did not happen on installer in CI, so I'd regard this as a bit of an edge case

I don't think we shouldn't let postgres/pdo block this just yet, though I've created a new issue to investigate to see if further work needs to be done.

[GuySartorelli]

Sweet. Given that pdo and postgres are slightly separate and there's an issue created to investigate that, I'm happy to move forward with this approach. I think it's good, would you mind just adding a quick sentence on the changelogs so people know they might need to start catching this exception in places they previously didn't need to?

[emteknetnz]

I've updated the code so that it calls $this->databaseError() which itself throws DatabaseException

I don't think this change really needs a changelog update. MySQL explicitly throwing errors in PHP 8.1 is independent of Silverstripe.

I think it's correct to NOT "backport" this behaviour to Silverstripe running older versions of PHP.

[GuySartorelli]

MySQL explicitly throwing errors in PHP 8.1 is independent of Silverstripe.

Except that we're making changes in Silverstripe to account for the new thrown exception.

After taking a closer look I think I agree that it doesn't need to be added to the changelogs - but I also wonder if we should just be setting $success to false instead of throwing an error? It's not hugely likely that someone is calling this method directly, but it is public... I'm on the fence about it now.

[emteknetnz]

Have updated to set $success = false on exception. It probably make very little difference in practice due to the thrown DatabaseException, though there's no harm it setting it to false and clearly it was not successful

[GuySartorelli]

@silverstripe/core-team can we get a third opinion on this please? I think this is fine but it is a (very low risk) breaking change.
Anyone who already has mysqli_report(MYSQLI_REPORT_ERROR|MYSQLI_REPORT_STRICT) will now have a different exception than they had previously, and anyone who upgrades from a previous php version who hasn't changed that config will get an exception they hadn't previously gotten.
It will almost exclusively affect developers who are calling prepareStatement() directly, which is not terribly likely but is possible.

I think if we just pop a sentence in the changelog like "as of php 8.1, failed mysql statements throw an mysqli_sql_exception by default (https://php.watch/versions/8.1/mysqli-error-mode) - in some cases we may catch that and throw a DatabaseException instead where previously this would have failed silently" then that covers us. But it also calls to attention the fact that we're not necessarily being consistent with this (not every interaction with mysql is wrapped like this I think).

[michalkleiner]

I tried to read the convo but not sure I fully get what the options here are. Is it

1. do nothing and deal with the original mysql exception in the code calling the prepareStatement method
2. catch the exception and throw a SS-custom db exception instead
3. catch the exception but NOT throw a SS-custom exception?

If we're trying to be as PHP 8.1 as possible, I'd say do 1, and add a short mention to changelog.

[dhensby]

When we call prepare statement internally, are we then catching these errors and handling them as we would normally, or would we see application layer database calls or those made through the ORM throwing too with this change?

I think if it only throws for people calling prepareStatement directly, that's a fine change, but if these exceptions could be encountered by developers not doing anything different, then we should probably not throw it. I have a feeling the community response to new exceptions being thrown and a whole lot more try/catch blocks around ORM calls is not going to be what they are looking for in an upgrade to PHP 8.1

I'd also make the argument that if the only time these would now be thrown is direct use of the method because our internal code is just catching them all anyway, then what's the point in throwing it at all if we implicitly silence it everywhere anyway?

So, my vote would be to err on the side of keeping the current behaviour and just swallowing the error as we currently do.

[GuySartorelli]

So, my vote would be to err on the side of keeping the current behaviour and just swallowing the error as we currently do.

Keep in mind that upgrading to PHP 8.1 isn't the only way to get the mysqli_sql_exception thrown - people on PHP 7.4 could have it thrown right now depending on their configuration. so "just swallowing the error" would still be a change in that case. Basically no matter what we do here something will change for someone.

[dhensby]

Oh right, I see. Not so straightforward, then!

Given this is an explicit change in behaviour in php and not explicit change in our implementation, I think I'd be happy with either approach, but I'd still probably lean slightly towards swallowing the exceptions (it feels like the framework's job to put one try/catch in rather than expecting developers to litter their code with it) and we are then taking active steps to try to maintain some consistent behaviour across PHP versions (again, arguably one of the jobs of a framework).

[maxime-rainville]

Just trying to wrap my head around this. I take it no one is arguing that there's a legitimate use case where properly written code will call prepareStatement and would expect it to fail. Basically, if you call prepareStatement right now and get a false it means something is broken ... e.g. Your DB hasn't been built or you've mangled your SQL.

Given the above assumption, I don't think we would be breaking anything that's no already broken by throwing an Exception. So we're mostly arguing about developer experience. My instinct in those situation would be to let Exception bubble up so devs can become aware of the problem. I suspect that's probably why PHP decide to start throwing an Exception in this situation.

My instinct would be merge this.

If we're concerned that this will break someone's code somehow, maybe do nothing and point the figure at PHP8.1 when they start complaining.

Swallowing the exception wouldn't be terrible, but that's my least preferred option.

It's worth pointing out that this is a super marginal use case ... I suspect the 5 blokes on this card are the only people that will ever come across it.

[GuySartorelli]

Given that there are no strong opinions against this approach, and the general consensus is that this is okay, I'm going to accept this. I'll merge it this afternoon.

[GuySartorelli]

@emteknetnz There are new conflicts - can you please resolve those before I merge?

[GuySartorelli]

For anyone looking at this in the future, the conflicts mentioned above came from an alternative approach to resolving the same problem - see 511b3bb#diff-9b2f2c197e0cd13a273016f609eebbf671ae5db75396ae73cba64635dd7b75ba