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

fix error condition for custom fields #13148

Merged
merged 3 commits into from Feb 9, 2023

Conversation

kevinsmgov
Copy link
Contributor

@kevinsmgov kevinsmgov commented Jan 26, 2023

#13144
#13136
When a ContentPart only contains custom fields, that GraphQL doesn't know how to use, the ContentPart is excluded.

@@ -53,6 +53,11 @@ public void Build(FieldType contentQuery, ContentTypeDefinition contentTypeDefin
continue;
}

if (!(part.PartDefinition.Fields.Any(field=> contentFieldProviders.Any(fieldProvider=>fieldProvider.GetField(field)!=null))))
Copy link
Member

Choose a reason for hiding this comment

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

This line will be more readable if you use a regular foreach loop. Also, fix the spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really a loop. It's a boolean expression testing that no standard field types are included in the current ContentPart.

Copy link
Member

Choose a reason for hiding this comment

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

well, Any loop over the records. I think the LINQ expression you have is hard to read when looking at the code. You could create a new collection of all valid fields and use the collection later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be duplicating the logic already in the fieldProvider and, IMO, it would make the process a lot more complicated that it is right now.

That being said, if the Orchard Core maintainers think it would improve the project, their welcome to create all that extra logic. This commit fixes the issue so I'm good with it as is.

@sebastienros sebastienros merged commit 9a80117 into OrchardCMS:main Feb 9, 2023
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.

None yet

3 participants