New York
Account created on 23 October 2009, over 15 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States fathershawn New York

Nice use of once property to verify the behavior had done its work +1!

With 10.2-alpha1 out should we move the tag to 10.3 and release all the functionality together?

🇺🇸United States fathershawn New York

New test passes.

🇺🇸United States fathershawn New York

Missed bringing over the Twig function

🇺🇸United States fathershawn New York

All tests green

🇺🇸United States fathershawn New York

All tests green

🇺🇸United States fathershawn New York

It might not be, but if we refactor BigPipe to use HTMX we should return HTML - probably with the placeholder as the main content, and I wondered if that might intersect with this use case??

🇺🇸United States fathershawn New York

I'm so glad to be working with collaborators who have a historical perspective on the code! This is an enhancement not a requirement that will give a bit of performance boost. I'm going to postpone it and move it down the task list before BigPipe.

In the module, this simplified HTML response is a route option and scanning the linked history that may be a good thing. Let's think about routes we might build to serve requests from HTMX and pick up this work in a while. Ideas that I have for routes are:

  • the one I have in the module /htmx/{entityType}/{entity}/{viewMode}
  • one to render placeholders as the main content
🇺🇸United States fathershawn New York

All other unresolved MR comments are from the previous MR which we have closed.

🇺🇸United States fathershawn New York

I can't mark the MR comment resolved but I removed the version requirement from the info.yml file in the test module.

🇺🇸United States fathershawn New York

@longwave Okay, that's two experienced maintainers recommending a renderer over page variants. Thanks for weighing in!

We don't need the query string as HTMX adds the header HX-Request: true to all requests. I'll start to work on that approach as an alternative. I think we do still want title though so that the HTMX feature of swapping the title is available if appropriate but we turn it off by default.

🇺🇸United States fathershawn New York

Rendering the system messages in the response would mean that we wouldn't need a command to define the messages. The core systems of defining and displaying messages is available.

We would need to implement in a later issue either

  1. Always select and display system messages as additional inserted content
  2. Provide an operation for selecting the messages from the response and displaying them to be used with appropriate
🇺🇸United States fathershawn New York

All tests green, including new unit test for this subscriber

🇺🇸United States fathershawn New York

Nice simple solution @nod_. All tests green including the the added Ajax interaction test. What else do we need to move to RTBC?

🇺🇸United States fathershawn New York

Added contacts to HTMX

🇺🇸United States fathershawn New York

Another advantage of the page variants is that SimplePageVariant contains system messages as well as main content. That would allow the calling element to use hx-select-oob, or for us to add an hx-swap-oob to the response, and display error messages as well as the selected content from the response.

🇺🇸United States fathershawn New York

Maybe. That doesn't necessarily look simpler to me but I don't know the internals as well as @larowlan. The work in 📌 Process attachments (CSS/JS) for HTMX responses and add drupal asset libraries Active is based on returning an HtmlResponse object, so would we be creating a simpler version of \Drupal\Core\Render\MainContent\HtmlRenderer? I would think we would still need an event subscriber to divert the request to the alternate renderer.

🇺🇸United States fathershawn New York

Opened an MR that uses a default simple page with a header control for using a block page variant.

🇺🇸United States fathershawn New York

fathershawn changed the visibility of the branch linting-in-progress to hidden.

🇺🇸United States fathershawn New York

fathershawn changed the visibility of the branch 11.x to hidden.

🇺🇸United States fathershawn New York

@nod_ requested two additional test cases in the prior PR.

  1. htmx powered elements inserted by our legacy ajax
  2. verify that our attach and detach behavior methods fire

I've added a route to the test_htmx module (/htmx-test-attachments/ajax) that creates markup for this test. In manual testing, htmx is inserted but does not process the inserted content. I think we need some glue code that calls htmx.process on markup inserted by our Ajax API.

The only way I see to verify that methods are called in the Nightwatch test is to use the callTracker assertion but this is deprecated, so I'm wondering if someone has a better approach?

I also added some documentation links to the JS files.

🇺🇸United States fathershawn New York

fathershawn changed the visibility of the branch without-htmx-attachement-processor to hidden.

🇺🇸United States fathershawn New York

fathershawn changed the visibility of the branch 3521173-process-attachments-cssjs to hidden.

🇺🇸United States fathershawn New York

fathershawn changed the visibility of the branch asset-tests to hidden.

🇺🇸United States fathershawn New York

