[6.x] revert PR #33453 (backport of #33430)

[rodrigopedra]

This is the same as PR #33488 but target at 6.x branch, text below is adapted from that PR

As noted by issue #33485 PR #33453 (backport of #33430) breaks a Laravel app when using the freeTDS driver to connect to SQL Server.

PR PR #33453 (backport of #33430) was sent to address issue #32883, but when implementing it I didn't test it against freeTDS drivers. I only tested against Microsoft official drivers ( https://github.com/Microsoft/msphpsql ). It seems freeTDS is widely used , specially with older versions of SQL Server.

After the issue was reported I started investigating a solution that could solve both issues #33485 and #32883 but I could not find one.

So I am sending this PR to revert the changes made. Issue #32883 will still need to be addressed after reverting it.

I shared some of my findings on a comment on PR #33430 discussion and also presented some alternatives. Please refer to comment #33430 (comment) to have more details about it.

There I said I would send PR for both 6.x and 7.x branches, but I will wait on feedback on this one before sending the second one as maintainers could have a different workflow on back porting features and fixes.

If someone has a better suggestion on how to address this I would be glad to help.

[joelharkes]

It seems freeTDS is widely used , specially with older versions of SQL Server.

I'm not sure for laravel latest version to keep support older versions of SQL server, this can be lacking behing.
instead of reverting it maybe its a better solution to provide a config variable/legacy where old working still works behind?

[GrahamCampbell]

Laravel 6 and 7 both support SQL Server 2017 and 2019, but no other versions. You will probably find most features work on 2016 too, but there is no official support.

[rodrigopedra]

Problem is not specifically with older SQL versions, my remarks on that might have sounded misleading.

Problem is using freeTDS driver and not the official driver from Microsoft to connect to the database.

freeTDS handles calls with multiple statements differently and does not return the expected value when running those.

My remark is that, from my research, freeTDS is used mostly with older SQL Server version, but they support newer versions. Therefore some users might be using that driver to connect to a newer version.

I would add to documentation that only the official driver is supported. But as Laravel 6 is LTS and those apps worked before that PR, it would be a breaking change to add this requirement to that.

I guess it would be fine to add that requirement to a newer Laravel version (8 maybe). Reason I reverted PR is that it was targeted to 6.x and 7.x and broke some apps.

[GrahamCampbell]

Can we not raise an issue with the broken driver to see if they can change their code to be compatible?

[rodrigopedra]

Sorry I don't quite understand your request, might be due to not being a native speaker.

You mean to add checks to the SQL processor to see which driver is being used? similar to what is done for the ODBC driver already?

Reference:

framework/src/Illuminate/Database/Query/Processors/SqlServerProcessor.php

Lines 26 to 30 in 97c8d87

	if ($connection->getConfig('odbc') === true) {
	$id = $this->processInsertGetIdForOdbc($connection);
	} else {
	$id = $connection->getPdo()->lastInsertId();
	}

If that is what you mean I think it would solve the issue. It is also one of the suggestions I made on how to handle it:

#33430 (comment)

[mfn]

He meant opening e.g. an issue at https://github.com/FreeTDS/freetds :)

[rodrigopedra]

thanks for the heads up =)