DX object to manage htmx attributes

All tests green

Missed bringing over the Twig function

New test passes.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Applied a suggestion and will refactoring some improvements inspired by @larowan review. A maintainer should add him to the credits.

All tests green.

There are two open questions:

1. The best way to ensure a string is returned in AttributeJson
2. Single/double quoted syntax. See also filtering setup for strings in AttributeHtmxString.

I don't understand why we need the single quote. If you add a JSON::encode(), to the process assets MR it just works.

    $build['replace'] = [
      '#type' => 'html_tag',
      '#tag' => 'button',
      '#attributes' => [
        'type' => 'button',
        'name' => 'replace',
        'data-hx-get' => $url->toString(),
        'data-hx-select' => 'div.ajax-content',
        'data-hx-target' => '[data-drupal-htmx-target]',
// add this part
        'data-hx-vals' => Json::encode(['test' => 'ok']),
      ],
      '#value' => 'Click this',
      '#attached' => [
        'library' => [
          'core/drupal.htmx',
        ],
      ],
    ];

It doesn't matter that the source html doesn't look pretty. Once it's in the JS it's the correct value.

That's a great question! I didn't think it would but I didn't try. I'll repeat the experiment with an attribute value selector and remove the complexity. Simpler is better!!

Confirmed! I can now remove a trait and a class - thank you @nod_ :)

I altered the render arrays in the test module to

    $build['replace'] = [
      '#type' => 'html_tag',
      '#tag' => 'button',
      '#attributes' => [
        'type' => 'button',
        'name' => 'replace',
        'data-hx-get' => $url->toString(),
        'data-hx-select' => 'div.ajax-content',
        'data-hx-target' => 'div[data-drupal-htmx-target="insert-here"]',
      ],
      '#value' => 'Click this',
      '#attached' => [
        'library' => [
          'core/drupal.htmx',
        ],
      ],
    ];

    $build['content'] = [
      '#type' => 'container',
      '#attributes' => [
        'data-drupal-htmx-target' => 'insert-here',
        'class' => ['htmx-test-container'],
      ],

Which produces data-hx-target="div[data-drupal-htmx-target="insert-here"]" through the standard string attribute. HTMX worked as expected.

All threads resolved - all tests passing :)

We agreed in Slack to pause on this builder while we work on 📌 Refactor BigPipe to use HTMX Active using attributes that are "hand-rolled."

Coming back to this now - we needed the htmx javascript API in 📌 Refactor BigPipe to use HTMX Active but not attributes.

Tagged \Drupal\Core\Template\HtmlAttributeInterface as @internal as that seems consistent with the policy on Attribute class internals → .

Discussed and agreed in Slack that a single change record and release note will cover:

• 📌 DX object to manage htmx attributes Active
• 📌 DX object to manage htmx headers Active
• 📌 DX object to collect and manage HTMX behaviors Active
• 📌 Define and process an #htmx render array key Active

Based on #26 and the description of the tag (A change record needs to be drafted before an issue is committed.) I'm removing the CR tag from this issue as these are sequential dependencies and the tag belongs on final component.

Reading through the governance doc I am marking this for framework manager review since it is blocking other work.

I brought the summary up to date with the code. We have 5 unresolved comment threads on the MR. Most open threads have a response.

Following a comment from @catch I updated the code in \Drupal\Core\Template\HtmxAttribute for:

• ::get
• ::post
• ::put
• ::patch
• ::delete
• ::pushUrl
• ::replaceUrl

to pass in and instance of CacheableDependencyInterface and collect the cacheable metadata emitted by the Url object.

There is an open question about the advisability of adding return types in \Drupal\Core\Template\Attribute. I understand this class to be internal by policy but maybe that's not right?

The other 3 open threads all have answers.

I have time to move this project forward to let's get this to RTBC so it can be committed and work can start on the next layer!!

@catch can you give guidance about adding return types to \Drupal\Core\Template\Attribute?

I hit a name collision on the BC policy with the Attribute name (As you and @nicxvan noted that was for PHP attributes). But it also seems to be permitted with these:

PHP and JavaScript classes →
In general, only interfaces can be relied on as the public API. With the exception of base classes, developers should generally assume that implementations of classes will change, and that they should not inherit from classes in the system, or be prepared to update code for changes if they do. See also more specific notes below.

Public methods not specified in an interface →
Public methods on a class that aren't on a corresponding interface are considered @internal.

We are adding an interface in this issue, but there no interface for this class now. @larowan seemed to think it was okay here: https://git.drupalcode.org/project/drupal/-/merge_requests/12097#note_51...

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thank you @phenaproxima for improving the code with your questions and suggestions. I've resolved all comments with either updates or answers except those related to return types (see #30). All tests are passing.

Given @larowlan's:

I did a search of contrib for classes that extend this where these new return types might be a BC break and could only find on in display suite, that was only adding a new method - and one in htmx which also didn't override any of the methods we're adding too here So I think this change is fine to do in a minor release, with a change record

and https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → I think we're OK to add the return type hints to Attribute here with a change record. It's allowed under the bc policy, and we can be fairly confident it won't break any existing code.

Ok that was tedious, but I clicked on all documentation links and confirmed they align except the ones that I have suggestions for.

I also reviewed all changes and have been following. I think this is ready assuming the three suggestions are accepted and tests pass.

Only remaining substantial question I have is where does sanitization happen?
I see some attributes do check the expected value is a boolean, but I don't see anything else, is that all handled upstream of this?

@nicxvan First, thank you for so carefully checking the tedious mundane details! Suggestions committed and test updated for the one method name fix.

Sanitization happens in the attribute value object's ::__toString method. They all extend AttributeValueBase which sanitizes the name property. We add AttributeJson in this MR, and follow this pattern.

I think this is ready, the extensive feedback has been addressed!

@catch @larowlan @nod_ A CacheableMetada parameter was added to methods that take a URL based on feedback. This complicates calling these methods within Twig as the caching has already bubbled.

Does this change the utility of a providing the attribute object within Twig? Should we drop that function from this MR?

I'd drop it. We can always add a followup to add it and we can figure out the problems without holding the working part of the MR

Thanks @nod_ !!

Removed the twig function from this issue's MR and the change record. It is extra.

CR is updated with the full picture of how all 4 issue changes work together.