Skip to content

set a new route ctx if rctx is nil#878

Closed
shedyfreak wants to merge 1 commit into
go-chi:masterfrom
shedyfreak:master
Closed

set a new route ctx if rctx is nil#878
shedyfreak wants to merge 1 commit into
go-chi:masterfrom
shedyfreak:master

Conversation

@shedyfreak
Copy link
Copy Markdown

PR to solve #839

Comment thread middleware/url_format.go
Comment on lines +58 to +60
if rctx == nil {
rctx = chi.NewRouteContext()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we move this check above line 54 and replace if rctx != nil?

@VojtechVitek
Copy link
Copy Markdown
Contributor

Hey @shedyfreak, @yurineydachin, @AndreiOslin,

I'm just checking back on this PR and I wonder -- how do you trigger the #839 panic? Under what circumstance is the route context nil?

Might be helpful to provide a test case for this too.

@shedyfreak what's your thoughts on #878 (comment) ?

@VojtechVitek
Copy link
Copy Markdown
Contributor

It looks like #841 was submitted first and has a test case too. I'll follow up there.

Thanks for the PR!

Copy link
Copy Markdown
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

Hi, @shedyfreak

After all, I prefer this fix over #841.

Can we move if rctx == nil { check to the below if branch to avoid the panic per https://github.com/go-chi/chi/pull/878/files#r1418718973?

Thank you

@VojtechVitek
Copy link
Copy Markdown
Contributor

VojtechVitek commented Aug 15, 2025

Superseded by #1008. Thank you for the PR!

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.

4 participants