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

Fix modParser so we can output brackets around parsed elements #13904

Merged
merged 12 commits into from
Feb 9, 2019

Conversation

JoshuaLuckers
Copy link
Contributor

What does it do?

Find the position of the last occurrence of prefix in the original content so we can output brackets around parsed elements.

Why is it needed?

This way we can for example use the output as array key in a form input name.

Related issue(s)/PR(s)

#13903

@Jako
Copy link
Collaborator

Jako commented May 17, 2018

Maybe this will break 'Mosquito' tags like [[[[*field:is=`0`:then=`!SomeScript`:else=`$SomeChunk`]]]]
described on https://modx.com/blog/2012/09/14/tags-as-the-result-or-how-conditionals-are-like-mosquitoes/, but I did not check that.

@JoshuaLuckers
Copy link
Contributor Author

I tried the 'Mosquito' tag and it seems to work as expected:

[[[[*id:is=`1`:then=`$TestChunk`:else=`$AnotherChunk`]]]]

Content of TestChunk:

<strong>TestChunk</strong>

Content of AnotherChunk:

<strong>AnotherChunk</strong>

Output:

<strong>TestChunk</strong>

@JoshuaLuckers
Copy link
Contributor Author

The build(s) seem to fail with this change. I will check the failures and try to fix them this weekend.

@JoshuaLuckers
Copy link
Contributor Author

Made some changes and added some additional tests. The builds are passing.

@opengeek opengeek added this to the v2.7.0 milestone May 24, 2018
@opengeek opengeek added area-core proposal Proposal about improvement aka RFC. Need to be discussed before start implementation. labels May 24, 2018
@JoshuaLuckers JoshuaLuckers changed the base branch from 2.6.x to 2.x May 26, 2018 13:46
Copy link
Contributor

@bezumkin bezumkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First it was okay, but then I noticed the error.

Copy link
Contributor

@bezumkin bezumkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the first parsed tag on page.

<title>[[*pagetitle]] - [[++site_name]]</title>

outputs

<title> - MODX Revolution</title>

in default template

@JoshuaLuckers
Copy link
Contributor Author

@bezumkin thanks for noticing. Will try to find the issue later. Did you get an error in the log?

@bezumkin
Copy link
Contributor

bezumkin commented Jun 5, 2018

@JoshuaLuckers Yep

[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name pagetitle.
[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name pagetitle.
[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name pagetitle.
[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name [pagetitle.
[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name [pagetitle.
[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name [pagetitle.
[2018-06-05 12:30:42] (ERROR @ /home/s12854/www/core/model/modx/modparser.class.php : 557) Could not find snippet with name [pagetitle.

@JoshuaLuckers
Copy link
Contributor Author

Found the problem. [1+1] or [something] will break the parser again.

Will try to fix it tomorrow.

@JoshuaLuckers
Copy link
Contributor Author

Solving this issue is more complex than expected. I'm not sure if I can solve it before 2.7.0 is released.

@JoshuaLuckers
Copy link
Contributor Author

JoshuaLuckers commented Jun 8, 2018

@bezumkin I decided to use regular expressions to solve the issue. I do think it needs some more testing.
If anyone is willing to test it, please do so!

@JoshuaLuckers
Copy link
Contributor Author

Tried it with a more complex site and I found some more issues. I will add tests to check for these issues later.

@JoshuaLuckers
Copy link
Contributor Author

Pushed some changes that solves the issues I had. Any more testing would be appreciated.

@JoshuaLuckers JoshuaLuckers force-pushed the issue-13903 branch 2 times, most recently from f541ae5 to 9d4af06 Compare June 23, 2018 11:00
@OptimusCrime
Copy link
Contributor

All tests pass, awesome work!

@opengeek
Copy link
Member

FYI, the site I use to test parser changes is completely broken in 2.x with or without this PR so I'm having a hard time verifying this.

@MrRoco
Copy link
Contributor

MrRoco commented Oct 13, 2018

Tested and working :)

Gives us possibilities with Formalicious :)

@gpsietzema gpsietzema added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Oct 13, 2018
@Mark-H Mark-H added pr/review-needed Pull request requires review and testing. and removed pr/ready-for-merging Pull request reviewed and tested and ready for merging. labels Nov 2, 2018
@opengeek opengeek modified the milestones: v2.7.0, v3.0.0-alpha Nov 27, 2018
@JoshuaLuckers JoshuaLuckers changed the base branch from 2.x to 3.x December 3, 2018 19:24
@opengeek opengeek dismissed bezumkin’s stale review January 24, 2019 16:24

Seems the issue was resolved by the author.

@JoshuaLuckers JoshuaLuckers added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Feb 2, 2019
@JoshuaLuckers JoshuaLuckers merged commit 5982c98 into modxcms:3.x Feb 9, 2019
@JoshuaLuckers JoshuaLuckers deleted the issue-13903 branch March 5, 2019 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core pr/ready-for-merging Pull request reviewed and tested and ready for merging. proposal Proposal about improvement aka RFC. Need to be discussed before start implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants