[andreaslborg]

Description

Fixes #24594

MigrationExpressionBase.GetQuotedValue turned numeric values into SQL text with val.ToString(), which uses the current culture. On machines whose culture formats numbers differently from invariant, the generated SQL was invalid and the migration failed. This affected Insert.IntoTable(...).Row(...), Update.Table(...).Set(...) and Delete.FromTable(...).Row(...), which all go through GetQuotedValue.

• Negative numbers in cultures whose negative sign is U+2212 (−), e.g. fo-FO, sv-SE, nb-NO, fi-FI: -73001 became −73001.
• Non-integer numbers in cultures with a comma decimal separator, e.g. da-DK, de-DE: 1.5m became 1,5, which is read as two values.

Fix: In MigrationExpressionBase.cs, GetQuotedValue now uses Convert.ToString(val, CultureInfo.InvariantCulture) for numeric values and for the default (string-quoted) case, so the generated SQL no longer depends on the machine's culture.

Tests: Added MigrationExpressionBaseTests, which runs InsertDataExpression under fo-FO, sv-SE, nb-NO, fi-FI, da-DK, de-DE and en-US and asserts that negative integers, decimals, doubles and longs are rendered invariantly. Without the fix, the six non-US cultures fail.

Testing

1. Add a package migration that runs Insert.IntoTable("umbracoLock").Row(new { id = -73001, value = 1, name = "CultureRepro" }).Do();.
2. Set the current culture to Faroese, for example by adding CultureInfo.DefaultThreadCurrentCulture = CultureInfo.GetCultureInfo("fo-FO"); at the top of Program.cs.
3. Start the site on SQLite so the migration runs.
4. Verify that the migration succeeds and the row is inserted with id = -73001.

Automated: dotnet test tests\Umbraco.Tests.UnitTests --filter "FullyQualifiedName~MigrationExpressionBaseTests" passed (7/7). The rest of the unit-test suite was not run.

[github-actions]

Hi there @andreaslborg, thank you for this contribution! 👍

While we wait for someone to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids introducing unwanted changes; note that behavioral changes might also be perceived as breaking
• Have a look at what to check before you start contributing; if this is a larger PR, it's best to discuss it with Umbraco HQ first
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[AndyButland]

Thanks for this @andreaslborg. The numeric fix is looks good and I've confirmed that the tests prove it: with the change reverted, all six non-US cultures fail.

While reviewing I found a few related issues in the same code path, so I've pushed a follow-up commit:

• Date and time values. DateTimeOffset, DateOnly and TimeOnly fall through to the default branch, as they don't have their own TypeCode. With the invariant culture they came out as MM/dd/yyyy, which SQL Server reads according to the login's language, so on a day-first login it would fail or swap day and month. These types are now formatted by the syntax provider via new ISqlSyntaxProvider methods: FormatDateTimeOffset, FormatDateOnly and FormatTimeOnly. FormatDateTime already existed and was in use.
• Single quotes. SqlSyntaxProviderBase.GetQuotedValue(string) didn't escape embedded quotes, so a value like O'Brien produced invalid SQL. It now doubles them.

[AndyButland]

@claude re-review this

[claude]

Re-review summary

Thanks @andreaslborg and @AndyButland. This looks good overall. The numeric fix is correct, and the follow-up commit closes the related gaps in the same code path in a sensible way.

What looks good

• Convert.ToString(val, CultureInfo.InvariantCulture) for numeric values and the default case fixes the U+2212 minus sign and comma decimal separator problems in Insert/Update/Delete data expressions.
• The new FormatDateTimeOffset/FormatDateOnly/FormatTimeOnly methods on ISqlSyntaxProvider are non-breaking: they have default implementations with // TODO (V20) markers. V20 fits, given the PR is labelled for 17.10, 18.5 and 19.1.
• The SQL Server formats (yyyyMMdd and ISO 8601 with T) are unaffected by language and DATEFORMAT settings. The SQLite overrides match what Microsoft.Data.Sqlite writes for parameters, and the test that round-trips through a real SqliteConnection checks this well.
• The tests run under fi-FI and the other cultures, so a regression to culture-dependent formatting would fail them.

Suggestions (inline)

1. GetQuotedValue escaping also affects DDL defaults. MemberDto uses [Constraint(Default = "''")], which was a pre-escaped literal. New installs would now get a default of '' (two quote characters) instead of ' (one quote character). The intent was an empty string in both cases, but this causes schema drift between new and existing installs. Changing those three attributes to Default = "" would fix it cleanly.
2. Wording nit in the SQLite remark: FormatDateTime isn't overridden there, so it doesn't use the format Microsoft.Data.Sqlite writes for DateTime.

Minor, no action needed: On SQL Server, a TimeOnly/DateTimeOffset literal with 7 fractional digits won't convert implicitly into a legacy datetime column, only into time/datetime2/datetimeoffset. Previously this path produced culture-dependent text that was worse, so it isn't a regression.

No breaking changes found.