Problem/Motivation
Twig has a useful function for implementing CVA (Class Variance Authority). It's exposed by this small shim module: https://www.drupal.org/project/cva
The reasoning for exposing the function is well-explained on that project's page, so I won't repeat that here.
But it would benefit Drupal CMS, and theme developers generally, if we could just bring this function into core. It would require us to depend on twig/html-extra (at least to have the function consistently available, which is probably the best themer experience), but that appears to be, at a glance, maintained by the same folks who maintain Twig itself.
I propose we add twig/html-extra to core as a dependency, and make its functions available to Twig templates.
Dependency evaluation
This package is part of Twig -- it looks to be a subtree split: https://github.com/twigphp/Twig/blob/3.x/splitsh.json#L6
I think it's safe to assume that it therefore has the same maintainership, security policies, and release cycles as Twig itself, and I'm not sure that any further evaluation is necessary. It appears to release at the same time as Twig, at the same version.
Release notes snippet
Core now includes the twig/html-extra package, which exposes the html_cva, html_attr, and html_classes functions. All three are now available for use in Twig templates.
Issue fork drupal-3559156
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
phenaproximaComment #3
phenaproximaComment #6
phenaproximaNeeds tests, but ready for initial review.
Probably the main sticking point here is whether to rely on the HtmlExtension optionally (as done here) and expect sites to install it, or to actually add it to core's dependencies. It's probably not particularly controversial as a dependency, since it's maintained by Twig's developers anyway, but it is a separate dependency and therefore might require some process to evaluate it. I'm not sure.
Comment #7
phenaproxima@longwave informed me in Slack that we could add
twig/html-extraas a dependency, as long as we do the dependency evaluation first. That's a bit of a procedure, but we can do it in this issue, and the answers are probably going to be the same as Twig itself.Comment #8
phenaproximaComment #9
phenaproximaComment #10
phenaproximaTest written. Probably needs review.
Comment #11
dcam commentedTagging for framework manager and release manager review. Per the dependency evaluation guidelines one of each must sign off on a new dependency.
Comment #12
longwaveGiven that this is just a subtree split of Twig with the same maintainers, and we already heavily depend on Twig, there are no issues to adding this dependency from the release management side.
Comment #13
alexpottI agree with #12 this looks like a good addition.
html_classesalso looks useful.Comment #14
phenaproximaI think it would make sense to add
html_classes, and a test for it, in a follow-up. Would that be okay?Comment #15
phenaproximaPer Slack discussion with @alexpott, opened #3578007: Adopt the html_classes function in some of our Twig templates to see if we can adopt the handy-dandy
html_classesfunction in some of core's templates.Comment #16
penyaskitoTIL apparently we use this tag when adding dependencies to track these issues (assuming this will be approved and will happen in this very same issue?)
edit: maybe I'm adding it prematurely, but a reminder to check https://www.drupal.org/about/core/policies/core-dependency-policies/depe... and update it accordingly.
Comment #17
mglamanTook a look at the MR, tested locally. Looks good to me! Seems like it'll make frontend developers happy
Comment #18
alexpottWe need a CR and release note here to tell people about this.
Comment #19
alexpottAdding in placeholder for release note.
Comment #20
phenaproximaDone, and linked to Twig's docs: https://www.drupal.org/node/3581193
Comment #21
phenaproximaAdded release note.
Comment #22
phenaproximaSince no code changes were made here, restoring RTBC.
Comment #23
alexpottCommitted 00bf9b2 and pushed to main. Thanks!
Committed 134cf9f and pushed to 11.x. Thanks!
I took care of the 11.x backport while merging.
Comment #27
alexpott