Thinking about the feedback on an additional processor service I realized that we should be able to simplify that whole part of the solution by adding some appropriate conditions to the existing HtmlResponseAttachmentsProcessor. I'm hitting a wrinkle though which is that HtmlResponseAttachmentsProcessor gets called more than once preparing the response to my test page. This is an issue because if I call setAlreadyLoadedLibraries on the assets, then they stop getting added in subsequent runs.

🇺🇸United States fathershawn New York

Someone with more permissions must be needed to edit the JS Dependencies page. I don't know if someone edited source or there are other text filters available, but I can't add additional headers and rows to the existing table.

Here's the needed data:

Repository https://github.com/bigskysoftware/htmx
Release cycle Releases are expected quarterly.
Security policies https://github.com/bigskysoftware/htmx/security
Security issue reporting https://github.com/bigskysoftware/htmx/security/advisories/new
Contact(s) 1cg , fathershawn

🇺🇸United States fathershawn New York

I hav two questions about merging drupalSettings.

ajax.js uses jQuery to merge the incoming drupalSettings value using the deep variation of .extend()

      if (response.merge) {
        $.extend(true, drupalSettings, response.settings);
      } else {
        ajax.settings = response.settings;
      }

In my contrib implementation I didn't want to start with a reliance on jQuery so adapted code from https://youmightnotneedjquery.com. I've kept this pattern in the MR at the moment, but I see that jQuery.extend() is used nearly a dozen other scripts. What's the best way forward? I see #3179551: Provide no-library equivalents of common/useful jQuery functions but it doesn't include .extend().

2. The ajax API provides two options for drupalSettings above. I currently simply merge. What is the use case for not merging incoming settings?

I've added a Nightwatch test, and all Nightwatch tests are passing.

🇺🇸United States fathershawn New York

I would recommend that not this child issue, but 📌 Process attachments (CSS/JS) for HTMX responses and add drupal asset libraries Active carry the change record. My reason is that although this adds the library, without the ability to bring in dependent CSS/JS it's not really usable yet. HTMX natively has properties to merge header tags but that clearly misses a lot of our JS.

If someone with more familiarity about the expectations around change records thinks that 2nd issue would be better place, I'll move the tag.

🇺🇸United States fathershawn New York

I have this working with in integration test. If anyone wants to see that locally, there is an asset-tests branch in the fork, which is this issue, plus the htmx dependency issue merged into the core 11.x branch. That allows the test to run. I'll bring that test over to this branch once the htmx dependency work is merged.

🇺🇸United States fathershawn New York

That exception is thrown by \Drupal\oauth2_client\Service\Oauth2ClientService::getClient which is called in each of those methods.

🇺🇸United States fathershawn New York

Tests passed 🎉

🇺🇸United States fathershawn New York

We have test failures that seem unrelated to this work: https://git.drupalcode.org/issue/drupal-3520723/-/pipelines/481254

🇺🇸United States fathershawn New York

Resetting status and moving to "Fixed" at @jurgenhaas suggestion so someone can comment and assign issue credits. Thought we needed to commit something to do that :)

🇺🇸United States fathershawn New York

I feel like this issue did it's job. We are now taking the learning from this work into new child issues on 🌱 Gradually replace Drupal's AJAX system with HTMX Active

🇺🇸United States fathershawn New York

MR is updated. Leaving as "active" until we have the security policy resolved

🇺🇸United States fathershawn New York

The storage is implemented by your own plugin class as the kind of storage varies by use case. My suggestion in #78 was to implement the storage methods but within them use a dpm to see the values rather than store them so you could see what was returned.

Alternatively you can use the provided storage trait and put a dpm() or ksm() call in the trait.

🇺🇸United States fathershawn New York

The snippet of code provided in #75 is from the StateTokenStorage which itself is provided to reduce the code that developers need to implement in their plugins.

If the StateTokenStorage file is empty of code then something is wrong with your code base. If you mean that nothing has been stored in the State system see my next paragraph.

The interface requires that you implement public function storeAccessToken(AccessTokenInterface $accessToken) but while you are developing you don't have to store anything. You could simply print info from the token object to a message in your implementation of this method and not use the trait.

🇺🇸United States fathershawn New York

Linting complete.

Most tests fixed.

Only 1 failing test: ActiveLinkResponseFilterTest. Suggestions welcome from anyone else who has worked in this area. I'm going to fire up xdebug to step through and puzzle out what is needed

🇺🇸United States fathershawn New York

So apparently it always has worked but no message of getting the token was ever placed anywhere to inform of success as you stated is supposed to occur

I have not been able to pull and print it from token storage however

