Symfony Twig Component

[froozeify]

Checklist before requesting a review

Please delete options that are not relevant.

• I have read the CONTRIBUTING document.
• I have performed a self-review of my code.
• I have added tests that prove my fix is effective or that my feature works.
• This change requires a documentation update. Done in Symfony component docdev#201

Description

This PR add the support of Twig Component with autowiring support (another branch is created simpler but use flagged @internal TwigComponents - Diff between this PR and the one without autowire froozeify#6)

Documentation

Roadmap

[cconard96]

I didn't think this was a good idea when I started hearing interest in using it for GLPI back in November 2024, and I still don't. My opinion of live components is even worse.

They simply don't make sense for a real, complex, non-public web application to me and it isn't clear from the roadmap entry just how far this is going to go. Certainly the plan isn't to add all of this only to render alerts?

I don't see what this accomplishes besides adding more layers of abstraction and ✨ magic ✨. Maybe you could say it adds encapsulation, but it doesn't seem any better than what the original macros did.

Alerts are so trivial that this doesn't show anyone how well or poorly this kind of solution would scale with anything but the simplest of components.

Testing a Twig template with:

{% for i in 1..100 %}
<twig:Alert:Danger>Danger alert</twig:Alert:Danger>
{% endfor %}

took about 20ms to render (sometimes up to 40) even without debug/development mode enabled while the raw HTML in the template:

{% for i in 1..100 %}
    <div class="alert alert-danger" role="alert">
        <div class="d-flex">
            <div class="me-2">
                <i class="ti ti-exclamation-circle fs-2x alert-icon"></i>
            </div>
            <div>
                Danger alert        </div>
        </div>
    </div>
{% endfor %}

took under 1ms consistently.

Another comparison is with the original macros which takes just 4-8ms:

{% import 'components/alerts_macros.html.twig' as alerts %}

{% for i in 1..100 %}
    {{ alerts.alert_danger("Danger alert") }}
{% endfor %}

A quick comparison in Vue rendering 100 Message components from PrimeVue (which are actually more customizable) took 12-14ms in development mode and 5-8ms in production, which makes it about as fast as SSR with Twig macros.
I don't have metrics for Twig Components in production mode.

Once a lot of components are being used, especially nested, the performance overhead is going to be more of an issue.

Finally, options for testing these components remains PHPUnit doing HTML parsing or E2E loading a full, slow GLPI page. With Vue, there is the option of a component test that mounts just the component(s) being tested in a real browser.

In order to even start convincing me of using twig components, I would need to see a more consequential use of then in something like the ticket timeline, datatables (with the pagination, sort and filter functionality), search or kanban.

[froozeify]

@cconard96 This was seen and discussed with @cedric-anne and @orthagh.

The goal is to remove macro for easier and more readable components enhancing the DX.

[orthagh]

Please @cconard96,

This pr doesn't mean we are fullly rewriting GLPI with Twig components, just cleaning a few quircks in the existing codebase.
The next release will be lite, and we are not ready for a full rewrite, either with symfony or vue.
I know you are pretty convinced and vocal about the latter, but this need to be a shared consensus and for the moment, even with support, it's not the case.

For the moment, with new interns, we need to build correctly the team to continue our progression.
And so, I will not greenlight a frontend rewrite project until we write a big picture document and work on specs.
For the moment, I prioritize feature for october release.

I appreciate your tests with your personal side-project and follow a bit the thing, but this has not been approved as an official project.

[cconard96]

Please @cconard96,

This pr doesn't mean we are fully rewriting GLPI with Twig components, just cleaning a few quirks in the existing codebase. The next release will be lite, and we are not ready for a full rewrite, either with symfony or vue. I know you are pretty convinced and vocal about the latter, but this need to be a shared consensus and for the moment, even with support, it's not the case.

Its not just that I am convinced of the latter. I want GLPI to be more maintainable, performant, and offer benefits to users no matter which direction we go. Part of that is not seeing something simple like a Twig alert macro being abstracted away making it slower for little if any benefit. Also the roadmap entry only talking about Alerts made me worried the scope was very limited. Adding another way to render HTML to GLPI just for alerts seems unnecessary and adding this for a lot of things could bring the performance concerns I outlined.

Obviously I take a lot of pride in my Vue work (and most of the work I do for GLPI as a whole), but I clearly haven't abandoned the current frontend. I've been trying my best to make improvements within the restrictions of the current design even as I work on something I'd like to see replace it. If my only opinion was that "my solution is right" I wouldn't be trying to fix what is already in use.

[cedric-anne]

With a huge application like GLPI, the "single responsibility" concept, combine with clear code structure, is very important to give the ability to developers to easilly locate the code responsible of something they want to fix/enhance. It is also mandatory if we want to be able in the long term to split the GLPI code into application modules.

Twig components, combined with the dependency injection, may help a lot to have standalone components with their own class+template.
Alerts is a simple case, but Twig components can be usefull, for instance, to have standalone "dropdown" components with their own class and template (and maybe their own JS module too), instead of having part of their HTML rendered by PHP code inside the Dropdown and the HTML classes and just wrapped inside a Twig macro.

[cconard96]

I think we will have to agree to disagree on the ease of locating responsible code (Symfony plugin doesn't even seem to properly support them in PHPStorm) at least with what I saw so far, especially for new contributors and plugin maintainers that aren't fully aware of GLPI's code and the Symfony Framework as a whole. I suppose there is a benefit for hired developers if Teclib' specifically hires PHP developers with Symfony experience.

Nevertheless without seeing how this works out with something more complex my opinion of it has shifted to neutral. Not ideal, but not terrible as long as it isn't overused and the performance impact should be kept in mind.

If more logic shifts to inside the component classes, developers should be careful not to over-fetch or fetch duplicate data when possible since nothing is cached. It is already an issue with the current Twig setup, but there are cases where an array of items is given to the template all at once. It could be worse like this if each component fetches the data it needs separately.

[cedric-anne]

Could you split this into two distinct PRs ? I would like to focus on the integration of Twig inside the dependency injection system. Once merged the Twig component part would be pretty simple to validate.

[AdrienClairembault]

Alerts is a simple case, but Twig components can be usefull, for instance, to have standalone "dropdown" components with their own class and template (and maybe their own JS module too), instead of having part of their HTML rendered by PHP code inside the Dropdown and the HTML classes and just wrapped inside a Twig macro.

If I understand things correctly, common UI items like drodowns are not the best candidates to be a twig component due to high performance impact compared to simple twig files (we have pages with tons of dropdowns like the form editor that definitively do not need to get slower than they already are).

[froozeify]

Looking into doing the split for the integration of Twig inside the dependency injection system (will be in another PR)

FYI, I ran a quick test with 1000 Alert on 4 types (you can find detailed datas and info in the outline) :

Performance Summary

The following table aggregates the average execution times (in milliseconds) for 1,000 iterations:

Method	Avg. Total Twig Time	Avg. Component Overhead	Performance Rank	Diff against Macro
Twig Embed	271.25 ms	N/A	1st (Fastest)	-16.00 ms (-5.57%)
Twig Macro	287.25 ms	N/A	2nd	0
Twig Component	353.00 ms	60.68 ms	3rd	+65.75 ms (+22.89%)
Anonymous Component	449.38 ms	57.19 ms	4th (Slowest)	+162.13 ms (+56.44%)

For Twig component, first Alert generation took on average .38ms and the 999 other alerts take .07-.06ms to render.

So having a lot of component on the page could even neglect the performance difference with macro.

[orthagh]

and the 999 other alerts take .07-.06ms to render.

So, nothing
having 1000 components is very unlikely to exists even in a few years, and we shouldn't bother to make more research about that for the moment.

[froozeify]

Failing tests are due to the deprecation message I've added.

It was planed to do a simple PR here and to the migration on another PR for all the Alert calls in core but maybe it should be done in this one so ? (or I remove for now the deprecated log ??)

[cedric-anne]

Failing tests are due to the deprecation message I've added.

It was planed to do a simple PR here and to the migration on another PR for all the Alert calls in core but maybe it should be done in this one so ? (or I remove for now the deprecated log ??)

Just comment the call to Toolbox::deprecated() for the moment.

[trasher]

LGTM looking at th code. Please rebase to solve conflicts. Tests are currently failing.