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

[TwigComponent] Fix TwigPreLexer edge case #1312

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

smnandre
Copy link
Member

Q A
Bug fix? yes
New feature? no
Issues Fix #1306
License MIT

Quick fix to handle commented blocks in sublexer

@weaverryan @WebMamba it may need a second look as i find weird the changes made do not have any other impacts...

@WebMamba
Copy link
Contributor

WebMamba commented Nov 30, 2023

Hum, I think if we have a problem with comments we also have an issue with the verbatim tag? And I think an easier solution could be if the pointer consumes {# then moves the pointer position to the next #} as we do at the top. With this method, you can remove all your condition

@smnandre
Copy link
Member Author

I just wanted to fix the specific issue there.. and i guess a lot more people uses block in comments (in a dev phase it is a frequent thing) than verbatim :)

But it's an issue indeed.

I do believe we can fix and improve some things here... but i'd suggest to do it a bit differently and that seems to me another (bigger) discussion.

WDYT ?
We can open an issue if you want to discuss this next move ?

@smnandre
Copy link
Member Author

Hum, I think if we have a problem with comments we also have an issue with the verbatim tag? And I think an easier solution could be if the pointer consumes {# then moves the pointer position to the next #} as we do at the top. With this method, you can remove all your condition

Sadly not if we want to handle properly verbatim + comments + strings + ....

@weaverryan weaverryan force-pushed the fix/pre-lexer-comment branch from 0c58bed to 5c0a30f Compare December 12, 2023 16:46
@weaverryan
Copy link
Member

This moves things a small step forward - thanks Simon!

@weaverryan weaverryan merged commit 8fe391d into symfony:2.x Dec 12, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint:twig error, possibly with twig component
3 participants