Skip to content
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

Don't configure JsonOptions by default #16837

Merged
merged 16 commits into from
Oct 9, 2024
Merged

Don't configure JsonOptions by default #16837

merged 16 commits into from
Oct 9, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Oct 4, 2024

Fix #16831

@hishamco
Copy link
Member

hishamco commented Oct 4, 2024

I have two notes about this:

  1. It introducing a breaking-change
  2. Why we have it before?

@MikeAlhayek
Copy link
Member Author

It introducing a breaking-change

I would say this is a bug in 2.0 and should be fixed. If someone already build on the top of the bug, we should fix the bug and release it. We should document it as a breaking change due to a bug.

Why we have it before?

Not sure but I added it in #15460

@hishamco
Copy link
Member

hishamco commented Oct 4, 2024

I would say this is a bug in 2.0 and should be fixed

I suggest deprecating the type with an error when someone uses it, please no more breaking changes as we follow SemVer

@MikeAlhayek
Copy link
Member Author

the type in already internal so it won't break anyone. It is would break their behavior if they are using API or endpoint on 2.0 that returns ContentItem. Again, this is a bug in 2.0 and should be addressed as a bug.

@hishamco
Copy link
Member

hishamco commented Oct 5, 2024

You're right I though it's a public type

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a breaking change, but still deserves a note in the release notes. I don't understand though how #15460 fixed originally, since it wasn't about web APIs.

@Piedone
Copy link
Member

Piedone commented Oct 7, 2024

How did #15460 fix its issue originally (since it wasn't about web APIs), and how does this not break what it fixed?

@MikeAlhayek MikeAlhayek requested review from gvkries and Piedone October 8, 2024 14:51
@Piedone
Copy link
Member

Piedone commented Oct 8, 2024

So, how did #15460 fix its issue originally (since it wasn't about web APIs), and how does this not break what it fixed?

@MikeAlhayek
Copy link
Member Author

So, how did #15460 fix its issue originally (since it wasn't about web APIs), and how does this not break what it fixed?

I don't think it fixed anything. Look at the comment here #15416 (comment) it sound as he just used a different options.

@Piedone
Copy link
Member

Piedone commented Oct 8, 2024

Ah I see.

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the APIs with https://github.com/Lombiq/Orchard-Core-API-Client and didn't notice any issues.

src/docs/releases/2.1.0.md Outdated Show resolved Hide resolved
src/docs/releases/2.1.0.md Outdated Show resolved Hide resolved
MikeAlhayek and others added 3 commits October 8, 2024 13:12
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft.AspNetCore.Mvc.JsonOptions uses default instead of OC JOptions settings
4 participants