Skip to content

Use Request.URL.RequestURI() for fcgi #14314

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
Merged
merged 3 commits into from
Jan 13, 2021
Merged

Use Request.URL.RequestURI() for fcgi #14314

merged 3 commits into from
Jan 13, 2021

Conversation

WKBae
Copy link
Contributor

@WKBae WKBae commented Jan 12, 2021

Closes #14312

Since #9473, new usages of Request.RequestURI seems to have added, which yields empty string on fcgi protocol.
This commit rewrites current usages of Request.RequestURI to Request.URL.RequestURI().

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 13, 2021
bagasme
bagasme reviewed Jan 13, 2021
View reviewed changes
Copy link
Contributor

@bagasme bagasme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are mentions to fcgi in this diff?

@Meano
Copy link
Contributor

Meano commented Jan 13, 2021

Where are mentions to fcgi in this diff?

The server configured with fcgi cannot get any avatar / repo-avatar since v13.0 (returned 404), because routers/routes/chi.go routers/routes/chi.go : storagehandeler() : req.RequestURI is empty.

@lunny
Copy link
Member

lunny commented Jan 13, 2021 •
edited
Loading

Where are mentions to fcgi in this diff?

The server configured with fcgi cannot get any avatar / repo-avatar since v13.0 (returned 404), because routers/routes/chi.go routers/routes/chi.go : storagehandeler() : req.RequestURI is empty.

It's v1.14 and the master branch. v1.13 hasn't introduce chi. So only storagehandler and example need to be backport but not chi part.

@lunny lunny added this to the 1.13.2 milestone Jan 13, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 13, 2021
@lafriks lafriks modified the milestones: 1.13.2, 1.14.0 Jan 13, 2021
@zeripath
Copy link
Contributor

make lg-tm work

@zeripath zeripath merged commit edbc5c8 into go-gitea:master Jan 13, 2021
@zeripath
Copy link
Contributor

please send backport

a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jan 14, 2021
@a1012112796
Merge branch 'master' into imap
4baedb5 
* master:
  Use Request.URL.RequestURI() for fcgi (go-gitea#14312) (go-gitea#14314)
  Update Link
  [skip ci] Updated translations via Crowdin
  Kd/add bountysource (go-gitea#14323)
@WKBae WKBae mentioned this pull request Jan 15, 2021
Merged
@lafriks lafriks added the backport/done All backports for this PR have been created label Jan 15, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@lafriks lafriks lafriks approved these changes

+1 more reviewer

@bagasme bagasme bagasme left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.

Empty URI in logs when using fcgi protocol
7 participants
@WKBae @Meano @lunny @zeripath @lafriks @bagasme @GiteaBot