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

Large file not firing all shortcodes with Regular Parser #77

Closed
rhukster opened this issue Apr 3, 2019 · 4 comments · Fixed by #78
Closed

Large file not firing all shortcodes with Regular Parser #77

rhukster opened this issue Apr 3, 2019 · 4 comments · Fixed by #78
Assignees
Labels

Comments

@rhukster
Copy link

rhukster commented Apr 3, 2019

Per #70, I'm still having this issue. Attached is a zip to help debug:

thuderer-shortcode-issue#77.zip

Just extract, run composer install and then php shortcode.php

The output with each parser can be seen via this online diff: https://www.diffchecker.com/vejLhuQq

Notice, the shortcodes are processed at the bottom of the file for the regular parser, but as things progress upwards, it cuts out.

@thunderer
Copy link
Owner

@rhukster I thought that the file being large was the least probable cause so I ruled it out first by reducing your document to contain only shortcodes without any text - nothing changed. I played a bit with the result until I found the issue: there is a shortcode "syntax error" on line 817 - notice the missing apostrophe in the parameter value: [div class="table]. I "fixed" that typo and the issue disappeared.

I kept reducing the document until I found the smallest example to reproduce the issue: [a][x][/x][x k="v][/x][y]x[/y], namely: open tag with "nested" valid and invalid shortcodes with the same name, followed by another valid shortcode with different name. The real issue is caused by not removing the correctly parsed shortcode name from the list of "open" names before finally returning them to the parent context. Faulty opening tag is processed and correctly rejected, but then the parser continues from the closing tag position and closes something which should not be reported as open in the first place. This breaks the whole loop and reports empty list of shortcodes. That's why you've got only those at the bottom - they appeared just after the faulty one.

I just published #78 with tests and a fix for this issue. Please verify your inputs using dev-issue-77 Packagist version and let me know how it works on your end. Thanks for using the library and all your help in resolving the issues! :)

@rhukster
Copy link
Author

rhukster commented Apr 14, 2019

Hah, well thanks for finding my bug :) Just glad it also managed to shine a light on an issue with the Regular parser at the same time.

I've tested with the dev-issue-77 tag and can confirm that it resolves the issue and only skips the broken shortcode now. Thanks for tracking this down!

I'll release a new version of the Grav shortcode-core plugin as soon as you get a release done. Cheers!

@thunderer
Copy link
Owner

Hi @rhukster, I just tagged v0.7.2 which includes the fix from #78 and improvements from #73.

By the way, I'm wondering, what do you think about the code below:

[a k="v][a k="v]

Currently RegularParser will report one shortcode a with parameter k equal to v][a k= and empty parameter v. I created a POC branch in which the parser requires explicit escaping of opening and closing tags (effectively reporting the shortcode above as invalid). I'd appreciate your input because I don't know which way should I go. Maybe I should make it configurable as in #66 (comment)? Thanks in advance for any help.

@rhukster
Copy link
Author

I think it makes more sense for both of those shortcodes to be invalid. There's really no possible reason that a missing double quote should be considered OK. And having k equal to v][a k= would surely never be expected behavior?

BTW, thanks for the 0.7.2 release.. my plugin is updated with it also. Cheer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants