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 route matching issues #5490

Merged
merged 5 commits into from Jan 15, 2023

Conversation

joshhanley
Copy link
Member

@joshhanley joshhanley commented Jan 6, 2023

Fixes #5462, #5467, #5471, #5466, #5470, #5464 and comments on PR #4570.

The original bug

This issue originally started when the persistant middleware feature was added to Livewire.

When this was added, Livewire support for locale prefixes broke, as it was trying to match routes based on the / root path. But localised applications may have a prefix like /en in the path.

A 404 is triggered when the route cannot be matched.

Pasted image 20230106133835

This was fixed in v2.3.14, by first trying to match the root route, and if that doesn't work then it gets the locale from the fingerprint, removes it from the path and tries again.

The bug that was recently fixed

But it still didn't completely fix the issue, due to a bug with the Str::replaceFirst() parameter order, causing it to always return a URL of an empty string "" which is essentially the root URL.

If the root route has authentication on it, and the user isn't logged in, it actually triggers a redirect to the login page, causing Livewire to break completely, as the response is the HTML for the login page.

Pasted image 20230106133703

If there is no authentication though, there are other instances where a NotFoundHttpException can occur (such as if there is no root route), triggering the 404 modal.

You can see an example of this bug in the below screenshot (see ReplaceFirst with bug output).

It takes the current url http://livewire-httpconnectionhandler-demo.test/en/welcome and returns an empty URL.

Pasted image 20230106133145

A fix was recently merged for this in PR #4570 that swapped the order of the parameters in Str::replaceFirst() and the result is shown above (see ReplaceFirst with fix), where the URL correctly has the locale removed.

The above fix works great for anyone using packages like mcamara/laravel-localization so that localisation is handled correctly.

The latest bugs

The fix which corrected the parameter order of Str::ReplaceFirst has uncovered two new bugs, one of which is when running a Laravel/Livewire app in a sub directory, and the other is when using localisation on the root route.

Localisation on the root route

For an app that uses localisation on the root route, it would have a URL like http://livewire-httpconnectionhandler-demo.test/en. This URL currently triggers a 404 with the fix from PR#4570 due to it trying to match the locale and then a forward slash.

This can be fixed by ensuring the forward slash is before the locale instead of after, as noted here.

Hosting in a subdirectory

In the scenario where a Laravel/Livewire application is hosted in a subdirectory, currently the latest release of Livewire (v2.10.8) breaks all Livewire components when a subsequent request is triggered.

First question is, why are people hosting them in subdirectories? As it turns out, there are quite a lot of people who still use Apache, and only have a single web root folder configured on their local machines. Therefore then run all they're apps out of subdirectories. So this issue is mainly impacting their local development environment. But when the deploy their app to a proper domain, it is fine.

So what is happening is, if running an application using a URL such as http://localhost/myapp/mycomponent, any subsequent Livewire requests return a 404, as the route that is trying to be matched is /myapp/mycomponent instead of the path relative to the project of /mycomponent.

Now as to why this bug has started happening.

The reason for this is, now that the Str::replaceFirst() is actually returning a full URL (instead of an empty string), Livewire is making use of this URL to match it against any available routes to try and find the original route (for the persistent middleware feature).

To determine the original route and its assigned middleware, Livewire generates a fake request using the URL and method provided. This request is then used to match against routes in Laravel's router.

But the faked request that gets generated isn't aware of the projects true base path being inside a subdirectory in the main web root folder, as such believes it's base path is the web root folder, causing to try and match the full URI (/myapp/mycomponent) as explained above.

To fix this, we can make use of the real request to Livewire's endpoint to determine the projects actual path and pass these details into the faked request, allowing it to run the match correctly.

Conclusion

This PR fixes both of the above issues. I have tested it locally using a repo that has localisation and another repo being hosted in a subdirectory, ensuring that the bugs do actually occur and that applying this PR fixes them.

I've also made sure persistent middleware still all works as expected.

These issues are related but also distinct, so if required, I can split into two PRs, but felt it was easier just to give all the context in one PR.

I have no idea how to test this, as such haven't added tests to this PR.

Hope this helps!

@joshhanley
Copy link
Member Author

joshhanley commented Jan 6, 2023

Failing tests are unrelated to this change. It turns out fixes in Laravel 9.46.0 have caused these failures laravel/framework#45456, laravel/framework#45492

@Eighke
Copy link

Eighke commented Jan 6, 2023

@joshhanley Tested and it's works well with this fix. :)

@calebporzio
Copy link
Collaborator

Thanks @joshhanley for being so thorough here - this all makes good sense.

I'm tempted to say yep, let's merge this, but I want to make sure we shouldn't just revert the replaceFirst() PR that caused all of this in the first place.

I recall reviewing that PR and admittedly being kind of confused.

As for testing this, yeah - that would be tough.

The most important thing here I think is not re-introducing all this stuff again in V3. Maybe we create a "manual-tests.md" file or something to keep a list of things that in theory should be in the test suite, but can't be? Idk.

