Hi,

I am trying to import a feed from a website that requires the User-Agent not be empty/null. I noticed that whenever the get request is made from https://git.drupalcode.org/project/feeds/-/blob/8.x-3.x/src/Feeds/Fetche... the User-Agent is empty. This could be GuzzleHttp issue with this function https://github.com/guzzle/guzzle/blob/6.5/src/Client.php#L287 but I didn't find any issues calling it out there so I am starting here.

I am suggesting for now as a workaround to add the User-Agent to the get function by getting the default from GuzzleHttp client.

Attaching the patch in a new comment. Any other suggestions?

CommentFileSizeAuthor
#7 missingUserAgent-3159688-7.patch786 bytesdmundra
#2 missingUserAgent-3159688-2.patch675 bytesdmundra

Issue fork feeds-3159688

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:

About issue forks

Comments

dmundra created an issue. See original summary.

dmundra’s picture

dmundra
he/him
Primary language English
commented
StatusFileSize
new missingUserAgent-3159688-2.patch675 bytes

Attaching patch.

jamesdixon’s picture

jamesdixon commented

I see what you're going for there.

@megachriz: I don't think it would hurt to add user agent here if it were configured, what do you think?

dmundra’s picture

dmundra
he/him
Primary language English
commented

Thanks.

Some more background. I did figure out that a WordPress vendor was blocking User-Agent with the string 'PHP/' in it. I am working with them to find out why they are doing that.

The generated User-Agent that is getting deleted will be like 'Drupal/8.9 (+https://www.drupal.org/) GuzzleHttp/6.5.5 curl/7.64.0 PHP/7.2.27' (from https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/lib/Drupal/C...)

irinaz’s picture

irinaz commented

@dmundra, thanks for putting together this patch. I am testing and still getting error - is there way to see which user agent is added by this patch? thanks in advance.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs work 
+++ b/src/Feeds/Fetcher/HttpFetcher.php
@@ -141,6 +141,10 @@ class HttpFetcher extends PluginBase implements ClearableInterface, FetcherInter
+    if (is_array($this->client->getConfig('headers'))) {
+      $options[RequestOptions::HEADERS]['User-Agent'] = $this->client->getConfig('headers')['User-Agent'];

Having a code comment explaining why only user-agent headers added would be great

Moreover not clear why is_array() used here because headers are always defined via \Drupal\Core\Http\ClientFactory::fromOptions() https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/lib/Drupal/C...

dmundra’s picture

dmundra
he/him
Primary language English
commented
StatusFileSize
new missingUserAgent-3159688-7.patch786 bytes

@irinaz here is an example of the User-Agent but it will depend on your version of the site and what server it is running on:

Drupal/9.1.0 (+https://www.drupal.org/) GuzzleHttp/6.5.5 curl/7.64.0 PHP/7.4.13

@andypost

Adding an updated patch with a comment and switched from is_array to isset() (which is probably what I was wanting to do).

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

Thank you, looks great 👍

Does it make sense to allow more headers?

dmundra’s picture

dmundra
he/him
Primary language English
commented

@andypost, I think it would be beneficial to even allow customization of the headers including user-agent.

megachriz’s picture

megachriz
Primary language Dutch
commented

We could add a field for setting additional headers in the Feeds UI. Are extra headers to be set usually static values? Or is it more common that they depend on other data, like the current date for example? Maybe the option for setting extra headers should be handled in an other issue.

dmundra’s picture

dmundra
he/him
Primary language English
commented

@megachriz agreed that setting extra headers should be handled in another issue that can cover the complexity. Following Postman app example it could be static or dynamic. Maybe an API hook would be beneficial for those who want to provide dynamic headers but they can also just extend the class. That is my two cents.

andypost’s picture

andypost
he/him
Primary language Russian
commented

I bet http-client should allow to hook-in and customize headers
Quickly skimmed and found https://www.drupal.org/docs/8/modules/http-client-manager/the-handler-stack

dmundra’s picture

dmundra
he/him
Primary language English
commented

Good find @andypost.

irinaz’s picture

irinaz commented
Status: Needs review » Reviewed & tested by the community

  • MegaChriz committed 09622a4 on 8.x-3.x authored by dmundra 
    Issue #3159688 by dmundra, andypost, irinaz, MegaChriz: Send the default...
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Reviewed & tested by the community » Fixed

I tried to reproduce the issue about no user agent being sent, but in my case an user agent is being sent even without the patch. But apparently, there is a situation where this doesn't happen and it doesn't look like the patch does any harm either, so committed #7 - with coding standards fixed.

Status: Fixed » Closed (fixed)

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

megachriz’s picture

megachriz
Primary language Dutch
commented

GuzzleHttp decided to deprecate the getConfig() and it looks there is no easy replacement for it: https://github.com/guzzle/guzzle/issues/3114
That means I either have to remove this feature, find another way to implement it or ignore the deprecation warning for now. Ignoring it could mean that at some point in the future this feature might stop working.

Since no test coverage for this was added, it will be harder to try to find a replacement for this. Also because in #16 I said I wasn't able to reproduce the bug.

@dmundra, @andypost
Can you help with this?

MegaChriz opened merge request !182

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Closed (fixed) » Active

MR 182 is merged manually.

Since this issue is now no longer fixed, I reopen it.

@dmundra said the following about this issue in Slack:

Thanks for working on this. I haven’t revisited that issue and I haven’t run into a case where I have used that module against a service that blocks empty user agents so I am not the best person to look into it right now. I will see if can follow up on the discussions in slack and in the thread but it going to be a while.

Reverting the changes make sense

See also https://drupal.slack.com/archives/C033S2JUMLJ/p1720346469627799

MegaChriz closed merge request !182

dmundra’s picture

dmundra
he/him
Primary language English
commented
Status: Active » Closed (cannot reproduce)

So I think this is no longer an issue and so no fix is needed. I tested with a new Drupal site and the latests feeds branch and the requests working to even the site I had an issue with. So I think this should be closed.