inertia-laravel icon indicating copy to clipboard operation
inertia-laravel copied to clipboard

fix duplicates app URL in the URL

Open akramghaleb opened this issue 2 years ago • 2 comments

fix duplicates app URL in the URL

akramghaleb avatar Sep 26 '22 06:09 akramghaleb

if you are working with a subdirectory in Laravel you will see this issue. Please fix it in the next version.

akramghaleb avatar Sep 26 '22 06:09 akramghaleb

possible duplicate of the following: https://github.com/inertiajs/inertia-laravel/pull/437, https://github.com/inertiajs/inertia-laravel/issues/421 and https://github.com/inertiajs/inertia-laravel/pull/422

SuperDJ avatar Sep 27 '22 06:09 SuperDJ

Sorry, I'm not sure we would merge a commented piece of code that says "old, incorrect code". Why was it there in the first place? What purpose did it serve?

taylorotwell avatar Dec 22 '23 14:12 taylorotwell

Sorry, I'm not sure we would merge a commented piece of code that says "old, incorrect code". Why was it there in the first place? What purpose did it serve?

My brother, it doesn't take a genius to figure this out. Give or take 60 seconds of searching maximum and you would see it's there because you guys committed some busted code at https://github.com/inertiajs/inertia-laravel/pull/347 that yall have yet to fix.

taylordragoo avatar Jan 09 '24 13:01 taylordragoo

Sorry, I'm not sure we would merge a commented piece of code that says "old, incorrect code". Why was it there in the first place? What purpose did it serve?

My brother, it doesn't take a genius to figure this out. Give or take 60 seconds of searching maximum and you would see it's there because you guys committed some busted code at https://github.com/inertiajs/inertia-laravel/pull/347 that yall have yet to fix.

It wasn't introduced in that PR; if you looked a bit closer, it was introduced as part of https://github.com/inertiajs/inertia-laravel/pull/333

In either case, despite being inactive / non-involved with Inertia for quite some time now and therefore reluctant to merge PRs at all, I honestly wouldn't just merge any PR like this in either case. You state something is broken, and I've seen some other identical reports and PRs on it just now, but the reality is that for over 99% of users it works just fine.

If you or whomever really needs a fix for this, please do show us a failing test instead of just having a PR with a fix. Something that beyond reasonable doubt displays that this is an issue with the adapter, and not just a misconfigured Nginx/webserver configuration, because regardless of whether we merge this or not, someone else's going to come along tomorrow with a different bug, that line's going to change again, and we'll be right back where we started.

It's easy to fix and merge something reproducible, but if as a maintainer you're not experiencing the issue and the reproduction is difficult or requires an unknown amount of active time investment to even just reproduce, the reality is that it's just not even going to get a lot of attention unless it either affects a significant amount of users frequently, or unless the maintainer has found enough time to try and resolve these kinds of difficult minor issues.

claudiodekker avatar Jan 31 '24 23:01 claudiodekker

Sorry, I'm not sure we would merge a commented piece of code that says "old, incorrect code". Why was it there in the first place? What purpose did it serve?

My brother, it doesn't take a genius to figure this out. Give or take 60 seconds of searching maximum and you would see it's there because you guys committed some busted code at #347 that yall have yet to fix.

It wasn't introduced in that PR; if you looked a bit closer, it was introduced as part of #333

In either case, despite being inactive / non-involved with Inertia for quite some time now and therefore reluctant to merge PRs at all, I honestly wouldn't just merge any PR like this in either case. You state something is broken, and I've seen some other identical reports and PRs on it just now, but the reality is that for over 99% of users it works just fine.

If you or whomever really needs a fix for this, please do show us a failing test instead of just having a PR with a fix. Something that beyond reasonable doubt displays that this is an issue with the adapter, and not just a misconfigured Nginx/webserver configuration, because regardless of whether we merge this or not, someone else's going to come along tomorrow with a different bug, that line's going to change again, and we'll be right back where we started.

It's easy to fix and merge something reproducible, but if as a maintainer you're not experiencing the issue and the reproduction is difficult or requires an unknown amount of active time investment to even just reproduce, the reality is that it's just not even going to get a lot of attention unless it either affects a significant amount of users frequently, or unless the maintainer has found enough time to try and resolve these kinds of difficult minor issues.

