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.

Sign up for GitHub

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

Jump to bottom

Removing end of lines #555

Closed
christiandavilakoobin opened this issue Nov 18, 2019 · 8 comments
Closed

Removing end of lines #555

christiandavilakoobin opened this issue Nov 18, 2019 · 8 comments

Comments

@christiandavilakoobin
Copy link

If there is any "?>" in the template file, smarty is removing the end of line after the closing tag, even when isn't their own. i.e:

<?xml version="1.0" encoding="UTF-8"?>
<root>
<child></child>
</root>

is changed to:

<?xml version="1.0" encoding="UTF-8"?><root>
<child></child>
</root>
@wisskid wisskid added the waiting Waiting for answer label Feb 19, 2020
@wisskid
Copy link
Contributor

wisskid commented Feb 19, 2020

Cannot reproduce. My output has <root> on a new line. Maybe this is fixed? Could you retry and provide additional reproduction steps?

@christiandavilakoobin
Copy link
Author

Hi,

Sorry for the delay. In the last version the example I gave you works correctly, but this one fails:

<html>
<body>
<script>

function ei()
{
	// {* Say Hello *}
	alert("Hello world");
}
</script>
<body onload="ei()">
</body>
</html>

After smarty, we have this:

<html>
<body>
<script>

function ei()
{
	// 	alert("Hello world");
}
</script>
<body onload="ei()">
</body>
</html>

In this case, the comment AND the end of line are removed, which causes that next line goes up, and in this example, comments the alert.

The source code is:

<?php

include("libs/Smarty.class.php");

$smarty = new Smarty();

$smarty->display('test.html');

@arvislacis
Copy link

arvislacis commented Jun 17, 2020 •
edited
Loading

You should probably escape any JavaScript code in your Smarty templates with {literal}{/literal} tags, for example:

<html>
<body>
{literal}
<script>

function ei()
{
	// {/literal}{* Say Hello *}{literal}
	alert("Hello world");
}
</script>
{/literal}
<body onload="ei()">
</body>
</html>

So the JavaScript and Smarty template code doesn't get mixed up and doesn't have unexpected behaviors.

I guess that removing end of lines is desired behavior of Smarty compiler so if your template has a lot of comment lines then the final compiled output would not contain just empty lines.

@christiandavilakoobin
Copy link
Author

I agree, but the behaviour is different than in previus smarty versions, and potentially backwards incompatible, like in this case. I found it because some script started to fail in one of our pages. Now we have to check all templates, and check for any tag like this. And I'm not sure this doesn't happen with other smarty functions, besides {**}. This is not acceptable in a minor version, which everyone expects fully compatible.

Closed
@christiandavilakoobin
Copy link
Author

I did a PR fixing this. With this change, the output generated with smarty is exactly the same than in older versions, and the error I reported doesn't happen anymore. You've the last word about accepting it, but I think is important that minor versions maintain backwards compatibility and doesn't broke anything.

@wisskid
Copy link
Contributor

wisskid commented Jun 21, 2020

@christiandavilakoobin thank you for your effort. The \n that gets removed with a comment is a nuisance, but it seems to be there for backwards compatibility in the first place.
What was the previous version you were running on when things where still fine?

@christiandavilakoobin
Copy link
Author

@wisskid We were running Smarty 3.1.29.

@wisskid wisskid removed the waiting Waiting for answer label Feb 7, 2021
@wisskid
Copy link
Contributor

wisskid commented Jul 16, 2022

@christiandavilakoobin I completely agree with you on minor version maintaining BC. However, 3.1.29 is from 2015. The change may have been introduced in 2017 or 2018. If we change it back, we'll break compatibility too. I think it's for the best to just keep it as it is now.

@wisskid wisskid closed this as completed Jul 16, 2022
@wisskid wisskid mentioned this issue Nov 22, 2022
Closed
@wisskid wisskid mentioned this issue Feb 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@arvislacis @wisskid @christiandavilakoobin