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.

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

Fix(i18n): Inaccessible public folder on non-default domains #55137

Closed
wants to merge 1 commit into from

Conversation

pekarja5
Copy link
Contributor

@pekarja5 pekarja5 commented Sep 8, 2023

Changes in #52492 made public folder accessible only on default domain if using i18n. PR fixes this by making public directory contents accessible from all i18n domains as the original solution did.

Fixes #54765

@ijjk
Copy link
Member

ijjk commented Sep 8, 2023

Allow CI Workflow Run

  • approve CI run for commit: acf43f4

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

1 similar comment
@ijjk
Copy link
Member

ijjk commented Sep 8, 2023

Allow CI Workflow Run

  • approve CI run for commit: acf43f4

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 1   low 0   info 0 View in Orca

@bobbonius
Copy link

Hi there! My colleague created the issue you're referencing and I was wondering if I could be of any assistance to get this through? We want to go to next 13 but this issue is holding us back at the moment.

@pekarja5
Copy link
Contributor Author

@bobbonius There is nothing to do apart from waiting for it to get merged. The fix is really simple as it is just one line edit. It is already approved so i hope it will get merged and released soon.

@bobbonius
Copy link

There is nothing to do apart from waiting for it to get merged. The fix is really simple as it is just one line edit. It is already approved so i hope it will get merged and released soon.

Thanks!

@aledovskikh
Copy link

Hey! When it'll be released? Looks like this is some kind of critical bug

@adri
Copy link

adri commented Sep 29, 2023

@timneutkens sorry for tagging you directly! If you do have a minute to check this PR it would be amazing. It blocks us from upgrading next.js our static assets are gone on most domains.

@chrisneven
Copy link
Contributor

This is blocking us as well at the moment. It would be really nice if we could get this one true!

@chrisneven
Copy link
Contributor

@ijjk, maybe you can take a look at this? Since you made the change initially

Thanks in advance!

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

This PR is missing tests, please have a look at and follow the PR template to ensure the changes can be reviewed correctly.

@pekarja5
Copy link
Contributor Author

@timneutkens Does the test suite even supports testing access to multiple domains? I could not find any test with such functionality.

The goal is to make the test access files in public directory from multiple defined locale domains. If anyone has any tips how to make such test feel free to give me some ideas.

@aledovskikh
Copy link

aledovskikh commented Oct 30, 2023

Is there any chance to fix and merge it to nearest release?
We can't upgrade NextJS upper than 13.4.7 because of this bug

@glenngijsberts
Copy link
Contributor

@pekarja5 could you please update the description of this PR?

@timneutkens sorry for tagging again, but any chance you can give some guidance on how to test this (personally I don't know)?

@pekarja5
Copy link
Contributor Author

@glenngijsberts Some changes in particular?

@pekarja5 pekarja5 changed the title Allow dynamic i18n output for public directory Fix(i18n): Inaccessible public folder on non-default domains Nov 10, 2023
@glenngijsberts
Copy link
Contributor

glenngijsberts commented Nov 13, 2023

@pekarja5 like Tim said, follow the PR template please. You can start creating a PR (without actually creating it) to see what the template is, or in this file.

@pekarja5
Copy link
Contributor Author

@glenngijsberts the PR template wants me to describe the problem it solves and reference the issue it fixes, to contain tests and to attach a link for errors.
The issue is already linked, problem is described, tests are not present, because i cannot see a way to test this particular case and no errors are introduced by the changes.

So the thing this PR is missing is the test.

@pekarja5 pekarja5 requested a review from timneutkens November 24, 2023 11:15
@aledovskikh
Copy link

aledovskikh commented Dec 12, 2023

timneutkens Any progress of releasing this fix for issue? It is probably critical bug for projects with I18n.
Same PR here #59773

@persocon
Copy link

persocon commented Jan 9, 2024

lol, just upgraded my nextjs version to 14 and now i'm having this exact problem, when will this PR be updated and merged? 🙏

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, this would match all locales and not jut the locale domains/default locale. Continuing fix/tests in #60749

@ijjk ijjk closed this in #60749 Jan 17, 2024
@ijjk ijjk closed this in 8e216ec Jan 17, 2024
@github-actions github-actions bot added the locked label Feb 1, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public files not accessible on i18n domains
10 participants