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

Revert "Add Accept: */* header" #5

Merged
merged 2 commits into from
Oct 17, 2018
Merged

Revert "Add Accept: */* header" #5

merged 2 commits into from
Oct 17, 2018

Conversation

j0k3r
Copy link
Owner

@j0k3r j0k3r commented Oct 17, 2018

We got some many side effects when declaring headers that way. Even if by reading the code, everything should work as normal.
Like in j0k3r/graby#162.
But also, paywall stuff in wallabag doesn't work anymore because some headers are missing after few requests.

It was really hard to debug and it's still hard to describe a real scenario to allow someone to fix the problem.
As a conclusion, reverting the PR is a better solution at the moment.

Reverts #4

@nijel
Copy link

nijel commented Oct 17, 2018

The problem might be in setting headers elsewhere in the code as caling curl_setopt($this->curlHandle, CURLOPT_HTTPHEADER,...) overrides previously defined headers.

The else branch for getPinDns() did not set the headers before and I probably it's the usually used one. The proper fix probably would be to make SafeCurl::execute accept additional headers as a parameter and callers would use this interface exclusively to set the headers without manually touching them.

Anyway if this patch broke things, the getPinDns() branch in SafeCurl::excecute is broken in same way, it's probably not that visible.

@j0k3r
Copy link
Owner Author

j0k3r commented Oct 17, 2018

You might be right.
I'm not using the pinDns option in any project.

I guess the problem will be gone when graby will switch to httplug (and use https://github.com/j0k3r/httplug-ssrf-plugin instead of SafeCurl)

In the meantime, sorry to revert your PR as you might not be able to save content from (at least) marigold.cz.

@nijel
Copy link

nijel commented Oct 17, 2018

Okay, I've tried to address my problem at higher level, what is probably more appropriate: wallabag/wallabag#3751

@j0k3r j0k3r merged commit 71bdb1a into master Oct 17, 2018
@j0k3r j0k3r deleted the revert-4-patch-1 branch October 17, 2018 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants