Framework: SQLServer unique not null -- prevents FK

[jlsjonas]

• Laravel Version: 5.9.0
• PHP Version: 7.2.18
• Database Driver & Version: sqlsrv & pdo_sqlsrv, both 5.6.1

Description:

SQL Server creates the null-excempt indexes in a different way even if the field is not nullable.

This causes FK-constraints to fail as the FK is not available

Steps To Reproduce:

run laravel/telescope migrations on an SQL server database on laravel 5.8+ with https://github.com/laravel/framework/commit/75166c484336fec7027cd0440983b13a30fe18e6 applied

Ideal solution

Expose (or use, if already exposed?) a flag indicating the field is nullable, and only add the where if true.

Alternative solution

Introduce an ignoreNull option to ->unique(), this would not apply on all DB's though as most ignore null by default

[staudenmeir]

Expose (or use, if already exposed?) a flag indicating the field is nullable, and only add the where if true.

I don't think we can make this assumption. Theoretically, you could have a nullable column with a UNIQUE index and want to reference it in a foreign key. This wouldn't be possible anymore.

[jlsjonas]

Fair point, I guess we'd have to add a flag to the ->unique() key to ignore nulls for sqlsrv-based implementations, as the where prevents (at least in SQL Server 2016) setting a FK on that index, but otherwise it's preventing having a unique constraint on a column while still allowing (multiple) nulls; f.e. for a GUID to an SSO

One other alternative I could think of (with the penalty of an extra index per unique()->nullable()) is to compile a non-unique index alongside the unique index to allow FK constraints to still function on those column(s). This might make the logic too complex / _magic_ though.

[driesvints]

I feel this is more of a feature request than an actual bug. At the very least a change in the current behavior. Feel free to open up an issue on the ideas repo to get support for your idea and discuss it.

[staudenmeir]

@driesvints Please reopen, this is a bug in Laravel 5.9.

[staudenmeir]

@jlsjonas Should we revert your PR until we find a good fix?

[driesvints]

@staudenmeir this was reported as 5.8? It's a bit confusing with the title.

Which PR caused this?

[staudenmeir]

Yeah, 5.8 is a typo. This was caused by #27938.

[driesvints]

If we can't get this fixed before the release then it's probably best that we revert the PR.

[jlsjonas]

Yeah, 5.8 is a typo. This was caused by #27938.

Not exactly, as I encountered the issue on 5.8 with specified commit patched into it, however, I can see how that led to confusion.

I definitely agree that we should revert the PR if we can't get it fixed before the release, as this could otherwise break existing packages.

If anyone has some thoughts on the best implementation strategy or wants to discuss possible solutions I'm available to a chat &/or contributing a(nother) PR.

[driesvints]

I think it's best that we revert anyway for now and accept a new pr that provides a working solution.

[driesvints]

Reverted. Feel free to attempt again with the problems here addressed. Thanks.