OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.42k stars 2.39k forks source link

Remove `VersionOptions.AllVersions` #16027

Closed MarGraz closed 5 months ago

MarGraz commented 5 months ago

For an explanation, see comments below from https://github.com/OrchardCMS/OrchardCore/issues/16027#issuecomment-2115840661. We should remove VersionOptions.AllVersions.

The original issue below is kept for context.

Describe the bug

The method _contentManager.GetAsync(model.ContentItemId, VersionOptions.AllVersions) fails to return any content items, whether published or draft. When debugging in Visual Studio with "Just my code" disabled, execution enters the condition else if (options.IsDraft || options.IsDraftRequired) in DefaultContentManager here.

I'm unsure why this condition is satisfied, given that my content item is published and the boolean IsDraft and IsDraftRequired are set to false.

Screenshots are provided for reference.

To Reproduce

Steps to reproduce the behavior:

  1. Use Orchard Core 1.8.3
  2. Activate the "Content Fields Indexing (SQL)" feature (I have it active)
  3. Create a Content Item and publish it
  4. In a controller, use IContentManager from dependency injection to retrieve the Content Item by its ID with VersionOptions.AllVersions set, like this: _contentManager.GetAsync(model.ContentItemId, VersionOptions.AllVersions)
  5. Execute the code to verify whether a Content Item is returned

Expected behavior

A published or drafted Content Item must be returned when I set VersionOptions.AllVersions

Screenshots

My Content Item:

image

IsDraft and IsDraftRequired are false, why the condition is satisfied?

image

Piedone commented 5 months ago

Isn't this what https://github.com/OrchardCMS/OrchardCore/pull/11433 addresses?

Piedone commented 5 months ago

A note for this and https://github.com/OrchardCMS/OrchardCore/pull/11433 @hyzx86 about AllVersions :

Also, there's no need for something like an AnyVersion or to change this to satisfy the expectations of this bug report, since Latest is already for that.

sebastienros commented 5 months ago

I suggest we remove the AllVersions option since it's not used, and not working, and the expectations wouldn't be fulfilled when you pass multiple content item ids.

If a user wants historical list of version they would use the ContentItemIndex to filter them. We could add a custom method that takes a simple query filter and would use the Load from DefaultContentManager to handle the lifecyle events.

Piedone commented 5 months ago

https://github.com/OrchardCMS/OrchardCore/pull/16077 takes the make it work approach, with throwing in GetAsync(contentItemId, VersionOptions options) and returning all versions in GetAsync(IEnumerable<string> contentItemIds, VersionOptions options).

Any comments?