OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.23k stars 2.34k forks source link

liquid jsonparse is stricter compared to oc 1.7 #16369

Open bashuss opened 2 weeks ago

bashuss commented 2 weeks ago

Describe the bug

when dealing with liquid arrays or objects in a template, you may be tempted to work with liquid capture, so you can build an object from text.

{% capture myjson %}
{
     "myArray":["{{  myStringVar1 | t }}", "{{  myStringVar2 | t }}"]
}
{% end capture %}

(this is just a rather random example)

When using json_parse to create the object {% assign myobject = myjson | json_parse %} this may fail now, due to more restricted syntax.

e.g. ['text','text2'] is not parsed as array anymore but was before. It needs to be ["text","text2"] to work properly.

It is not the problem, that this is more strict now. The problem is, when this happens during migration from an older version. Some pages will simple display a server error, but it is quite hard to find out, where it is located. I had this problem with two not so obvious, but important pages and it took a google indexing report for me to find out about it and quite some time to find the cause.

Orchard Core version

v2.0.0.-preview I guess the error would also appear with any version, that already has switched to other json library

To Reproduce

Steps to reproduce the behavior:

  1. Go to some liquid template, which is used for display
  2. add this line to the template: <div>{{ "['text1','text2']" | jsonparse | first }}</div> and store it
  3. open a page which uses the template
  4. See error, log, error in debug environment ...

Expected behavior

It should either work like it did with Newtonsoft base jsonparse, or it should create an error, which at least states the name of the template and that the reason was jsonparse (which is already the case, when looking in the stack trace).

sebastienros commented 1 week ago

You could create a custom jsonparse using NewtonSoft and replace the current one, but we won't be able to change this behavior in STJ.

However your suggestion is to add more information about the error. IF the current error doesn't show the template name maybe this could be added in the log, from the component that does the parsing and knows the name of the template (if available), so I will add it as a valid request.

github-actions[bot] commented 1 week ago

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.