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: Ignore non-HTML responses in storePreviousURL #6012

Merged
merged 5 commits into from
May 31, 2022

Conversation

tearoom6
Copy link
Contributor

@tearoom6 tearoom6 commented May 21, 2022

Description

In this PR, I added an exclusion condition for storePreviousURL: "Response Content-Type is not text/html".

Background

In our application, some images are composited in the CodeIgnitor controller and returned.

The request URL for such case is saved as _ci_previous_url because it does not meet any of the exclusion conditions of the existing storePreviousURL implementation, and when redirecting back, etc., there is a case where the request is redirected to the URL of the image.

The URL to store as previous_url is usually considered to be an HTML (text/html) case, so I have added that as a condition.

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

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 was worried this would be too restrictive because there might be some served content of a variant type. A little research suggests that the content type should actually be this restrictive:

text/html: All HTML content should be served with this type.

I would like to see a tests added to confirm & exempt this - it should be fairly easy, copying what we already have.

@kenjis kenjis added tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. bug Verified issues on the current code behavior or pull requests that will fix them labels May 21, 2022
@kenjis
Copy link
Member

kenjis commented May 21, 2022

This fix seems to be reasonable. The image URL is not a page.

previous_url([$returnObject = false])
Returns the full URL (including segments) of the page the user was previously on.
https://codeigniter4.github.io/CodeIgniter4/helpers/url_helper.html

But this changes the behavior of storePreviousURL() and previous_url(), so
it is better to add at least the changelog.
https://codeigniter4.github.io/CodeIgniter4/changelogs/v4.2.0.html#breaking

@kenjis
Copy link
Member

kenjis commented May 21, 2022

@tearoom6
Thank you for sending this PR.

We expect all contributions to conform to our style guide, be commented (inside the PHP source files), be documented (in the user guide), and unit tested (in the test folder).
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

The user guide and unit testing are missing.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#unit-testing for unit testing.
Could you add them?

@tearoom6 tearoom6 force-pushed the fix/previous_url_only_html branch from 74a11bc to 2fe0228 Compare May 30, 2022 18:35
@tearoom6
Copy link
Contributor Author

@MGatner @kenjis
Thank you for your feedbacks! I have added the unit test case and updated the documentation.
In addition, the original commit was not signed, so I also added signature to the commit. (forced push was needed to do so.)

I would appreciate it if you could review it again.

@@ -33,6 +33,7 @@ Breaking Changes
****************

- The ``system/bootstrap.php`` file no longer returns a ``CodeIgniter`` instance, and does not load the ``.env`` file (now handled in ``index.php`` and ``spark``). If you have code that expects these behaviors it will no longer work and must be modified. This has been changed to make `Preloading <https://www.php.net/manual/en/opcache.preloading.php>`_ easier to implement.
- ``previous_url()`` has been changed to return only the URLs whose Content-Type was ``text/html``. Accordingly, the behavior of ``redirect()->back()`` has been changed.
Copy link
Member

Choose a reason for hiding this comment

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

In the upgrade note, we write what to do when upgrading.
Since it seems unlikely that any developers were using non-HTML URLs, is there no need to list it?

Copy link
Contributor Author

@tearoom6 tearoom6 May 31, 2022

Choose a reason for hiding this comment

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

I see, I understood. I have removed it from the upgrade note 👍

@kenjis kenjis removed tests needed Pull requests that need tests docs needed Pull requests needing documentation write-ups and/or revisions. labels May 31, 2022
@tearoom6 tearoom6 force-pushed the fix/previous_url_only_html branch from 3925228 to a1e0804 Compare May 31, 2022 01:00
@kenjis
Copy link
Member

kenjis commented May 31, 2022

@tearoom6 Thank you for updating.

The PHPStan errors are not related to this PR.

But these errors was fixed in the latest develop.
So if you rebase this branch, all GitHub Action checks would pass.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#pushing-your-branch

There is no conflict, so we can merge this PR if you don't rebase.

@tearoom6 tearoom6 force-pushed the fix/previous_url_only_html branch from a1e0804 to a86e392 Compare May 31, 2022 03:33
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM!

@tearoom6
Copy link
Contributor Author

Thank you for reviewing and the advices!

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.

Very nice work! Thank you for your contribution.

@MGatner MGatner merged commit cae264a into codeigniter4:develop May 31, 2022
@tearoom6 tearoom6 deleted the fix/previous_url_only_html branch May 31, 2022 10:53
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants