Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow manipulating the handler stack per request #3152

Open
chx opened this issue Jul 7, 2023 · 3 comments
Open

Allow manipulating the handler stack per request #3152

chx opened this issue Jul 7, 2023 · 3 comments
Labels
lifecycle/stale No activity for a long time

Comments

@chx
Copy link

chx commented Jul 7, 2023

Description

#2514 removes the only (reflection-free) way to manipulate the handler stack.

Example

$additional_handler = (new HandlerStack(new CacheMiddleware(new PrivateCacheStrategy(new DrupalGuzzleCache($this->cache)));
$this->client->get($url, ['handler' => $additional_handler]);

Additional context

#2514 (comment) says

You can make the configuration yet another item in your container. You can "extend" it if you want to, before injecting it in a particular client.

This simply doesn't work if I want some requests cached and some I don't want to. Currently Drupal injects an instance of Client and I can just run getConfig handler and push the cache middleware and be done. Drupal uses a compiled container so changing the definition is really tricky run time.

Overall, trying to predict every use case is hard.

We could add a merge method to HandlerStack which accepts another HandlerStack, creates a pristine new HandlerStack, operates both the original and the one being merged and pushes the handlers on the new one.

public method merge(static $other) {
  $result = new static();
  foreach ([$this, $other] as $object) {
    foreach ($object->stack as $k => $v) {
      $result->push($k, $v);
    }
  }
  return $result;
}

And then Client::prepareDefaults could look at the request options, if a handler is provided then it calls said merge.

if (!empty($options['handler'])) {
  $result['handler'] = $this->config['handler']->merge($options['handler']);
}

It's not 100% compared to the current situation because you can't splice a handler before another but it's a big step ahead.

@sagikazarmark
Copy link
Member

As you also said, it's hard to recommend a solution that works for every scenario.

A couple things I would try before meddling with the HandlerStack:

  • Custom middleware deciding which requests to cache
  • Cache requests outside of the HTTP layer
  • Maintaining two clients: one with caching, one without

In my experience, HTTP middleware are often misused (another example that comes to mind is handling errors in middleware).

And then Client::prepareDefaults could look at the request options, if a handler is provided then it calls said merge.

This is potentially a backward incompatible change. Another potential use case for this option could be overwriting the entire HandlerStack instead of merging, so it would be confusing at best.

I could imagine an option that accepts a function and returns a HandlerStack. That way both merging and overwriting would be possible.

That being said, I haven't worked on Guzzle for a while, so I don't know if it would have any major implications. Let's wait for other maintainers to chime in.

@sagikazarmark
Copy link
Member

@GrahamCampbell I'd love to hear your thoughts about the topic since you've been spearheading Guzzle maintenance lately (for which I'm eternally grateful).

Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale No activity for a long time
Projects
None yet
Development

No branches or pull requests

2 participants