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.

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

Add absolute check to url_for_asset and url_for_file to fix appending "/" to absolute file paths #2112

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Add absolute check to url_for_asset and url_for_file to fix appending "/" to absolute file paths #2112

merged 1 commit into from
Oct 5, 2023
+9 −3

Conversation

rawleyfowler
Copy link
Contributor

@rawleyfowler rawleyfowler commented Sep 20, 2023 •
edited
Loading

Summary

There is currently a bug where:

%= image('https://www.somesite.com/image.png')

produces

<img src="/https://www.somesite.com/image.png">

This PR adds an absolute check to the file_path and asset_path methods that currently breaks this.

Motivation

This is breaking multiple of my systems in production. (And it seems its breaking others as well)

References

#2107

kraih
kraih previously requested changes Sep 20, 2023
View reviewed changes
Copy link
Member

@kraih kraih left a comment

Choose a reason for hiding this comment

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

This duplicates code that already exists in Mojolicious::Controller. Just implement it the same way it is done in mojo.js.

@rawleyfowler rawleyfowler requested a review from kraih September 20, 2023 15:05
@mergify mergify bot dismissed kraih’s stale review September 20, 2023 15:05

Pull request has been modified.

@rawleyfowler rawleyfowler changed the title Add absolute check to file_path and asset_path to fix appending "/" to absolute file paths Add absolute check to url_for_asset and url_for_file to fix appending "/" to absolute file paths Sep 20, 2023
@rawleyfowler
Copy link
Contributor Author

rawleyfowler commented Sep 20, 2023 •
edited
Loading

I haven't seen any constants in Mojolicious (perl), wasn't sure if you'd like to pull the regex to a constant ala Mojo.js, but besides that this should be 1:1 with Mojo.js.

kraih
kraih reviewed Sep 20, 2023
View reviewed changes
lib/Mojolicious/Static.pm Outdated Show resolved Hide resolved
kraih
kraih reviewed Sep 20, 2023
View reviewed changes
lib/Mojolicious/Static.pm Outdated Show resolved Hide resolved
@rawleyfowler rawleyfowler force-pushed the main branch 2 times, most recently from d6ce386 to e26ce8e Compare September 20, 2023 16:43
@rawleyfowler rawleyfowler requested a review from kraih September 20, 2023 16:43
kraih
kraih previously requested changes Sep 20, 2023
View reviewed changes
lib/Mojolicious/Controller.pm Outdated Show resolved Hide resolved
lib/Mojolicious/Controller.pm Outdated Show resolved Hide resolved
lib/Mojolicious/Controller.pm Outdated Show resolved Hide resolved
@mergify mergify bot dismissed kraih’s stale review September 20, 2023 17:13

Pull request has been modified.

@rawleyfowler rawleyfowler requested a review from kraih September 20, 2023 17:16
@jixam
Copy link
Contributor

jixam commented Oct 5, 2023

FWIW, I added this patch to our build and our tests started passing again. So, works for me :)

kraih
kraih reviewed Oct 5, 2023
View reviewed changes
lib/Mojolicious/Controller.pm Outdated Show resolved Hide resolved
@rawleyfowler rawleyfowler requested a review from kraih October 5, 2023 17:44
@kraih kraih requested review from a team and Grinnz October 5, 2023 18:29
jhthorsen
jhthorsen approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mergify mergify bot merged commit a607e42 into mojolicious:main Oct 5, 2023
10 checks passed
@sergiotarxz sergiotarxz mentioned this pull request Feb 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhthorsen jhthorsen jhthorsen approved these changes

@kraih kraih kraih approved these changes

@marcusramberg marcusramberg Awaiting requested review from marcusramberg marcusramberg was automatically assigned from mojolicious/maintainers

@jberger jberger Awaiting requested review from jberger jberger was automatically assigned from mojolicious/maintainers

@christopherraa christopherraa Awaiting requested review from christopherraa christopherraa was automatically assigned from mojolicious/maintainers

@Grinnz Grinnz Awaiting requested review from Grinnz Grinnz was automatically assigned from mojolicious/maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rawleyfowler @jixam @kraih @jhthorsen