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

Refactor URL redirect after login #2270

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

williamjallen
Copy link
Collaborator

Our current redirect logic is messy, and just happens to work for most CDash systems. For CDash-in-a-subdirectory systems, the redirect logic is somewhat broken.

This PR refactors the entire system to consistently apply redirects to all web routes.

@williamjallen williamjallen added this to the v3.5 milestone Jun 12, 2024
@josephsnyder
Copy link
Member

The changes all look good, but don't have an effect on the subpath system as I've set it up. Redirecting to a project page still duplicates the subpath and redirects to / still do not redirect to /projects

@williamjallen
Copy link
Collaborator Author

@josephsnyder are the values of url.intended consistent now at least?

@zackgalbreath
Copy link
Contributor

zackgalbreath commented Jun 17, 2024

A bit more context that I recently discovered:

  1. Laravel officially does not support deploying to a subpath 😱 [1] [2]

  2. Nonetheless, I got the "/" route to work correctly once again for my local bare-metal, CDash-in-a-subpath installation by making the following change to install.sh:

-php artisan route:cache
+php artisan route:clear

@williamjallen
Copy link
Collaborator Author

@zackgalbreath Thanks for digging up that issue! I looks like the issue is specifically with caching of the / route. One option is to simply turn off route caching. Based on my testing a while back, this would probably result in a 2-3% slowdown for every request.

I wonder if this issue is related to the specific configuration you have though. Does clearing your route cache (php artisan route:clear) also fix your system @josephsnyder? Could an Apache RewriteRule be used instead?

@josephsnyder
Copy link
Member

I wonder if this issue is related to the specific configuration you have though. Does clearing your route cache (php artisan route:clear) also fix your system @josephsnyder? Could an Apache RewriteRule be used instead?

@williamjallen, it does. Though, further testing finds that running route:cache causes it to stop working 🤷

@williamjallen
Copy link
Collaborator Author

Since the / route issue is independent of these changes, I created a separate PR to remove the route cache altogether: #2276

@williamjallen williamjallen added this pull request to the merge queue Jun 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 17, 2024
@williamjallen williamjallen added this pull request to the merge queue Jun 17, 2024
Merged via the queue into Kitware:master with commit 058863f Jun 17, 2024
6 checks passed
@williamjallen williamjallen deleted the intended-url-refactor branch June 17, 2024 19:41
williamjallen added a commit to williamjallen/CDash that referenced this pull request Jun 17, 2024
As discussed
[here](Kitware#2270 (comment)),
Laravel's route cache does not work for CDash-in-a-subpath
installations. Since the route cache provides a negligible speedup, this
PR simply removes it for all systems. This fixes an issue observed on
some CDash-in-a-subpath installations where the `/` route does not
behave correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants