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

Remove VersionOptions.AllVersions #16027

Closed
MarGraz opened this issue May 10, 2024 · 4 comments · Fixed by #16077
Closed

Remove VersionOptions.AllVersions #16027

MarGraz opened this issue May 10, 2024 · 4 comments · Fixed by #16077
Labels
Milestone

Comments

@MarGraz
Copy link
Contributor

MarGraz commented May 10, 2024

For an explanation, see comments below from #16027 (comment). 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

@MarGraz MarGraz changed the title "_contentManager.GetAsync(contentItemId, VersionOptions.AllVersions)" fails to return any content items, whether published or draft _contentManager.GetAsync(contentItemId, VersionOptions.AllVersions) fails to return any content items, whether published or draft May 10, 2024
@Piedone
Copy link
Member

Piedone commented May 12, 2024

Isn't this what #11433 addresses?

@Piedone
Copy link
Member

Piedone commented May 16, 2024

A note for this and #11433 @hyzx86 about AllVersions :

  • It's most possibly meant to let you fetch all past and the single current version of a content item.
  • It isn't actually doing anything now, it's not used. But we can make it work, or remove it.
  • It doesn't actually make sense for GetAsync(contentItemId, VersionOptions options): that method will always return at most a single content item, so it can't return all versions of a content item. That method should throw an exception with this input.
  • It does make sense for GetAsync(IEnumerable<string> contentItemIds, VersionOptions options) though, since it returns multiple items anyway. However, this might get confusing there, when you get back N ContentItems, but those are mixed as versions and content items.

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.

@Piedone Piedone changed the title _contentManager.GetAsync(contentItemId, VersionOptions.AllVersions) fails to return any content items, whether published or draft Remove VersionOptions.AllVersions May 16, 2024
@sebastienros
Copy link
Member

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.

@sebastienros sebastienros added this to the 2.x milestone May 16, 2024
MikeAlhayek added a commit that referenced this issue May 17, 2024
@Piedone
Copy link
Member

Piedone commented May 17, 2024

#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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants