search

tenancy / multi-tenant

Run multiple websites using the same Laravel installation while keeping tenant specific data separated for fully independent multi-domain setups, previously github.com/hyn/multi-tenant
https://tenancy.dev
MIT License
2.56k stars 394 forks source link

url() helper ignoring port when update-app-url setting is true #905

Open Bibendus83 opened 4 years ago

Bibendus83 commented 4 years ago

I have my dev environment working on a port different from 80/443. If I set update-app-url in tenancy.php to true all my url() helper calls return the url without including the port. Checking Hyn\Tenancy\Listeners\URL\UpdateAppUrl force() method I noticed that the url rewrite does not include the port $url = sprintf('%s://%s', $scheme, $hostname->fqdn);

Shouldn't the port be included?

ArlonAntonius commented 4 years ago

Hi there,

This is a known issue and is something that is simply not supported in the package at this moment. We really work with URL's and not with ports.

webard commented 4 years ago

@ArlonAntonius but the port is part of URL...

bkintanar commented 4 years ago

@webard FQDN != URL image

Bibendus83 commented 4 years ago

but we are talking about the url() helper so it should contain a full URL not only the FQDN

ArlonAntonius commented 4 years ago

Hi @Bibendus83,

Tenant's have an FQDN that is used for the URL and is excluding a port. This is something that is known and it's simply not something that we support at this moment.

Bibendus83 commented 4 years ago

It's not a question of supporting it or not, using this setting causes an error on the default laravel helper. Shouldn't it be fixed?

ArlonAntonius commented 4 years ago

What exactly is the error? Could you provide a stacktrace? What is the fqdn set for the tenant?

fletch3555 commented 4 years ago

I have a few thoughts on this. I definitely understand where @Bibendus83 is coming from. It's not necessarily that the functionality is "wrong" as much as it's "counter-intuitive"

In general, modern websites/webapps really shouldn't live on any port other than 80/443. If you need to (for the sake of supporting multiple sites/apps on the same host, then that can easily be done with domain-based vhosts in the webserver(apache/nginx/etc) instead of by port. The backend(php/laravel) layer of your application shouldn't care what port it's running at. The exception to this is if you're attempting to distinguish your tenants by port number, but that doesn't appear to fit your requirements (and is honestly a pretty terrible idea in general).

Having said that, I don't entirely disagree with @Bibendus83. If we claim to support dynamically updating the app-url config, then we should only replace the portion of it that corresponds to the FQDN (i.e. leave any provided/configured port alone). We should be able to dynamically support these kinds of use-cases.

Bibendus83 commented 4 years ago

Of course in production port 80/443 will be used. My main issue is that I wasted a couple of days to understand what was the cause of the problem on my dev environment. By default my docker environment runs on port 8000 (and I can't change it because I'm running another apache service on 80), so I could access my site only using that port (ex. http://www.mysite.dev:8000). However all internal links that laravel was creating where without the :8000 part, causing problems with direct links, login pages, etc. So what I ask is, if the site is originally hosted on a different port from 80/443, why should it be forgotten while forcing the FQDN?

Bibendus83 commented 4 years ago

I would add that I had some issues with email notifications sent by jobs. To fix the problem I had to enable the update-app-url setting, however it breaks all links in my dev environment. That's why I created PR #951 to fix it.