fix spaceless deprecation

[garak]

Fix #395

[tacman]

Any chance this can be merged and a new release can be published? Twig 4 will be out soon, better to get ahead of this. Every one of my sites uses this wonderful library, so I see this message all the time.

 c:c --env=prod

 // Clearing the cache for the prod environment with debug false                                                        

[2025-02-06T15:19:57.084445+00:00] deprecation.INFO: User Deprecated: Since twig/twig 3.12: Twig Filter "spaceless" is deprecated in /home/tac/g/sites/member-directory/vendor/knplabs/knp-menu/src/Knp/Menu/Resources/views/knp_menu.html.twig at line 12. {"exception":"[object] (ErrorException(code: 0): User Deprecated: Since twig/twig 3.12: Twig Filter \"spaceless\" is deprecated in /home/tac/g/sites/member-directory/vendor/knplabs/knp-menu/src/Knp/Menu/Resources/views/knp_menu.html.twig at line 12. at /home/tac/g/sites/member-directory/vendor/twig/twig/src/DeprecatedCallableInfo.php:65)"} []
                                                                                                                        
 [OK] Cache for the "prod" environment (debug=false) was successfully cleared.                                          

[garak]

I rebased this PR and updated it, now the CI passes without deprecations.

The question raised by @stof is still open, though. This update would formally allow the library to be used with Twig 4, but the possible problem with the spaceless filter is not solved.

[ro0NL]

maybe im missing it, but why not just remove the deprecated spaceless function from the template here? basically what #400 does.

if tests are the issue, maybe apply spaceless output there to keep them passing/compatible.

[curry684]

maybe im missing it, but why not just remove the deprecated spaceless function from the template here? basically what #400 does.

The "problem" is that this very library is more or less the prototype case of why the spaceless filter was created in the first place - nested unordered lists have of old had the issue that making the HTML readable would often result in stray whitespace causing layout issues. For example:

<ul>
   <li>Aap</li>
   <li>Noot</li>
   <li>Mies</li>
</ul>

Coupled with CSS rule li { display: inline-block; } will render differently with spaceless as it will remove the 5 to 8 pixel gap between elements resulting from the whitespace in between collapsing to a single space in the current font.

In other words: just removing the spaceless from the root template will likely break many, MANY, layouts using this library.

[ro0NL]

😅 ok, i dont want to find out. Then i'd say port the twig spaceless filter as-is here.

[curry684]

Here's a Codepen illustrating the problem: https://codepen.io/niels-keurentjes/pen/ByaKrrQ

[curry684]

Then i'd say port the twig spaceless filter as-is here.

I am tempted to agree as in practice spaceless is a useless filter commonly only used for all the wrong reasons (obfuscation and compression) except for the single exceptional case of nested menu structures. For now I think that grants KnpMenu the 'right' to maintain the legacy implementation until such time as a better replacement is made or we deem it unnecessary to provide the BC.

[wouterj]

The issue with spaceless is not that it is used for "wrong reasons". The problem is it has no knowledge of block or inline nodes, and ignorantly removes all whitespace everywhere between tags.

I wouldn't be surprised if that means that having a bold tag in part of a menu item label will be broken currently.

Wouldn't it be a better option to make it exclusively remove whitespace between <li> elements?

[ro0NL]

solving the deprecation in a BC manner is a 1st concern to me
then if something is currently broken, that's a 2nd concern to me

[curry684]

Exactly, it doesn't matter if right now it's breaking bold tags and everything, because at least now everybody's counting on it to be broken. We just want it to remain reliably broken, without the deprecation, and with forward compatibility with Twig 4, whilst we ponder whether a new major of this library should fix the underlying issue differently.

The currently proposed fix can be shipped as patch release within Semver constraints. Let's do that and open a new central issue discussing the future.

[stof]

For the next major version of that library, I think we should be relying on whitespace control to remove Twig indentation spaces from the output (and this will have no runtime cost as the removal is even done at compile time).
This will impact projects doing custom themes, as they will need to make sure to remove spaces in any place where the removal is important.

[curry684]

Yes I looked into a more fundamental solution today, but indeed that is the only way to go, and I don't see any way to do it in a backwards compatible way.

[ro0NL]

maybe there's a css fix for future major upgrades, so we can stop rely on spacing in output

[stof]

@ro0NL if you rely on display: inline-block, the difference between no spaces and some spaces is relevant by design of how inline flow works (otherwise, words would not have spacing between them). If you use other kind of layout for the menu (display: flex on the ul for instance), they make no difference. But knp-menu does not manage the styling of the menu.

[curry684]

maybe there's a css fix for future major upgrades, so we can stop rely on spacing in output

The CSS fix is essentially a matter of removing the spaceless filter, fixing the built-in templates to use whitespace control tags, slapping a v4 major version number on it, and adding a large bold section to the release notes stating "B/C break: all your template overrides are now broken if you are relying on inline block rendering, which is what older Bootstrap versions have done for years".

We cannot fix for others how they use the library, so we have to treat it as a B/C break for every user. And if we were to do this, I'd recommend updating a ton of other stuff as well.

[stof]

@curry684 I fail to see how this is a CSS fix

[curry684]

It isn't 😆 The implication is that, to fix it in CSS, is to force every library user to revise their local CSS. We cannot fix it here as we explained.