[10.x] Ignore MSSQL exception about PDO::ATTR_STRINGIFY_FETCHES

[tonysm]

Fixed

• Ignores the MSSQL exception about PDO::ATTR_STRINGIFY_FETCHES. Something changed in the extension and this attribute now triggers an exception. This wasn't the case before, so I think we can ignore the exception when setting that attribute until there's a release/hotfix.

---

Here's the comment I left in another PR for context:

I found this reference from Drupal, where they are using a try/catch when setting the PDO attributes. It looks like something changed in the SQL Server extension. Some other references from php/php-src#11587 and a #47937. A PR was merged already in the microsoft/msphpsql#1468, but I still see the issue. It looks like they are arranging a microsoft/msphpsql#1468 (comment). In the meantime, the solution described in the package referenced in microsoft/msphpsql#1468 (comment) seems to be similar to what the Drupal folks did.

An alternative solution would be to remove that attribute from the list of $options in the SqlServerConnector class.

Hat tip to @SakiTakamachi for the implementation here.

[tonysm]

I just noticed there's a draft PR from @crynobone to ignore the MSSQL suite temporarily. That might be a better approach than changing the connector code here...

[crynobone]

It is better to let @taylorotwell decide on this. There are pros and cons to both solutions

[taylorotwell]

Can someone explain the pros / cons? 😅 Mark as ready for review when done.

[tonysm]

From what I understand reading the threads linked, a change was made in php-src, and the PDO core extension now also passes down the attributes set to the underlying PDO driver extension. It looked like the pdo_sqsrv extension wasn't expecting this and started throwing an error. The fix follows what the Drupal folks are doing. In both cases, we assume that the PDO core sets the attribute internally and then forwards it to the extension. Since the extension throws the exception (because it doesn't know that attribute), we assume it's safe to ignore it, as the attribute should have been successfully set in the core PDO object.

This should be handled in the next release/hotfix of the pdo_sqlsrv, but that may take some time. They said the next release was planned for the end of the year. But they are considering the possibility of a hotfix.

Perhaps @SakiTakamachi can add more context to this. I couldn't find the commit in php-src that introduced this change (forwarding the attributes to the underlying extension).

[SakiTakamachi]

Hi.

I'm probably one of the most knowledgeable on this subject.

First, let me explain how this issue came about.

Last month I made a modification in php-src, including PDO Core, to fix mysql behavior.

As for the C language, the change was to pass the PDO::ATTR_STRINGIFY_FETCHES setting to the set_attr() of the PDO drivers from Core.

The PDO driver just returns false on failure of set_attr(), such as when an unsupported attribute is passed, so this shouldn't be a problem.

However, only PDO SQLSRV, managed outside of php-src, was implemented with its own design that instead throws an exception when it should return false.

Of the 16 PDO drivers provided by PECL, only SQLSRV causes problems.

I submitted a monchy patch to PDO SQLSRV and it was merged into the development branch.

But the PDO SQLSRV lead didn't want to solve this problem on the PDO SQLSRV side.

We are currently urging Microsoft to release the hotfix as soon as possible, but the lead has not yet responded and there is no change in the situation.

[SakiTakamachi]

Now let's talk technically about the problem.

The error is on the driver side of SQLSRV.

After the PDO core accepts the attribute value, it passes it to the SQLSRV driver. The SQLSRV driver should have returned false, but it throws an error.

This error is triggered by receiving an unsupported attribute value, so you can safely ignore it by swallowing it with a try catch.

Alternatively, you can set PDO's error mode to silent mode just before calling set_attribute(), and change it back after calling one.

[SakiTakamachi]

In any case, I can assure you that the error is not caused by something broken internally and can be safely ignored.

[tonysm]

Thanks for the context @SakiTakamachi. Marking this as ready for review so @taylorotwell can check it again.

[SakiTakamachi]

For your reference.

Changes of PDO Core:
https://github.com/php/php-src/blob/3e0e7e3f9016e9c09c484cf7050a960be20da860/ext/pdo/pdo_dbh.c#L799

Where the exception occurs in PDO SQLSRV:
https://github.com/microsoft/msphpsql/blob/f6f76d4ac116411b6c53ce482c16da73a0652292/source/pdo_sqlsrv/pdo_dbh.cpp#L1367