The message display is a conditional in the StateTokenStorage trait. I have just re-verified that it is functioning correctly.

  /**
   * Stores access tokens obtained by the client.
   *
   * @param \League\OAuth2\Client\Token\AccessTokenInterface $accessToken
   *   The token to store.
   */
  public function storeAccessToken(AccessTokenInterface $accessToken): void {
    $this->state->set('oauth2_client_access_token-' . $this->getId(), $accessToken);
    if ($this->displaySuccessMessage()) {
      $this->messenger->addStatus(
        $this->t('OAuth token stored.')
      );
    }
  }

I don't find any cloud based services for testing the client credentials flow, but you can configure and authorization code plugin to intereact with https://docs.wiremock.io/security/oauth2-mock#using-with-your-app

🇺🇸United States fathershawn New York

It was the last client that sends and gets a response

I have different results. In a clean install of Drupal, using latest version of Devel, the example plugin is both posted and uploaded in #66 along with inserting debugging functions in both sides of the code branch within getParsedResponse also posted in #66, I get an expected denial response from USPS, also as posted when using random strings as client id and client secret.

I can try to add some more debugging output built into the module, but you could one of Devel's other functions there such dpm() instead.

🇺🇸United States fathershawn New York

Rebased on this week's changes. Looking at test failures next

🇺🇸United States fathershawn New York

What I posted in #66 was the result of enabling the attached module in a new install of Drupal and altering the AbstractProvider code as shown.

I used the latest version of Devel which has this backwards compatibility code: https://gitlab.com/drupalspoons/devel/-/blob/5.x/devel.module?ref_type=h... but any of Devel's methods of outputing data would probably work. I use other methods to inspect values at runtime but this was my best idea.

I entered dummy client ID and secret and did a save and test. USPS responded with a message about improper credentials. What I posted works correctly, it just needs a real id and secret. If it is not working in your situation there must be some interaction with other code or the environment. I can't think of anything else.

🇺🇸United States fathershawn New York

If you have a ksm() call in both sides of the try/catch as shown in AbstractProvider then something is very much off with how the code is running in your setup. That's a dual path code branch and so one of them runs.

I can't tell more from here. Is there a Drupal community where you are? Someone you could pair with to debug this with you?

🇺🇸United States fathershawn New York

Making multiple large changes to your code is not the best way to debug. Single changes and test will get you where you want to be.

You didn't get a message in #62 likely because USPS returned an error. So let's move to this:

    public function getParsedResponse(RequestInterface $request)
    {
        try {
            $response = $this->getResponse($request);
ksm($response->getBody()->getContents());
        } catch (BadResponseException $e) {
            $response = $e->getResponse();
ksm($response->getBody()->getContents());
        }

        $parsed = $this->parseResponse($response);

        $this->checkResponse($response, $parsed);

        return $parsed;
    }

With this in place, and this plugin (Also uploaded in a zip file as example 3)

<?php

declare(strict_types=1);

namespace Drupal\usps_api_example\Plugin\Oauth2Client;

use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\oauth2_client\Attribute\Oauth2Client;
use Drupal\oauth2_client\Plugin\Oauth2Client\Oauth2ClientPluginBase;
use Drupal\oauth2_client\Plugin\Oauth2Client\StateTokenStorage;
use League\OAuth2\Client\Provider\AbstractProvider;
use League\OAuth2\Client\Provider\GenericProvider;

/**
 * Client credentials for USPS test endpoint.
 */
#[Oauth2Client(
  id: 'usps_test_api',
  name: new TranslatableMarkup('USPS Test API'),
  grant_type: 'client_credentials',
  authorization_uri: 'https://apis-tem.usps.com/oauth2/v3/token',
  token_uri: 'https://apis-tem.usps.com/oauth2/v3/token',
)]
class UspsTestApi extends Oauth2ClientPluginBase {
  use StateTokenStorage;

  /**
   * Creates a new provider object.
   *
   * @return \League\OAuth2\Client\Provider\GenericProvider
   *   The provider of the OAuth2 Server.
   */
  public function getProvider(): AbstractProvider {
    return new GenericProvider(
      [
        'clientId' => $this->getClientId(),
        'clientSecret' => $this->getClientSecret(),
        'urlAuthorize' => $this->getAuthorizationUri(),
        'urlAccessToken' => $this->getTokenUri(),
        'urlResourceOwnerDetails' => $this->getResourceUri(),
      ],
      $this->getCollaborators()
    );
  }

}

I get this message after clicking "Save and request token"

🇺🇸United States fathershawn New York

You have two return statements in your getProvider code in #63

Maybe try inserting calls to ksm earlier in the stack after you fix that. You should be able to use that to find where it is failing

Production build 0.71.5 2024