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

rework: URI creation and URL helper #7282

Merged
merged 34 commits into from
Aug 16, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Feb 20, 2023

Needs #7252, #7256, #7297

Description
Fixes #7123, #7296, #7643

  • Fixes
    • $this->request->getUri() returns a normalized current URI (SiteURI)
      • if $indexPage is index.php, the URI contains index.php
    • [BC] $this->request->getUri()->getPath() returns full URI path
    • current_url(true) returns the same instance with $this->request->getUri()
    • base_url() and site_url() now support protocol-relative links
    • ControllerTestTrait::withUri() updates the Request instance, too. Because the Request instance has the URI instance. See https://forum.codeigniter.com/showthread.php?tid=88213
  • use SiteURIFactory, SiteURI
  • add Services::siteurifactory()
  • add Services::superglobals()
  • URL helper
    • add SiteURI::baseUrl() and SiteURI::siteUrl()
    • remove _get_uri()
    • site_url(), base_url(), current_url() use SiteURI (Services::request()->getUri())

Before:
before
After:
after

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 marked this pull request as draft February 20, 2023 09:01
@kenjis kenjis added 4.4 refactor Pull requests that refactor code labels Feb 20, 2023
@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities labels Feb 21, 2023
@kenjis kenjis force-pushed the refactor-URI-creation branch 2 times, most recently from 4463439 to 43e8df1 Compare February 23, 2023 07:18
@kenjis kenjis mentioned this pull request Feb 23, 2023
5 tasks
@kenjis kenjis changed the title refactor: URI creation and URL helpers refactor: URI creation and URL helper Feb 24, 2023
@kenjis kenjis changed the title refactor: URI creation and URL helper rework: URI creation and URL helper Feb 24, 2023
@kenjis kenjis force-pushed the refactor-URI-creation branch 2 times, most recently from bb7229b to 257357a Compare February 26, 2023 06:52
@kenjis
Copy link
Member Author

kenjis commented Feb 26, 2023

All checks passed!

However, to be honest I am a little concerned about whether the existing Controller/HTTP tests will be broken.

@kenjis
Copy link
Member Author

kenjis commented Mar 27, 2023

Rebased.

@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 25, 2023
@kenjis kenjis force-pushed the refactor-URI-creation branch 6 times, most recently from b47fa76 to fc6c3a6 Compare July 28, 2023 04:03
@kenjis
Copy link
Member Author

kenjis commented Aug 16, 2023

I'm a little nervous how many test changes there are. It looks like the only real assertion changes are leading / which hopefully are just internal and won't affect projects.

I expect few tests to fail unless you test the URI by setting the state of dependent objects and/or global variables, as CI4 does.

I don't think this is such an unrealistic expectation, since in a normal web app, site URIs are rarely tested.

@kenjis
Copy link
Member Author

kenjis commented Aug 16, 2023

Thank you for reviews!

@MGatner
Copy link
Member

MGatner commented Aug 20, 2023

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 breaking change Pull requests that may break existing functionalities 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.

None yet

3 participants