At first glance, it looks like the exception is caught, but actually this code is executed before that and the exception is raised:
https://github.com/microsoft/msphpsql/blob/f6f76d4ac116411b6c53ce482c16da73a0652292/source/pdo_sqlsrv/pdo_util.cpp#L519

[SakiTakamachi]

@taylorotwell

I have read the contents of this PR.

Adopting this PR will allow Laravel's CI to PASS, allowing users to avoid errors caused by SQLSRV in later versions of Laravel.
However, it will leave a temporary monkey-patch history in the repository.

If you adopt another PR, Laravel's CI tests will pass. No monkey patches remain in the source code.
However, users will continue to suffer from SQLSRV errors until this issue is truly resolved.

[SakiTakamachi]

Originally, I planned to submit a PR to Laravel for the contents of this Laravel library.

https://github.com/SakiTakamachi/laravel-sqlsrv-err-avoid

However, since the content is nothing more than a temporary adhesive bandage, there is a history of providing it as a library.

[SakiTakamachi]

If you provide a temporary bandage, the problem arises of when to remove the bandage, if a hotfix for SQLSRV is released.

My library explicitly states that the repository will be deleted 6 months after a hotfix release.

I personally think that if Laravel itself provides a bandage, it should be carefully considered when to remove it.

[SakiTakamachi]

I wrote a lot of long sentences, but that's it.

[tonysm]

@SakiTakamachi when the MSSQL PDO driver releases the hotfix, we should be able to remove the changes here, no? Or do you mean we may need to keep the changes if we want to support the affected versions of pdo_sqlsrv? Not sure if it's worth keeping it. Either way, even if we keep it, it won't break anything.

[SakiTakamachi]

This means that not everyone updates their drivers immediately after they are released.
Not everyone uses an environment where they can freely install and update middleware.

On the other hand, in most environments you should be able to update Laravel freely even if you can't update the driver freely.

Then there will be users who use Laravel with the bandage removed, even though the drivers are still outdated.

There is an option to not change versions of Laravel, but when critical security updates are made, not being able to use the new version is a disadvantage for users.

On the other hand, I don't think it's a good idea to leave the bandage on forever, so what I wanted to say is to discuss how to remove the bandage in advance.

[SakiTakamachi]

Also, while we know exactly what the issue is, we can't expect everyone to have the same level of understanding.

If this PR is adopted and ostensibly solves the problem, many users may mistake it for a permanent solution.

In that sense, it is only a temporary measure, and I think it would be better if there was some way to encourage drivers to update as soon as they are released.

Otherwise, ignorance can damage Laravel's reputation.

[SakiTakamachi]

If there is a third option, it is to provide functions like my library as a separate repository/package like laravel-sail.
If it is a Laravel official package, it will be easy to incorporate it into composer's dependencies.
And I think it's easier to convey that it's a band-aid rather than modifying the main body.

However, I don't know much about Laravel's policy, so I'm sorry if this is an irrelevant suggestion.

[tonysm]

Yeah, I understand what you mean. I don't usually add external packages to fix core issues. In this case, the problem is at the extension level, which is odd. I don't think we should merge this anymore. At this point, I'd recommend folks patching their apps internally until the fix is released at the extension. The mssql integration suite has been disabled for now already, so we should be able to bring it back when the fix is released.

I am closing this PR for now. But feel free to re-open it and merge if y'all think it's worth it.

[SakiTakamachi]

Hi.

I would like to inform you that a hotfix has been released!

[tonysm]

@SakiTakamachi Thank you for the heads up. Not sure why, but the tests are still failing. Maybe the GitHub action doesn't have the hotfix yet? #48343

[SakiTakamachi]

@tonysm

Looking at the execution log of github actions, it seems that it's using PDO SQLSRV 5.10.1.

The one it should use is 5.11.1.

The setup-php definition may be outdated.

---

My prediction was correct.

https://github.com/shivammathur/setup-php/blob/master/src/scripts/extensions/sqlsrv.sh

[GrahamCampbell]

shivammathur/setup-php#766 should fix that.

[GrahamCampbell]

setup-php v2.26.0 now loads the latests version of the extension on supported PHP versions.