Yes, I am well aware you all do not typically use IIS as your web server when developing Inertia apps because if you did you too would know this issue is not because of a misconfigured IIS web server and per the other half dozen issues that have been submitted regarding this exact bit of code, it's fairly clear that this isn't an isolated incident or a configuration mistake.

The issue at hand directly impacts those of us using IIS as our web server, which, while may not be the majority, still constitutes a significant portion of your user base deserving of support. I understand the constraints you face as maintainers, but I encourage you to consider the inclusivity of your support for various environments.

And yeah sure to assist in this, I am ready to put in the work necessary to provide detailed logs, configurations, and even a minimal reproducible example that showcases how this bug specifically affects those of us using IIS as our web server.

However, I'd like to point out that among the half dozen issues already submitted on this matter, there already exists a wealth of information that highlights the exact nature of this problem. These submissions include error logs, configuration details, and some even have steps to reproduce the issue, all of which could easily serve as your starting point for a deeper investigation without any additional information from me or anyone else for that matter.

taylordragoo avatar Feb 01 '24 11:02 taylordragoo

Yes, I am well aware you all do not typically use IIS as your web server when developing Inertia apps because if you did you too would know this issue is not because of a misconfigured IIS web server and per the other half dozen issues that have been submitted regarding this exact bit of code, it's fairly clear that this isn't an isolated incident or a configuration mistake.

This is the first mention I see of IIS being the issue. There's not a single issue/PR that mentions IIS. The only comment about IIS is this one; https://github.com/inertiajs/inertia-laravel/issues/359#issuecomment-1146374973 and it only speaks about having tried multiple servers

Thing is, yes, there are many issues being reported, but it's difficult to replicate, as many don't include basic steps to replicate the issue (things such as nginx config that's used, or a reverse proxy being used). That would also need to be replicated in tests, to make sure it doesn't break existing applications that just run fine.

Also, if it's such a big issue, it's possible to rebind the ResponseFactory, overwrite the render method and implement your own response. If that solves it for a small group of people, that might be a good approach too, as hosting an app in a sub-directory is a rather specific/niche use-case. Inertia might just choose to not support that.

RobertBoes avatar Feb 01 '24 11:02 RobertBoes

Yes, I am well aware you all do not typically use IIS as your web server when developing Inertia apps because if you did you too would know this issue is not because of a misconfigured IIS web server and per the other half dozen issues that have been submitted regarding this exact bit of code, it's fairly clear that this isn't an isolated incident or a configuration mistake.

This is the first mention I see of IIS being the issue. There's not a single issue/PR that mentions IIS. The only comment about IIS is this one; #359 (comment) and it only speaks about having tried multiple servers

Thing is, yes, there are many issues being reported, but it's difficult to replicate, as many don't include basic steps to replicate the issue (things such as nginx config that's used, or a reverse proxy being used). That would also need to be replicated in tests, to make sure it doesn't break existing applications that just run fine.

Also, if it's such a big issue, it's possible to rebind the ResponseFactory, overwrite the render method and implement your own response. If that solves it for a small group of people, that might be a good approach too, as hosting an app in a sub-directory is a rather specific/niche use-case. Inertia might just choose to not support that.

I find it surprising that there's a perception of IIS issues being a novel topic when several discussions, albeit not labeled explicitly under IIS, describe symptoms unique to it. The assertion that there's scant mention of IIS is at odds with the reality that users have been flagging related problems, for nearly two years, complete with configurations and potential fixes, only to be met with silence.

It's concerning that the response to a known issue is to suggest a workaround that requires users to delve into framework internals. This isn't just about IIS or hosting in sub-directories; it's about acknowledging and addressing user-reported issues rather than suggesting band-aid solutions that sidestep the root problem.

If thorough issue reports are what's needed, ask for them. Many of us are ready to provide detailed logs, steps to reproduce, and configurations, some have already even done that. Ignoring these offers and leaving issues unaddressed for two years isn't a solution. My vote would be for the maintainers to take a more proactive approach to tackle this, rather than relegating it to the 'too hard to figure out' basket.

taylordragoo avatar Feb 01 '24 12:02 taylordragoo

Thanks for the PR, and I'm sorry for the delay! Unfortunately, this PR would have re-introduced the bug that #333 was trying to fix, so I've merged #592 which should address both use cases.

jessarcher avatar Mar 08 '24 01:03 jessarcher