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

URL Functions #4675

Merged
merged 4 commits into from
May 13, 2021
Merged

URL Functions #4675

merged 4 commits into from
May 13, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 11, 2021

Description
The next stage of reworking URI handling, this PR refactors the URL Helper's *_url() functions to use a new internal helper, _get_uri(). This should a) make clearer what each function does, and b) fix a few bugs, including the "big one" #4116.

Due to #4116 I expect there to be failures from dependent components. This will give some idea of the impact this could have on developers who have been expecting current_url() to leave off indexPage, even though that creates invalid project URLs. In anticipation of this upset we will need some loud screaming in the changelog and release notes.

Note: This relies on #4672 and will need to be rebased after that is merged.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

I agree we need to shout about this one, but it fixes what needs to be fixed. Thanks.

I don't see any issues with it.

@MGatner
Copy link
Member Author

MGatner commented May 13, 2021

@lonnieezell See if you think these User Guide changes are appropriate and sufficient.

@MGatner MGatner requested a review from paulbalandan May 13, 2021 13:47
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Yes, I think those changes are sufficient. Will want to include info about that in our release notice, too.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

We should also document the changes in the function signatures.

@MGatner
Copy link
Member Author

MGatner commented May 13, 2021

@paulbalandan I already updated the function comments to match what it now does a bit more accurately. Did you mean something like, "Note: this function has changed to..."?

@paulbalandan
Copy link
Member

I meant the documentation in url_helper.rst

@MGatner
Copy link
Member Author

MGatner commented May 13, 2021

@paulbalandan Something like this?

@paulbalandan
Copy link
Member

@paulbalandan Something like this?

Ah I see it now. The insertion of the _get_uri function complicated the diff views. I thought the signatures changed.

Oh, the current_url function has now a nullable second param. We should update that in the docs.

@MGatner
Copy link
Member Author

MGatner commented May 13, 2021

Good catch! Added.

@MGatner MGatner requested a review from paulbalandan May 13, 2021 15:02
@MGatner MGatner merged commit 43e0e96 into codeigniter4:develop May 13, 2021
@MGatner MGatner deleted the get-uri branch May 13, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants