Placement rules for shape types that include a double underscore are applied incorrectly

[mroskamp]

Describe the bug

This came up while I was investigating #10958. I discovered that the BaseDisplayManager ignores everything after the __ (double underscores) delimiter. Orchard defines several shapes that include this delimiter (e.g. CommonPart_Edit__Date, CommonPart_Edit__Owner).

If, for example, I wanted move the Owner field above the date field, I'd expect to be able to write the following placement rules:

{
  "CommonPart_Edit__Date": [
    {
      "place": "Content:20"
    }
  ]
  "CommonPart_Edit__Owner": [
    {
      "place": "Content:10"
    }
  ]
}

However, if I do this, the shape type for both rules gets evaluated as CommonPart_Edit, so both shapes end up being in the content zone in position 10.

Note that creating placements via the admin placements feature would likely exhibit the same issue, if not blocked by #10958.

Orchard Core version

commit 2d49b61

To Reproduce

Placement File

1. Create a new Orchard Core site using the Blog recipe
2. Add the Common part to the Article content type
3. Edit the Common part setting to show the owner and date fields
4. Edit the Article content item created by the recipe. Notice that the date field comes before the owner field.
5. Create a new placement file with the above contents to your site
6. Restart the site
7. Edit the Article content item created by the recipe. Notice that the order of the date and owner fields is unchanged.
8. Edit the placement file, modifying the "CommonPart_Edit__Owner" entry to suppress the shape entirely (i.e. "place": "-").
9. Restart the site
10. Edit the Article content item created by the recipe. Notice that both the date and owner fields are suppressed.
11. Edit the placement file, modifying the "CommonPart_Edit__Owner" entry to move it to the top of the content zone (i.e. "place": "Content:before").
12. Restart the site
13. Edit the Article content item created by the recipe. Notice that both the date and owner fields are moved to the top.

Expected behavior

Shapes should be able to controlled independently using placement rules.

While I believe the behavior of stripping out everything after the delimiter is intentional, per #4484, it's problematic when a shape includes the double underscores and it's not delimiting the display mode modifier.

Note that, in my example, Orchard Core does set a differentiator, so an omniscient developer could strip everything after the delimiter in the placement and use that as the shape type and, instead, specify the differentiator, but that's not really documented and it I'm not sure that that's the intended behavior. This is documented specifically for field display modes per #17333, but this scenario is different in that we're not using display modes.

A developer using Orchard Core could run into this scenario by creating a new driver that returns a combined result with shapes Foo__A and Foo__B. Then, I don't believe the differentiator would be available when resolving the placement, and thus the developer could not use placement on those shapes independently.

Similarly, say we wanted to refactor SearchController.Search to, rather than using adhoc shapes, use the display manager to build the page using the same Search__Form, Search__List, and Search__Results search results page. Those shape names all include the delimiter so we would be unable to use placement to individually control the placement of those shapes.

Should the __ delimiter in a shape type be reserved strictly for use by display modes?

Logs and screenshots

[sebastienros]

What if this was not a bug. The placement file is indexed on shape types, so CommonPart_Edit being what it seeks in the dictionary, this might just be correct, and the placement only needs the differentiator to select the correct entry for this type.

The base logic to strip the __ from the shape descriptor is expected. I'd say the original problem comes from the choice of shape descriptors for these owner and date sections.

But I might be wrong, do you think the other issues prove that it's not the case?

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[mroskamp]

@sebastienros, yes, I agree that the code indicates that the base logic to strip the __ from the shape descriptor is expected and intentional. Perhaps this is primarily a case where the docs can be improved to warn of this behavior, telling developers to avoid using __ in their shape types. Additionally, as you said, the owner and date sections could be changed to address the bug to allow placement of these sections.

Now, would it be fair to say, more generally, that shape types shouldn't include __ at all? Would it be reasonable to add some logging to warn developers when a shape type is encountered that does include __?

Would is also be reasonable to proactively rename the Search__Form, Search__List, and Search__Results shapes to avoid the __? I ran into a scenario in my code where I was reusing these shapes and displaying them via a driver. Then, in some cases, I wanted to rearrange the sections via placement, but that wasn't possible. I can work around this by wrapping those shapes with my own, which would then allow placement, but I wonder if the base search shapes are just additional examples of poorly named shaped.

[sebastienros]

Before I answer, can you confirm that the discriminator would work without you needing to wrap the shapes?

[mroskamp]

Knowing that the __ gets stripped off, I can add the following placement rule and all the Search__* shapes are suppressed.

{
  "Search": [
    {
      "place": "-"
    }
  ]
}

Using the Search__Results shape as an example, when I wired it up in my driver, I wasn't setting a differentiator. I can confirm that, if I do explicitly set a differentiator in my driver, then I can add the following placement and only the search results are suppressed:

{
  "Search": [
    {
      "place": "-",
      "differentiator": "Search__Results"
    }
  ]
}