8000 Add a protocol to relative urls in FileHandler fixes #4918 by octfx · Pull Request #4957 · SemanticMediaWiki/SemanticMediaWiki · GitHub
[go: up one dir, main page]
Skip to content

Add a protocol to relative urls in FileHandler fixes #4918 #4957

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add a protocol to relative urls in FileHandler fixes #4918 #4957

wants to merge 4 commits into from

Conversation

octfx
Copy link
Contributor
@octfx octfx commented Mar 25, 2021

This PR is made in Reference to #4918

Tries to add a protocol to a relative url, by checking wgCanonicalServer or the request through RequestContext::getMain. If both fail http is used as a fallback.

@kghbln
Copy link
Member
kghbln commented Mar 30, 2021

@octfx Thanks for providing this patch and please excuse that is has been rotting here since.

@JeroenDeDauw Could you have a peep at this?

@JeroenDeDauw
Copy link
Member

I'm afraid to merge this without tests

@octfx
Copy link
Contributor Author
octfx commented Apr 27, 2021

I'll try to add some when my time permits it!

@26
Copy link
26 commented May 19, 2021 •
edited
Loading

I do not think this would work for two reasons:

  • file_get_contents also does not understand protocol-less URIs. The URL also needs to be protocolized before it is passed to that.
  • In the function protocolizeUrl, you return the unaltered canonical URL if it has a scheme. This means any path or query that was passed to the function would be lost.

I believe just removing lines 136 to 145 (the try-catch block) would fix the second issue. The first issue may be fixed by assigning the protocolized URL to a variable and passing that variable to both file_get_contents as well as get_headers.

@octfx
Copy link
Contributor Author
octfx commented May 19, 2021

I do not think this would work for two reasons:

  • file_get_contents also does not understand protocol-less URIs. The URL also needs to be protocolized before it is passed to that.
  • In the function protocolizeUrl, you return the unaltered canonical URL if it has a scheme. This means any path or query that was passed to the function would be lost.

I believe just removing lines 136 to 145 (the try-catch block) would fix the second issue. The first issue may be fixed by assigning the protocolized URL to a variable and passing that variable to both file_get_contents as well as get_headers.

Oh, the file_get_contents part is a good catch, that slipped through!

I've updated the try-catch block to add the canonical server protocol to the input url.

Further I've added some tests, but I couldn't get them to run locally as it always errored out with Use of undefined constant SMW_PHPUNIT_AUTOLOADER_FILE [...] boostrap.php on line 15 :/

@octfx
Update protocol adding logic
812f367 
- Add FileIndexerTest
@JeroenDeDauw
Copy link
Member

@Seb35 does this look good to you?

@marijnvanwezel
Copy link
Contributor

@JeroenDeDauw Could you take a look at this again when you have the time? We are facing this issue again.

@JeroenDeDauw
Copy link
Member

@marijnvanwezel did you test that this patch fixes the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@octfx @kghbln @JeroenDeDauw @26 @marijnvanwezel
0