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

At sign (@) inside chunk parameter value nested into tag output filter breaks tag parsing #16318

Closed
juro opened this issue Dec 2, 2022 · 21 comments · May be fixed by #16322
Closed

At sign (@) inside chunk parameter value nested into tag output filter breaks tag parsing #16318

juro opened this issue Dec 2, 2022 · 21 comments · May be fixed by #16322
Labels
bug The issue in the code or project, which should be addressed.

Comments

@juro
Copy link

juro commented Dec 2, 2022

Consider this content:

[[+xyz:empty=`
aaa
[[xxx? &x=`bbb@ccc`]]
ddd
`]]
eee

Supposed output should be:

aaa xxxSnippetOutput ddd eee

Instead it is:

aaa [[xxx? &x=`bbb eee

This works fine as expected:

[[+xyz:empty=`
aaa
[[xxx? &x=`bbbccc`]]
ddd
`]]
eee

At sign (@) inside parameter value breaks parsing, but only when nested inside output filter parameter value. Workaround here is to put inner tag inside separate chunk.
With some more complicated scenarios this even causes Server error with End of script output before headers: index.php in error log.
Clean MODx 3.0.2 install, the same with older 2.7.3

@juro juro added the bug The issue in the code or project, which should be addressed. label Dec 2, 2022
@juro juro changed the title At sign (@) inside chunk parameter value nested into other chunk breaks tag parsing At sign (@) inside chunk parameter value nested into tag output filter breaks tag parsing Dec 2, 2022
@halftrainedharry
Copy link
Contributor

I can't reproduce the issue with the provided example content.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 3, 2022

I can not reproduce this either (on a local install of 3.0.3-dev). If there is in fact a reproducible bug, you're going to need to provide detail on your actual code, not just the pseudo code shown above.

@juro
Copy link
Author

juro commented Dec 3, 2022

Thanks guys for your testing. The code above is not a pseudocode, but actual code, the simplest version that demostrates problem. Snippet xxx don't have to even exist, placeholder xyz never gets replaced with value. I put this code into new resource with no template to avoid any other factors. The same behaviour on fresh 3.0.2 and older 2.7.3.
Originally we had problem with combination of Formit (that have email adresses as parameter values) and ContentBlocks (that wraps content with conditional output defined in Layout). So at first I blame one of them, but then stripped it down to the code used above. No specific snippets, no plugins, no template.
I will investigate further, the only thing that comes to my mind is PHP version. We are on 7.2.

@halftrainedharry
Copy link
Contributor

Snippet xxx don't have to even exist

OK, I can reproduce it now (MODX 3.0.2, PHP 7.4) but only if the snippet "xxx" doesn't exist.
If the snippet "xxx" exists, everything works correctly.

@juro
Copy link
Author

juro commented Dec 3, 2022

Oh, at least we are moving forward. I can confirm that when snippet exist, it works correctly. I still considers this to be core MODx bug. Probably it wouldn't impose problem on most sites, but have some implications in more complicated scenarios, like ContentBlocks.
When saving resource with CB, output modifiers on placeholders are parsed, but snippets are not. It is somehow similar to non-existent snippet (because at that moment it is not parsed) inside parsed output modifier.
Other workaround could be to replace output modifier with snippet call, unfortunately this is only possible with user modifiers, not build-in.

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 3, 2022

Think I have a fix. Will report back in a bit.

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 3, 2022

See potential fix at #16322 - please test and report back. That PR is surfacing a potentially different problem related to property sets that I haven't figured out yet though.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 3, 2022

Just curious — is it necessary to support the ability to call a non-existent snippet? Or put differently, what's the use-case for being able to do so?

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 3, 2022

The use case is not breaking the parsing if a snippet doesn't exist.

@smg6511
Copy link
Collaborator

smg6511 commented Dec 3, 2022

Fair enough ;-)

Is there any/sufficient error reporting when a non-existent snippet is called? I know this would be a different issue if the answer were "no."

@juro
Copy link
Author

juro commented Dec 3, 2022

Agree with Mark, parsing should be done right even if snippet doesn't exist. On very complex sites we dynamically build chunk and snippet names, e.g. from context settings. So, in some context we call a different snippet than in others.
For example:
[[mysnippet_[[++contextspecificsuffix]]]]

In this case sometimes no snippet should be called and of course should not break page.
Will try Mark's fix later today

@smg6511
Copy link
Collaborator

smg6511 commented Dec 3, 2022

Not disagreeing, just wanted to understand the motivations. I too have set up dynamic tags like that of your example, but have never done so where the resulting tag calls a snippet that might not exist. I've more so used that strategy with chunks that pull up different variations of a block of code depending on a certain context/condition.

@Mark-H
Copy link
Collaborator

Mark-H commented Dec 3, 2022

The problem may've also been reproducible with an uncached valid snippet in the modifier of a cached placeholder.

@juro
Copy link
Author

juro commented Dec 3, 2022

On my test caching of snippet and/or placeholder doesn't change the result.
I looked into Mark's fix, but cannot test it, it's too complicated for me, sorry. But from description it seems it is related to property sets. But how? Property sets have completely different syntax than just using @ somewhere ínside tag parameter. Parameter value shouldn't be parsed at all, unless there are double square brackets in it. I am wandering why is this parsing problem specific to snippets inside filters and not for example snippets inside other snippets?

@opengeek
Copy link
Member

opengeek commented Dec 3, 2022

On my test caching of snippet and/or placeholder doesn't change the result. I looked into Mark's fix, but cannot test it, it's too complicated for me, sorry. But from description it seems it is related to property sets. But how? Property sets have completely different syntax than just using @ somewhere ínside tag parameter. Parameter value shouldn't be parsed at all, unless there are double square brackets in it. I am wandering why is this parsing problem specific to snippets inside filters and not for example snippets inside other snippets?

Output filters are parsed using a completely different approach than the rest of the parser. It was bolted onto the parser after the original design to incorporate features from a popular extra into the core.

@juro
Copy link
Author

juro commented Dec 4, 2022

Btw, we have found another, similar problem, this time related only to MODx 3.x.
Special timing tags inside output filter breaks parsing as well.
On all projects we use small front-end toolbar, visible only to admin (placed on to of everything with css):

[[!+modx.user.id:is=`1`:then=`
<div id="toolbar">
Page ID: <strong>[[*id]]</strong><br />
MySQL: [^qt^]<br />
[^q^] req.<br />
PHP: [^p^]<br />
total: [^t^]<br />
from [^s^]
</div>
`]]

This works fine in 2.7 version.

@halftrainedharry
Copy link
Contributor

Special timing tags inside output filter breaks parsing as well.

The pull request #16302 probably fixes this problem.

@juro
Copy link
Author

juro commented Dec 5, 2022

See potential fix at #16322 - please test and report back. That PR is surfacing a potentially different problem related to property sets that I haven't figured out yet though.

I have sucessfully tested this PR, works well in stand-alone test as well in ContentBlocks implementation

@juro
Copy link
Author

juro commented Dec 5, 2022

The pull request #16302 probably fixes this problem.

Yes, this PR seems to fix it

@smg6511
Copy link
Collaborator

smg6511 commented Jul 11, 2023

@juro - Can this issue be closed (i.e., does #16302 take care of the problems raised here)?

@juro
Copy link
Author

juro commented Jul 12, 2023

@juro - Can this issue be closed (i.e., does #16302 take care of the problems raised here)?

Yes, on our test the fix is working, issue can be closed.

@juro juro closed this as completed Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue in the code or project, which should be addressed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants