-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Named Part Fields are not indexed #14366
Conversation
This pull request has merge conflicts. Please resolve those before requesting a review. |
@sebastienros Could you please take a look? |
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 sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and this would be quite useful.
I checked this out and it seems to be correct. However, this might have broader implications than I can grasp, so I asked this to be covered during the weekly triage meeting too.
@@ -13,7 +13,7 @@ public static class ContentPartFieldDefinitionExtensions | |||
ContentItem contentItem) | |||
where TField : ContentField | |||
{ | |||
if (((JsonObject)contentItem.Content)[fieldDefinition.PartDefinition.Name] is not JsonObject jPart || | |||
if (((JsonObject)contentItem.Content)[fieldDefinition.ContentTypePartDefinition.Name] is not JsonObject jPart || |
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.
For those wondering: fieldDefinition.PartDefinition.Name
here will always be the technical name of the content part itself, as also listed under /Admin/ContentTypes/ListParts. fieldDefinition.ContentTypePartDefinition.Name
will be the same for non-reusable parts, however, it'll be the instance name for reusable named parts.
@@ -77,7 +77,7 @@ public override void Describe(DescribeContext<ContentItem> context) | |||
ContentItemId = contentItem.ContentItemId, | |||
ContentItemVersionId = contentItem.ContentItemVersionId, | |||
ContentType = contentItem.ContentType, | |||
ContentPart = pair.Definition.PartDefinition.Name, | |||
ContentPart = pair.Definition.ContentTypePartDefinition.Name, |
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.
This is not unique among content types, i.e. you can attach a named part with the name "Example" to more than one content type, even different named parts. I don't think this is an issue, but worth keeping in mind. Please check out if this causes any issues, including with SQL indices (it doesn't seem so; unless you created custom unique indices on just that column).
fixes #9444
@barthamark I faced the same issue, and fixed it in this PR.
/cc @sebastienros or @jtkech