[StimulusBundle] Improve StimulusAttributes rendering performances by switching to html escaping strategy

[Kocal]

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

A better alternative to #2178, by changing the escaping strategy for HTML attributes value from html_attr to html, as indicated by @stof in twigphp/Twig#4322 (comment).

On my use case (a Map with 1000 Marker and InfoWindow), I have a performance gain of ~68%:

This PR should also improve performances of our packages using the StimulusAttributes DTO, like Chart.js, LiveComponent, ...

---

Important

The initial PR changed a bit, the default rendering strategy does not change, but instead we introduce a new configuration to use the new (and optimized) rendering strategy, see #2180 (comment).

[smnandre]

Do we still need to call the Runtime then ? Would this not be sufficient ?

 return htmlspecialchars($string, \ENT_QUOTES | \ENT_SUBSTITUTE, $charset);

[Kocal]

Do we still need to call the Runtime then ? Would this not be sufficient ?

 return htmlspecialchars($string, \ENT_QUOTES | \ENT_SUBSTITUTE, $charset);

If we assume UTF-8 is used everywhere, yes, but |escape('html') does more than that :/

[smnandre]

This bundle is the most used / downloaded. So we cannot release a version that will break tests (and maybe code) during a minor...

so we need to find a way out here... what about thinking a new object ? less "stimulus" and more "attributes" ?

[WebMamba]

Hey! I am agree with @smnandre here! It's a BC break for me. Maybe we can have a config that allow to switch from html_attr to html ?

[smnandre]

Sadly, it's difficult, as this class is directly used by other components / bundle.. and it's neither final or internal :|

[Kocal]

It's a BC for tests/assertions yes, but I don't really see how and where it can impact end users.

However, we can't stay ad vitam aeternam with html_attr escaping for HTML attribute values, as we all see it is not performant at all.

Since StimulusAttributes is not final nor internal, maybe... we can configure the escaping format through a new method?

[kbond]

New bundle config? Deprecate not setting it to the new, desired value?

[smnandre]

New bundle config ?

We cannot here, this class may be instanciated directly in many projects, as it is not a service.

Deprecate not setting it to the new, desired value?

This is a good idea. Probably we can add an argument either in the render / toString methods, or maybe more in the constructor..

.. and deprecate no passing it (stating in 3.x it will use the new mode)

?

[smnandre]

Regarding performances, i'm very very happy you found this bottleneck and have a clean method to improve things.

But as always, we cannot "force it" down, and it's been written everywhere Symfony UX would follow Symfony BC promise.

To be clear, i'm not saying we cannot use these optimisations in the ux components

• ux map is still experimental, and this is a very legit case of accepted change
• we did not made any promise/engagement on the attributes generated by live component, and it could 100% be considered internal .. this also work for chartsjs, etc (but should we release a typed bundle version just for this ? i'm not entirely sure here..)

I'm concerned about two things here:

• the StimulusAttributes class in itself, that can be used elsewhere / tested / etc ... this would be a major BC break, as simple unit test cases would break
• the StimulusBundle twig functions (stimulus_xxx), that could be configured by the stimulus bundle but i'm not sure this is worth it

[kbond]

This is a good idea. Probably we can add an argument either in the render / toString methods, or maybe more in the constructor..

.. and deprecate no passing it (stating in 3.x it will use the new mode)

Constructor then, yes.

[Kocal]

I'm reverting to html_attr escaping strategy by default, then I will add whatever needed to re-use html for escaping html attribute values (without BC breaks)

[Kocal]

I've updated the PR and introduced a new config stimulus.use_optimized_attributes_rendering (default to null, which allows me to trigger a deprecation if not explicitly specified):

Note: I'll gladly take any comments for my deprecation messages... 😅

This configuration is injected to the StimulusHelper constructor, then injected to the StimulusAttributes constructor.

I've rollbacked tests added in my first commit as it was considered as breaking changes. Instead, I've configured stimulus.use_optimized_attributes_rendering/$useOptimizedRendering to false in order to get rid of deprecation notices (which make tests fails and CI red). I also adapted actual tests to test the new optimized rendering, this way we can clearly see the difference between the actual and new rendering behaviour.

With all those changes, I took a new Blackfire profile on my page with a Map and 1000 Marker/InfoWindow, and did a new comparaison:

[javiereguiluz]

@Kocal impressive performance improvement 🤯 thanks a lot 🙇

I still think this is not a true BC break and we shouldn't complicate things with the new config option.

[nicolas-grekas]

I agree with Javier, no need for the option.
We might reconsider if we get enough reports but without, I wouldn't anticipate this aspect.

[stof]

I'm still not convinced this should be considered a breaking change. The generated HTML string will be different, but this will not change anything for the output of parsing it.

[Kocal]

I agree with all of you, I don't consider it a breaking change either.
The behavior won't change in the browser, users won't be (badly) impacted by this modification.

Let's revert the 2 last commit then, just in case. We will squash the PR at merging.

[Kocal]

The PR is now back at its original state, there is no stimulus.use_optimized_attributes_rendering config anymore.

[nicolas-grekas]

thanks

[javiereguiluz]

Thank you Hugo.