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

Use original_fullpath for request path #112

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Use original_fullpath for request path #112

merged 1 commit into from
Feb 21, 2024

Conversation

thomasleese
Copy link
Contributor

This changes the event to get the request path from the original_fullpath method rather than the path method. I'm making this change as the original_fullpath represents the path of the page as requested by the user, not necessarily the page being rendered.

In most cases the original path and the path will be the same, but there are some scenarios where it is. Specifically, when rendering a custom error page in Rails which is mounted at /404 the path ends up being the value /404 rather than the path which triggered the 404 page.

This changes the event to get the request path from the
`original_fullpath` method rather than the `path` method. I'm making
this change as the `original_fullpath` represents the path of the page
as requested by the user, not necessarily the page being rendered.

In most cases the original path and the path will be the same, but there
are some scenarios where it is. Specifically, when rendering a custom
error page in Rails which is mounted at `/404` the path ends up being
the value `/404` rather than the path which triggered the 404 page.
@thomasleese thomasleese requested a review from asatwal February 21, 2024 08:51
Copy link
Collaborator

@asatwal asatwal left a comment

Choose a reason for hiding this comment

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

Looks Good @thomasleese 👍

@thomasleese thomasleese merged commit 735932a into main Feb 21, 2024
7 checks passed
@thomasleese thomasleese deleted the full-path branch February 21, 2024 12:38
@thomasleese
Copy link
Contributor Author

Thanks @asatwal - I've merged this in. I don't think it needs to be released any time soon so it can wait until the next release.

@ericaporter ericaporter mentioned this pull request Feb 29, 2024
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.

2 participants