Summary:

  • A) Should we just revert the PR that caused this?
  • B) If not, let's merge and tag this
  • C) Maybe as part of this PR we add a "manual-tests.md" file in the tests directory or something

Thanks again @joshhanley!

@Eighke
Copy link

Eighke commented Jan 6, 2023

@calebporzio Reverting the previous replaceFirst PR will not fix this issue. The bug on <= 2.10.7 was just different.

  • On 2.10.8: All AJAX will 404
  • On <= 2.10.7: All AJAX will use the Homepage middewares instead of one from the ref page

So basically replaceFirst didn't create a bug, it just revealed an already existing one by making it worst.

I think it just that nobody noticed the previous one. Most of the application use similar middleware everywhere, but well it's still a bug (and it's not super optimized, it will trigger the Homepage Controller to get the middleware).

@orgabor
Copy link

orgabor commented Jan 9, 2023

@joshhanley as requested:

There is no such thing as a 'proper domain'. Running apps in subdirectories happens in in production as well, for instance behind multi-application authenticating & authorizing reverse proxies, that use the path to decide about the target application. We could probably supply an automatic test for future use.

@adilupu7
Copy link

adilupu7 commented Jan 9, 2023

@joshhanley Tested on IIS hosted Laravel (in subdirectory) and no more 404's :)

@joshhanley
Copy link
Member Author

joshhanley commented Jan 9, 2023

@orgabor thanks! You mentioned previously that this fix didn’t work for you. Is that correct? If so can you provide steps for us to replicate the issue?

@joshhanley
Copy link
Member Author

@Eighke and @adilupu7 ok great! Thanks for testing, much appreciated 😁

@q0015300153
Copy link

Hi, there is a small problem with the test results,
I currently use the dirty method to fix,
But I almost agree that this fix is not correct,
I'm just not sure how to fix it correctly.

First there is the wrong code...

Apache website settings
You can see that using Alias to set URL
This makes the connection URL for this Laravel/Livewire project http://localhost/test
image

Then fix this PR to the project's
\vendor\livewire\livewire\src\Controllers\HttpConnectionHandler.php
image
image

Update \public\.htaccess configuration file to include RewriteBase
image

Update asset_url and app_url of \config\livewire.php configuration file
image
image

Then build livewire Component hello-world

php artisan make:livewire helloWorld

Modify \app\Http\Livewire\HelloWorld.php, add $message
image

Modify \resources\views\livewire\hello-world.blade.php
image

Modify \resources\views\welcome.blade.php
image

Finally open the website http://localhost/test
image

It can be opened normally, but as long as $message is updated, 404 will be displayed
image

Failure reason

Through xdebug tracing, the reason for the error is "The route test could not be found."
Apparently Laravel adds alias /test to the search path for route
image

Dirty workaround

Modify \vendor\livewire\livewire\src\Controllers\HttpConnectionHandler.php,
Add syntax to replace baseUrl with empty
image

The route can now be found correctly, updating $message successfully
image

PS. Since my native language is not English, please let me know if there are any mistakes, thanks

@joshhanley
Copy link
Member Author

@q0015300153 thanks so much for that, it's much appreciated! So detailed, that's great!

I will test it out in the next couple of days and update the PR. 🙂

@andrezanna
Copy link

I've lost my mind yesterday to understand this issue. Not present (for me) with livewire 2.10.7 but if I update it my website is frozen, every component click throws a 404.

@joshhanley
Copy link
Member Author

joshhanley commented Jan 15, 2023

@q0015300153 and @orgabor I've just pushed some new changes that should fix your use cases. I'm going to merge this now anyway, but can you both please test this PR if you get the chance and let me know if there are any other issues? That'd be much appreciated!

The latest changes fix issues with the root route still causing 404s.

For future testing purposes:

  • an app in a subdirectory need to be tested on both the root route '/' and any other route
  • the same needs to be tested for an app with localisation, tested on the root route '/en' and any other route

Update: This has been tagged and released in v2.11.0, if people could test this and let me know if there are any issues. Thanks!

@joshhanley joshhanley merged commit 1ae504e into livewire:master Jan 15, 2023
@joshhanley joshhanley deleted the fix-route-matching-issues branch January 15, 2023 23:58
@q0015300153
Copy link

@joshhanley Just finished testing, no problems~
Thank you for your contribution
I also learned a lot from this case : )

@joshhanley
Copy link
Member Author

@q0015300153 great! Thanks for testing 😄no worries! Glad we manage to get it working

@Eighke
Copy link

Eighke commented Jan 16, 2023

@joshhanley Looks good for me. :)

@CovertError
Copy link

this issue still exists for me on other locale's /en works but /ar or others dont work

@joshhanley
Copy link
Member Author

@CovertError will follow up on your discussion #5526

calebporzio pushed a commit that referenced this pull request Jul 20, 2023
Implementation doesn't currently exist for persistent middleware, so adding this todo for future reference
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.

None yet

8 participants