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

fix(gatsby-core-utils): decode uri-encode filename for remote file #35637

Merged
merged 5 commits into from Nov 16, 2022
Merged

fix(gatsby-core-utils): decode uri-encode filename for remote file #35637

merged 5 commits into from Nov 16, 2022

Conversation

cometkim
Copy link
Contributor

Description

Tried decode url-encoded filename before to store remote file. So it can be served properly via generated publicURL

Related Issues

Fixes #35636

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 12, 2022
@pieh
Copy link
Contributor

pieh commented May 16, 2022

This looks like something we would like to add test case for (to prevent regression in the future)

@pieh pieh added type: bug An issue or pull request relating to a bug in Gatsby topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 16, 2022
@cometkim
Copy link
Contributor Author

@pieh do you think is a unit test ok? or should it be e2e?

@pieh
Copy link
Contributor

pieh commented May 16, 2022

@pieh do you think is a unit test ok? or should it be e2e?

e2e would be most welcome (that gives highest level of confidence), but I think unit test will be enough given that https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-core-utils/src/__tests__/fetch-remote-file.js tests entire fetchRemoteFile pipeline (not just naming helper function)

@cometkim
Copy link
Contributor Author

@pieh added a test, but it seems some unrelated unit test is failing in the CI

@LekoArts LekoArts mentioned this pull request Oct 28, 2022
Closed
@cometkim cometkim mentioned this pull request Nov 1, 2022
Closed
2 tasks
@cometkim cometkim force-pushed the non-ascii-filename branch 2 times, most recently from b098ac8 to 5e529c3 Compare November 3, 2022 07:41
@cometkim
Copy link
Contributor Author

cometkim commented Nov 3, 2022 •
edited

Fixed to handle file directory correctly in other cases @Boronare mentioned

(No idea about test setup failure, I'll keep rebasing)

@cometkim
Copy link
Contributor Author

cometkim commented Nov 5, 2022

@pieh @LekoArts could you help me to merge this change?

@cometkim
Copy link
Contributor Author

ok it now pass all tests except Cloud Tests

@cometkim
Copy link
Contributor Author

Can it be merge soon?

LekoArts
LekoArts approved these changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait on this! Thanks for the PR and the tests 👍

@LekoArts LekoArts removed the type: bug An issue or pull request relating to a bug in Gatsby label Nov 16, 2022
@LekoArts LekoArts merged commit 086c862 into gatsbyjs:master Nov 16, 2022
@cometkim cometkim deleted the non-ascii-filename branch November 16, 2022 15:05
This was referenced Nov 22, 2022
Open
Open
This was referenced Nov 23, 2022
Open
Open
This was referenced Nov 23, 2022
Open
Open
This was referenced Nov 23, 2022
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Nov 23, 2022
Open
Open
This was referenced Nov 23, 2022
Open
Open
Open
Open
@ajesse11x ajesse11x mentioned this pull request Nov 23, 2022
Open
@0xSebin 0xSebin mentioned this pull request Nov 23, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Nov 27, 2022
Open
@saurabharch saurabharch mentioned this pull request Dec 5, 2022
Open
@0xSebin 0xSebin mentioned this pull request Feb 28, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Mar 3, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Mar 16, 2023
Open
@snyk-bot snyk-bot mentioned this pull request Mar 24, 2023
Open
This was referenced Apr 16, 2023
Open
Open
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
@cometkim
fix(gatsby-core-utils): decode uri-encode filename for remote file (g…
a42f4ca 
…atsbyjs#35637)

Fixes gatsbyjs#35636
@nidhi42 nidhi42 mentioned this pull request May 2, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LekoArts LekoArts LekoArts approved these changes

Assignees
No one assigned
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot download file with non-ascii filename from remote source
3 participants
@cometkim @pieh @LekoArts