-
Notifications
You must be signed in to change notification settings - Fork 2k
Updates to breaking changes #4673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dd398c1 to
640acf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
entity-framework/core/what-is-new/ef-core-6.0/breaking-changes.md
Outdated
Show resolved
Hide resolved
entity-framework/core/what-is-new/ef-core-7.0/breaking-changes.md
Outdated
Show resolved
Hide resolved
| <a name="discriminators"></a> | ||
|
|
||
| ### Discriminators now have a max length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main breaking change here is that EF 8.0 will generate a migration changing the discriminator column from nvarchar(max) to nvarchar(x), right? If so, I'd make it clear that this unexpected migration gets generated etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not really the break we're trying to document. The migration is an appropriate way to manage the change. However, this is not the case when Migrations is not able to automatically make the change, such as when the column is part of an index, since the index would need to dropped and re-created, and EF doesn't do that, so it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, ok. It still may be worth explaining that an unexpected migration is created (and why) as background etc. But no big deal either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I added another sentence to that effect.
Co-authored-by: Shay Rojansky <[email protected]>
| protected override void OnModelCreating(ModelBuilder modelBuilder) | ||
| { | ||
| modelBuilder.Entity<Blog>() | ||
| .ToTable(tb => tb.UseSqlOutputClause(false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ajcvickers we still only show the convention below for adding triggers to all tables (rather than using the new UseSqlOutputClause() mechanism) - I'm wondering whether we're missing functionality or something for doing UseSqlOutputClause() from a convention (or should we just doc it?)
|
@roji I asked @AndriySvyryd about this (I think on Teams) back when I did the PR. We're missing the APIs used for bulk config. |
|
Ah OK, thanks for the clarification! |
Fixes #4608
Part of #4538
Fixes #4315
Fixes #4257
Fixes #4655
Fixes #4654