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: spark routes shows incorrect hostname routes #7176

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jan 25, 2023

Description
From https://forum.codeigniter.com/showthread.php?tid=86264

HTTP_HOST do not exist in CLI, so routes for hostnames cannot currently be displayed by spark routes.

It is strange that another.domain is shown as preferred.

$routes->get('/', 'Controller::default');
$routes->get('/', 'Controller::another', ['hostname' => 'another.domain']);
$routes->get('/', 'Controller::subdomain', ['subdomain' => 'subdomain']);

Before:

+--------+-------+------+--------------------------------------+----------------+---------------+
| Method | Route | Name | Handler                              | Before Filters | After Filters |
+--------+-------+------+--------------------------------------+----------------+---------------+
| GET    | /     | »    | \App\Controllers\Controller::another |                | toolbar       |
+--------+-------+------+--------------------------------------+----------------+---------------+

After:

+--------+-------+------+--------------------------------------+----------------+---------------+
| Method | Route | Name | Handler                              | Before Filters | After Filters |
+--------+-------+------+--------------------------------------+----------------+---------------+
| GET    | /     | »    | \App\Controllers\Controller::default |                | toolbar       |
+--------+-------+------+--------------------------------------+----------------+---------------+

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 25, 2023
@iRedds
Copy link
Collaborator

iRedds commented Jan 25, 2023

I think it should be like this

| GET    | /                    | »    | \App\Controllers\Controller::default   |                | toolbar       |
| GET    | another.domain/      | »    | \App\Controllers\Controller::another   |               | toolbar       |
| GET    | subdomain.localhost/ | »    | \App\Controllers\Controller::subdomain |                | toolbar       |

@kenjis
Copy link
Member Author

kenjis commented Jan 25, 2023

@iRedds I too think all routes should be shown by the command.
But it is not possible now. We need the enhancement.

@iRedds
Copy link
Collaborator

iRedds commented Jan 25, 2023

@kenjis It's not just the command.
Now routes that don't match HTTP_HOST are ignored. That is, for example, you cannot get the route path through the route_to() function if the domain does not match the current one.

@kenjis
Copy link
Member Author

kenjis commented Jan 26, 2023

@iRedds I don't know a use case that route_to() (or url_to()) with other hostname is needed.

To begin with, I don't really see the need for this limit to host/domain feature itself, which changes the route only for certain hostnames.

In what case would it be used? Do you have real use cases?

@iRedds
Copy link
Collaborator

iRedds commented Jan 26, 2023

The framework allows that one application can serve different domains. Correctly? So it makes sense that this application should be able to generate links to different domains.

For example, "our" application consists of several parts.
example.com - landing page
app.example.com is the private part of the application.

And it is quite possible to use cross-references between the landing page and the private part.

Or, for example, we need to generate a url like user.example.com or locale.example.com

@kenjis
Copy link
Member Author

kenjis commented Jan 27, 2023

@iRedds Do you have a real use case of multiple domains with different routes in a app?
Do you really want the functionality?
https://codeigniter4.github.io/CodeIgniter4/incoming/routing.html#limit-to-hostname
Why don't you have two apps if you serve two domains?

@iRedds
Copy link
Collaborator

iRedds commented Jan 27, 2023

I don't have the case you describe.
Good question. Two domains = two applications.
But I have a question.
Why is such functionality as serving multiple domains with one application added to the framework?

@michalsn
Copy link
Member

I think we need an enhancement or alternative to the route_to() and url_to(), to support the domain part of the URL.

Do you have a real use case of multiple domains with different routes in a app?

I used it in the SaaS app. Very handful.

Why is such functionality as serving multiple domains with one application added to the framework?

There was a demand from developers for this. Personally, I think this is quite a common "problem" - a lot of services use multiple domains for certain areas of the application.

Since we support different hostnames/subdomains in the routes, being not able to generate this type of route was always a lacking feature for me.

@kenjis
Copy link
Member Author

kenjis commented Jan 30, 2023

a lot of services use multiple domains for certain areas of the application.

For what? Example?

@michalsn
Copy link
Member

michalsn commented Jan 30, 2023

I think iRedds already gave pretty valid examples.

If I have something like: user.example.com there is a big chance that the routes used here won't be valid for example.com, because this is a different part of the site/service.

@kenjis
Copy link
Member Author

kenjis commented Jan 30, 2023

iRedds example is just an imagination.
I want to know real use case to understand the needs.

Why don't you have two apps user.example.com and example.com?

If I have something like: user.example.com there is a big chance that the routes used here won't be valid for example.com,

In the current implemention,
all the routes for example.com (w/o host/domain limitation) are valid to user.example.com.
Is that okay?

@michalsn
Copy link
Member

Why don't you have two apps user.example.com and example.com?

We should not require specific architecture from our users. Besides, different domains do not necessarily mean we will deal with different applications.

Example. A "social network" app built around sports activities. People are engaging in conversation in the main domain example.com and can plan a bike route, but my-username-goes-here.example.com is my user's unique, public address where I can view my activities, others can send me a DM or so...

Another example would be different departments in a company, which are under other subdomains. Some parts of the application are common, but others have special modules. Of course, whether these should be separate applications or not is open, but again, that's probably not our role. Anyway, separate applications will not always be justified, and a different domain does not always mean a separate service.

In the current implemention,
all the routes for example.com (w/o host/domain limitation) are valid to user.example.com.
Is that okay?

To be clear, something like that?

$routes->group('', static function ($routes) {
    $routes->('something', 'Whatever::something');
});

$routes->group('', ['hostname' => 'user.example.com'], static function ($routes) {
    $routes->('else', 'Whatever::else');
});

Then accessing https://user.example.com/something should be valid.
If there is no host/domain limitation specified, this behavior is okay.

@kenjis kenjis merged commit 9a59d99 into codeigniter4:develop Jan 30, 2023
@kenjis kenjis deleted the fix-spark-routes-hostname branch January 30, 2023 22:39
@kenjis
Copy link
Member Author

kenjis commented Jan 30, 2023

Yes, something like that.

In your example my-username-goes-here.example.com,
if there are routes:

the following URLs seem to be accessible:

It seems odd to me.

Also, if a different department runs a site on a different subdomain, each time a route with no host limitation is added on the main site, the URL for that department's site will also be added automatically.

@michalsn
Copy link
Member

Yes, that's true, but the solution for this is very simple. We have to declare the hostname for every route group definition. This way, everything will be separated correctly.

@kenjis
Copy link
Member Author

kenjis commented Jan 31, 2023

Okay, thanks.

Then the current structure of the $routes array does not look good.
It seems better to add the hostname (regex) as a key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants