Fix whitespace control #944
Conversation
wouterj
force-pushed
the
pull-931
branch
2 times, most recently
from
5ca2629 to
b13fb39
Compare
|
@jaapio do you think we can backport this? |
|
Just to understand it: is the main branch 2.x now and are we backporting things to 1.x to release a new minor? (cause Composer is configured that main is alias for 1.x as well) Is there anywhere I could learn more about this process? It's the opposite of what I'm used to Symfony world, so I would love to learn more about it. |
|
@wouterj we are using version management similar to what TYPO3 does here. So we are always working on main and breaking changes are allowed on main. Whatever is non-breaking we can backport to 1.x. Releases for new minor or bugfix versions are done from 1.x. One day we hope main will become 2.0. The day that will happen we will branch of 2.0. |
|
I never looked into details of branching as I do not maintain multiple versions, once I release a major version of a project I do normally just move on. I try to do no BC breaks and when I do so, I just move to the next major version. That has worked for me for years. However this library is a bit different. There are multiple people actually contributing which is kinda new for me :-). So I just followed the model as suggested by @linawolf. I guess it's based on the gitflow model. The alias on main is an error from my side, that should be solved. Maybe we should have a look at this because right now mostly every pr we currently make is backported. We automated this, but still we need to think about it. So I'm open for suggestions to change this or other improvements. Let's move this discussion to an issue. I have no issues to merge this to v1 version, as the output format is not part of our BC promise as it is up to me. Backward compatibility for templates is a hard issue. Of course some changes are clearly not backward compatible, but as this PR is just focusing on whitespaces I do not see any issues with that. |
|
@wouterj I enabled the automerge option, so once the tests are green this will be merged automatically and ported to our 1.x branch. |
Head branch was pushed to by a user without write access
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Fixes #922
Some templates used
{% apply spaceless %}. This filter does a very naive preg_replace to remove any space between HTML tags. This is a big issue, as it also removes whitespace between inline HTML tags (as shown by the testcase added in 5ede52d).We must no longer use spaceless in templates, as this creates issues like this. Instead, we must rely on Twig's whitespace control characters:
{{-/{%-/{#-removing all whitespace before/after the Twig tag and{{~/{%~/{#~removing all horizontal whitespace before/after the Twig tag.I also introduced a Twig file loader that removes the last newline feed from files (8790e8e). In unix systems, all files must end with a new line feed. But we don't want all templates to end with a new line feed in the output. That seems to be the main reason spaceless was used in this project. By removing them during loading, we no longer need spaceless. If you want to enforce a new line in the resulting HTML, you can end the file with a Twig comment (as the new line between the last HTML and the Twig comment is not removed).
Furthermore, I improved whitespace control a bit to create nicer output HTML and had to update a LOT of tests (c1452ed). Merging this PR might also result in tests breaking in TYPO3 / Symfony Docs test suites, but this would have been the case also when only removing the use of spaceless.
(quick note: I updated most tests manually, except from the
tests-fulltests as these are very tedious. We might want to double check that all whitespace changes in this file are indeed not resulting in a display change)