-
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
Wrong placement is applied #15174
Wrong placement is applied #15174
Conversation
src/OrchardCore/OrchardCore.DisplayManagement/Descriptors/PlacementInfoExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.DisplayManagement/Descriptors/PlacementInfoExtensions.cs
Outdated
Show resolved
Hide resolved
combined.ShapeType = string.IsNullOrEmpty(second.ShapeType) ? first.ShapeType : second.ShapeType; | ||
combined.Location = string.IsNullOrEmpty(second.Location) ? first.Location : second.Location; | ||
combined.DefaultPosition = string.IsNullOrEmpty(second.DefaultPosition) ? first.DefaultPosition : second.DefaultPosition; | ||
combined.Source = $"{first.Source},{second.Source}"; |
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.
placements from placement.json have null for Source property, so this will results with: ",OrchardCore.Plament" string produced when combined with placements from AdminUI, I would add some additional condition to prevent that
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.
Would it be interesting to see where the placement.json is located? As it is now it's possible to see that it's configured in any placement.json file if there is a leading ",". If there is no "," it's only defined in code.
Example: "OrchardCore.Contents,OrchardCore.Placements" or "TheAdmin,OrchardCore.Placements".
In that case I would prefer to do that in a separate Issue/PR.
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.
Would it be interesting to see where the placement.json is located?
in my opinion it might be useful for debugging purposes
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've just tested @microposmp changes and now everything works as expected.
@MikeAlhayek can you merge this PR?
Fixes: #15166
Changed to not update any in parameters. That caused the resolved value to be cached and returned for other content types and after placement definition overrides were deleted.