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: site_url() does not use alt Config #7215

Merged
merged 26 commits into from
Feb 8, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Feb 3, 2023

Description
Supersedes #7211

I need to refactor the three function code because I cannot fix without adding an parameter.

  • fix site_url() does not use alt Config
  • refactor base_url(), site_url(), current_url()
  • change _get_uri()

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them refactor Pull requests that refactor code labels Feb 3, 2023
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Fixing the custom config is fine. However, I'm not a fan of other parts that were refactored.

system/Helpers/url_helper.php Outdated Show resolved Hide resolved
system/Helpers/url_helper.php Outdated Show resolved Hide resolved
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I've only given this a first read. If you and @michalsn feel confident about it feel free to proceed, otherwise I will come back for a more thorough review. Framework URI handling is a sticky business.

@kenjis kenjis merged commit 9c0141a into codeigniter4:develop Feb 8, 2023
@kenjis kenjis deleted the rework-URI-related-code branch February 8, 2023 00:56
@kenjis kenjis changed the title fix: site_url() does not use alt Config and refactoring fix: site_url() does not use alt Config Feb 8, 2023
@kenjis
Copy link
Member Author

kenjis commented Feb 8, 2023

In the end, the code was not that different from the original code.
Also, michalsn has done a careful review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants