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

allowContentTypes regression #8928

Closed
rymsha opened this issue Jun 23, 2021 · 3 comments
Closed

allowContentTypes regression #8928

rymsha opened this issue Jun 23, 2021 · 3 comments
Assignees
Labels
Projects
Milestone

Comments

@rymsha
Copy link
Contributor

rymsha commented Jun 23, 2021

Enonic version: 7.7

With 7.7 and Content Studio 3.3 it seems the way regex for "allowContentType" of x-data is changed in a way that breaks current behavior.

These patterns all work fin before we upgraded. After upgrade these x-datas are not added at all. This breaking change is not documented, so I kind of assume this is unexpected behavior for you.

<x-data name="teasers" allowContentTypes="(link|link-application|page-(article|general))" optional="true" />
<x-data name="menu-item" allowContentTypes="(page-article|page-general|link|menu-heading|base:shortcut)" optional="true" />

We tried changing these to this but that also failed:

<x-data name="teasers" allowContentTypes="(com.gjensidige.builders.theme:link|com.gjensidige.builders.theme:link-application|com.gjensidige.builders.theme:page-article|com.gjensidige.builders.theme:page-general)" optional="true" />
<x-data name="menu-item" allowContentTypes="(com.gjensidige.builders.theme:page-article|com.gjensidige.builders.theme:page-general|com.gjensidige.builders.theme:link|com.gjensidige.builders.theme:menu-heading|base:shortcut)" optional="true" />

After some chatting on Slack with Sergei it seems the regex-ness of these allow patterns is only activated when a * is present. This wasn't the case before. And you never needed to type out the name of the app. The docs for this is very sparse so it's hard to know what is correct way of doing this. Examples for ContentSelectors is far more expanded, but those patterns looks to differ from x-data patterns slightly.

Can this be fixed so the old way keeps on working, or can we get another way that works that basically lets us handpick specific content types from current app (but some times other apps) and mix those with XP bundled types like base:shortcut.?

@rymsha rymsha added the Bug label Jun 23, 2021
@rymsha rymsha added this to Needs triage in Bugs via automation Jun 23, 2021
@rymsha rymsha moved this from Needs triage to High priority in Bugs Jun 23, 2021
@rymsha
Copy link
Contributor Author

rymsha commented Jun 24, 2021

Previously pattern matching was using "find" instead of "match", and it was certainly problematic. If you had

<x-data name="feed" allowContentTypes="folder" optional="true" />

it would match any content types from any applications like "app:my-folder", "app:foldertoo", "app:infolderapp" or even "com.my.folder:content"

We don't plan to restore previous behavior completely, but we are going to revisit how we do matching when ​*​ is not part of an expression.

@rymsha
Copy link
Contributor Author

rymsha commented Jun 24, 2021

Fix:

  • ${app} replace anywhere in the string, not just at the beginning. Regex-escape the value.
    It is not exactly how it was prior 7.7 (used to be "if starts with ${app} replace all") but actually retrofits it, and gives better flexibility. So one would be able to write (${app}:a|${app}:b). Note that $ in regexp is "end of string", and practically not used in the middle of expressions, especially with { right after.
  • always use regex match even when there is no * in expression.
    It is also not exactly how it was prior 7.7 (used to be regex "find" leading to unintentional matches). It was certainly a bug.

vbradnitski added a commit that referenced this issue Jun 25, 2021
vbradnitski added a commit that referenced this issue Jun 28, 2021
vbradnitski added a commit that referenced this issue Jun 28, 2021
vbradnitski added a commit that referenced this issue Jun 28, 2021
vbradnitski added a commit that referenced this issue Jun 28, 2021
rymsha added a commit that referenced this issue Jun 29, 2021
rymsha added a commit that referenced this issue Jun 29, 2021
rymsha added a commit that referenced this issue Jun 29, 2021
rymsha pushed a commit that referenced this issue Jun 30, 2021
vbradnitski added a commit that referenced this issue Jun 30, 2021
vbradnitski added a commit that referenced this issue Jun 30, 2021
rymsha pushed a commit that referenced this issue Jun 30, 2021
rymsha pushed a commit that referenced this issue Jun 30, 2021
@rymsha
Copy link
Contributor Author

rymsha commented Jul 1, 2021

Extra fix:
introduce LEGACY mode for "allowContentType inputtypes" a "allowContentTypes xdata" config. in this mode regex "find" is used and ${app} is looked up only at expression start

@rymsha rymsha moved this from High priority to Closed in Bugs Jul 1, 2021
@rymsha rymsha added this to the 7.7.1 milestone Jul 1, 2021
@jsi jsi closed this as completed Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Bugs
Closed
Development

No branches or pull requests

3 participants