OrchardCMS
IContentManager.CreateAsync() should not publish the content item by default
The arguments sounds good, but this is a very risky change. I'm sure there was a reason for how this was done originally. Was it publishing from its inception? And it's a subtly breaking change that you only may notice during runtime, and as such, should be well documented.
@Piedone seems there's a flaky test. I just re-run the failed jobs and they are passed :)
I agree that we need to document this, also I need to do a few tests
I agree with @Piedone, this is a risky change. Maybe it is better to make the VersionOptions parameter required, i.e. do not allow null anymore and remove the default. That would be a breaking change that is obvious to any caller.
I avoid breaking change, while we document these changes, nothing risky IMHO BTW, we react to these changes, and I check the content management as it works as expected
It's more like behavior change, but I don't think someone is using 'create' for publishing when there's a publish option. However, I don't have any objection. Also, let's hear @sebastienros feedback, and it would be nice if we could triage this tonight
In this we are changing behavior of an existing interface which no one will notice until it breaks at runtime. If we change it (and I think your reasoning makes sense), we should break the build in such cases.
In this we are changing behavior of an existing interface which no one will notice until it breaks at runtime. If we change it (and I think your reasoning makes sense), we should break the build in such cases.
I agree. I think you should modify the IContentManager.CreateAsync by making the second argument required. and document that If you want to created a published version, then pass VersionOptions.Published as the second argument otherwise, we suggest passing VersionOptions.DraftRequired and publish using the PublishAsync api
We should close this PR and improve the documentation instead (at least the API doc).
I might agree with @gvkries & @MikeAlhayek
@sebastienros it's not a matter of the documentation, it doesn't make sense to publish the content while there's a specific API, but it could be done explicitly
After thorough discussion, we have decided to maintain the current behavior of the API. While we acknowledge that the existing design is not ideal from a modern API design perspective, we believe that altering the behavior at this stage would introduce unnecessary risk and limited benefit. Below are the key reasons supporting this decision:
-
Longstanding Design Stability The current API design has remained unchanged since Orchard 1 (O1), providing a consistent and predictable interface for developers over many years.
-
Minimal Demand for Change There have been very few requests or issues raised by the community regarding this method, suggesting that the current behavior is either well-understood or not problematic in practice.
-
Risk of Breaking Changes Modifying the behavior of this method would constitute a breaking change, potentially impacting existing applications that rely on the current implementation. Given the API’s long history, the cost of such a change outweighs the benefits.
-
Improved Documentation as a Mitigation Strategy To address any confusion or limitations in the current design, we propose enhancing the documentation: particularly by adding detailed remarks in the source code. This approach provides clarity without disrupting existing consumers of the API.
If that's the case I will close this PR and open another one by providing a docs in that method