Skip to content

Conversation

@krpisz
Copy link

@krpisz krpisz commented Apr 15, 2025

This PR adjusts {*} comment parsing behavior to resolve inconsistencies with IDE syntax highlighting (e.g., PhpStorm, VS Code) and to better support projects upgrading from Smarty v2.

Why:

  • In legacy Smarty v2, {*} comments required *} to close, which aligns with modern IDE behaviors.
  • Current behavior treats {*} as both the start and end of a comment, which causes confusion when using IDEs or updating old templates.

Behavior Before and After

For the following code:

{*} comment 1 {*}

Before:
Outputs: comment 1

After:
Outputs: (nothing, comment is ignored as expected)

This fixes parsing inconsistencies, aligns with IDE syntax highlighting, and retains expected behavior for old templates written in Smarty v2.

Screenshots

Screenshots of IDE comment highlighting

PhpStorm

002n

VS Code

002o

…ng and legacy behavior

- Adjusted TemplateLexer to ensure `{*}` properly opens comments and ends only on valid `*}`
- Improved consistency with IDEs like PhpStorm and VS Code
- Simplified upgrades for projects migrating from Smarty v2 by preserving familiar comment handling
- Added new test case
@wisskid
Copy link
Member

wisskid commented Apr 15, 2025

I'm quite sure neither {*} comment 1 *} nor {*} comment 2 {*} was ever supposed to be interpreted as a comment in Smarty v2. The docs don't mention it either.

Plus, if I understand this correctly: if we change this now, we break compatibility with Smarty v3, v4 and v5. (v3 has been out since 2010, so 15 years ago.) That seems worse than leaving it as it is.

I do agree it's a bummer the IDEs format this incorrectly, but shouldn't the IDEs plugins be fixed instead?

@krpisz
Copy link
Author

krpisz commented Apr 16, 2025

Thanks for the feedback. I understand the hesitation around changing something that’s been the same since v3.

The goal here is to make Smarty’s comment syntax act exactly as the docs say: comments are “surrounded by asterisks,” meaning {* ... *}. The current situation, where {*} alone is accepted as an empty comment, isn’t part of that definition and doesn’t really have a practical use.

With this PR, {* will always start a comment block that needs a matching *} to close it, just like the docs describe. Standard comment blocks like {* some comment *} aren’t affected, they keep working as expected.

If a template has a standalone {*}, this change will either cause a missing-closing-tag error or comment out code until the next *}. In reality, though, most people using an IDE wouldn’t use {*} as simple “decoration” - IDEs immediately treat the rest of the file as a comment after a lone {*}, breaking syntax highlighting and code completion. So this pattern is unlikely to show up anywhere intentionally.

Overall, this update brings Smarty’s behavior in line with its documentation and what developers (and their tools) expect, making things clearer and more predictable all around.

@wisskid
Copy link
Member

wisskid commented Apr 16, 2025

The current situation, where {*} alone is accepted as an empty comment, isn’t part of that definition and doesn’t really have a practical use.

I totally agree

So this pattern is unlikely to show up anywhere intentionally.

Also agree.

Overall, this update brings Smarty’s behavior in line with its documentation and what developers (and their tools) expect

I'm not sure about that. I would have expected {*} comment 1 {*} to either trigger an error or to print "comment 1" and disregard the empty tags. I'll ask some colleagues what they think.

@krpisz
Copy link
Author

krpisz commented Apr 17, 2025

Here’s how {*} shows up as a comment start in a few different editors - this matches the “surrounded by asterisks” wording in the docs, and seems to be how most tools handle it in practice.

GitHub (github-linguist/linguist)

{**}              CONTENT 1
{*  normal  1  *} CONTENT 2
{*} pincers 1  *} CONTENT 3
{*} pincers 2 {*} CONTENT 4
{*  normal  2  *} CONTENT 5

NetBeans 25
002p

vim (shadowwa/smarty.vim)
002q

Sublime Text
002r

@Visualq
Copy link
Contributor

Visualq commented Apr 17, 2025

I'd vote for consistency and let {*} generate an error, {* ... *} should be the way to go. This however raises the question on how {} should be parsed, right now it simply prints {}, if you put anything between the curly braces it's ofcourse interpreted and the curly braces are omitted. If {} is parsed without {literal}{/literal} should it be displayed at all?

Perhaps the latter is a discussing for a different thread.

@wisskid wisskid added the major requires new major label Apr 17, 2025
@wisskid
Copy link
Member

wisskid commented Apr 17, 2025

My colleagues were split on what they expected what would happen with {*} hello {*}. Some expected to see nothing, some expected to see ' hello ' and one even expected to see the full string {*} hello {*} as plain text. Nobody expected an error, by the way.

I'm not sure how to proceed. One of my main goals with Smarty is to maintain stability and backwards compatibility. Changing the syntax would require a new major release. I don't think the problem at hand is so big that it warrants such a big step.

I'd rather help fix the IDEs as that surely will not have any negative side effects for anyone.

@krpisz
Copy link
Author

krpisz commented Apr 17, 2025

Thanks for considering all sides of this. I get that a major version sounds like the “proper” route, but I wanted to lay out what actually happens if the quirk stays for 3/4/5:

  • If tool maintainers fully adopt the {*} quirk for 3–5, it basically locks in {*} as an official feature, making a fix in v6 very unlikely.
  • If it’s only partly adopted, users will get inconsistent highlighting and comment handling across editors.
  • If it isn’t adopted at all, comments in v3 - v5 will highlight incorrectly until those projects move to v6.
  • If v6 eventually does fix the quirk, then tools will have to support two diverging comment syntaxes - one just for 3/4/5, and one for 2/6

By confirming this as a bug and aligning the behavior for 3–5, tools and editors can support a single, predictable approach, and developers won’t get surprised when code that’s highlighted as a comment unexpectedly shows up in output. Long-term, this seems less disruptive than deferring the issue and ending up with even greater fragmentation.

Thanks again for hearing me out and weighing the options with the bigger picture in mind.

@wisskid
Copy link
Member

wisskid commented Apr 18, 2025

If tool maintainers fully adopt the {} quirk for 3–5, it basically locks in {} as an official feature, making a fix in v6 very unlikely.

I'd say it already is a official feature, since it has been implemented like this for over 15 years. One could say the way things were in v2 was a quirk, because Smarty didn't have a real parser/lexer back then and relied on regular expressions to do the matching. Thus it follows that changing the behavior requires a new major version number.

If it’s only partly adopted, users will get inconsistent highlighting and comment handling across editors.
If it isn’t adopted at all, comments in v3 - v5 will highlight incorrectly until those projects move to v6.

I (again) propose fixing the IDE plugins. I've found a textmate bundle for Smarty that seems rather outdated.

@krpisz
Copy link
Author

krpisz commented Apr 18, 2025

Understood, and thanks for the thoughtful back-and-forth.

As a last idea: would a config toggle for comment parsing be possible? That would let people choose the behavior that fits their workflow, and everyone else keeps the status quo.

If that’s not practical, I’ll just stick with my patch locally. Appreciate your time and all the work you put into Smarty!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major requires new major

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants