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

Command icon 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

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Title: Expose Twig's built-in CVA function » Support Twig's CVA function

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

phenaproxima’s picture

Status: Active » Needs review
Issue tags: +Needs tests

Needs 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.

phenaproxima’s picture

Status: Needs review » Needs work

@longwave informed me in Slack that we could add twig/html-extra as 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.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Test written. Probably needs review.

dcam’s picture

Tagging for framework manager and release manager review. Per the dependency evaluation guidelines one of each must sign off on a new dependency.

longwave’s picture

Given 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.

alexpott’s picture

I agree with #12 this looks like a good addition. html_classes also looks useful.

phenaproxima’s picture

I think it would make sense to add html_classes, and a test for it, in a follow-up. Would that be okay?

phenaproxima’s picture

Per 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_classes function in some of core's templates.

penyaskito’s picture

TIL 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.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community

Took a look at the MR, tested locally. Looks good to me! Seems like it'll make frontend developers happy

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs release note

We need a CR and release note here to tell people about this.

alexpott’s picture

Issue summary: View changes

Adding in placeholder for release note.

phenaproxima’s picture

Issue tags: -Needs change record

Done, and linked to Twig's docs: https://www.drupal.org/node/3581193

phenaproxima’s picture

Issue summary: View changes
Issue tags: -Needs release note

Added release note.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Since no code changes were made here, restoring RTBC.

alexpott’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 00bf9b2 and pushed to main. Thanks!
Committed 134cf9f and pushed to 11.x. Thanks!

I took care of the 11.x backport while merging.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 134cf9f7 on 11.x
    feat: #3559156 Support Twig's CVA function
    
    By: phenaproxima
    By:...

  • alexpott committed 00bf9b29 on main
    feat: #3559156 Support Twig's CVA function
    
    By: phenaproxima
    By:...
alexpott’s picture

Issue tags: +11.4.0 release notes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.