fix: [Commands] `routes` with `{locale}` in route #8152

neznaika0 · 2023-11-05T10:52:27Z

Description
Fix #7997

Previously, the command output <unknown> for URLs with {locale} instead of possible filters. Since there can be ~10 language options for one route. The commit adds !filtername to filters, which means probable application, but not guaranteed.

Set $routes->useSupportedLocalesOnly(true) in app/Config/Routes.php

Before:

$ ./spark routes

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-05 10:39:57 UTC+00:00

+--------+--------------------------------+------+------------------------------+----------------+---------------+
| Method | Route                          | Name | Handler                      | Before Filters | After Filters |
+--------+--------------------------------+------+------------------------------+----------------+---------------+
| GET    | /                              | »    | \App\Controllers\Home::index | honeypot csrf  | toolbar       |
| GET    | products/all                   | »    | \App\Controllers\Home::index | honeypot csrf  | toolbar       |
| GET    | products/([0-9]+)/([^/]+)/show | »    | \App\Controllers\Home::index | honeypot csrf  | toolbar       |
| GET    | {locale}/products/(.*)         | »    | \App\Controllers\Home::index | <unknown>      | <unknown>     |
| GET    | products/{locale}              | »    | \App\Controllers\Home::index | <unknown>      | <unknown>     |
| GET    | products/{locale}/([0-9]+)     | »    | \App\Controllers\Home::index | <unknown>      | <unknown>     |
| GET    | {locale}                       | »    | \App\Controllers\Home::index | <unknown>      | <unknown>     |
+--------+--------------------------------+------+------------------------------+----------------+---------------+

After:

$ ./spark routes

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-05 10:44:13 UTC+00:00

+--------+--------------------------------+------+------------------------------+-----------------+---------------+
| Method | Route                          | Name | Handler                      | Before Filters  | After Filters |
+--------+--------------------------------+------+------------------------------+-----------------+---------------+
| GET    | /                              | »    | \App\Controllers\Home::index | honeypot csrf   | toolbar       |
| GET    | products/all                   | »    | \App\Controllers\Home::index | honeypot csrf   | toolbar       |
| GET    | products/([0-9]+)/([^/]+)/show | »    | \App\Controllers\Home::index | honeypot csrf   | toolbar       |
| GET    | {locale}/products/(.*)         | »    | \App\Controllers\Home::index | !honeypot !csrf | !toolbar      |
| GET    | products/{locale}              | »    | \App\Controllers\Home::index | !honeypot !csrf | !toolbar      |
| GET    | products/{locale}/([0-9]+)     | »    | \App\Controllers\Home::index | !honeypot !csrf | !toolbar      |
| GET    | {locale}                       | »    | \App\Controllers\Home::index | !honeypot !csrf | !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 · 2023-11-06T01:50:08Z

Okay, this is an improvement!
Please add tests and documentation.

Then, if useSupportedLocalesOnly(true), it seems that checking all locales would reliably determine if the filters are applied.

neznaika0 · 2023-11-06T02:27:29Z

There is a remark, such a check is only for the command. When used in other places (directly from classes), the filters will be <unknown>.

Tests. I didn't understand what to check? An option without a locale and with it?

Docs. My English is bad, we'll have to figure out what I'm going to write together🤔

kenjis · 2023-11-06T05:15:54Z

At least, a test to check the changed output like !honeypot is needed.

kenjis · 2023-11-06T05:17:09Z

Docs: you need to add changelog (Enhancement), and what the ! means in the output.

system/Commands/Utilities/Routes/FilterFinder.php

kenjis

Sorry. I did not take a good look at what was inside.

Changes to the behavior of the FilterFinder are not acceptable.
Please make the functionality happen without changing the FilterFinder.

kenjis · 2023-11-06T10:18:21Z

system/Commands/Utilities/Routes/FilterFinder.php

@@ -50,6 +51,12 @@ public function find(string $uri): array
    {
        $this->filters->reset();

+        $isLocaleExist = strpos($uri, '{locale}') !== false;


This change is not acceptable.
Because the param $uri is "URI path to find filters for".
{locale}/admin/settings is not a URI path.
The URI path is like ja/admin/settings or fr/admin/settings.

This change will break the filter:check command.

See debug info.

// Routes.php /** * @var RouteCollection $routes */ $routes->useSupportedLocalesOnly(true); $routes->get('/{locale}/home', 'Home::index'); // FilterFinder.php public function find(string $uri): array { d($uri); $this->filters->reset(); // Fix for the search filters command $isSupportedLocaleOnly = false;

Output:

Set ja, fr crash my test.

1) CodeIgniter\Commands\Utilities\Routes\FilterFinderTest::testFindFiltersWithSupportedLocalesOnly Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ Array &0 ( 'before' => Array &1 ( - 0 => '!csrf' + 0 => 'csrf' ) 'after' => Array &2 ( - 0 => '!toolbar' + 0 => 'toolbar' ) )

And filter:checl normal output.
useSupportedLocalesOnly = true

$ ./spark filter:check get en/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 10:41:42 UTC+00:00 +--------+---------+----------------+---------------+ | Method | Route | Before Filters | After Filters | +--------+---------+----------------+---------------+ | GET | en/home | | toolbar | +--------+---------+----------------+---------------+ $ ./spark filter:check get ja/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 10:43:31 UTC+00:00 Can't find a route: "GET ja/home"

useSupportedLocalesOnly = false

$ ./spark filter:check get en/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 10:42:01 UTC+00:00 +--------+---------+----------------+---------------+ | Method | Route | Before Filters | After Filters | +--------+---------+----------------+---------------+ | GET | en/home | | toolbar | +--------+---------+----------------+---------------+ $ ./spark filter:check get ja/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 10:42:03 UTC+00:00 +--------+---------+----------------+---------------+ | Method | Route | Before Filters | After Filters | +--------+---------+----------------+---------------+ | GET | ja/home | | toolbar | +--------+---------+----------------+---------------+

No. FilterFinder's responsibility is to find filters for a URI path.
If you pass {locale}/home to it, it is a URI path of e.g., http://localhost/{locale}/home.
It does not mean ja/home or fr/home.

Don't change the behavior.

I don't understand what the problem is... Is there a specific example with a behavior error?
Now everything is working correctly: with and without the useSupportedLocalesOnly option.

We test route as useSupportedLocalesOnly = true/false:

$ ./spark filter:check get {locale}/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 11:03:00 UTC+00:00 +--------+---------------+----------------+---------------+ | Method | Route | Before Filters | After Filters | +--------+---------------+----------------+---------------+ | GET | {locale}/home | | !toolbar | +--------+---------------+----------------+---------------+ $ ./spark filter:check get {locale}/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 11:03:22 UTC+00:00 +--------+---------------+----------------+---------------+ | Method | Route | Before Filters | After Filters | +--------+---------------+----------------+---------------+ | GET | {locale}/home | | toolbar | +--------+---------------+----------------+---------------+

filter:check should not output !. It must always return actual filters to be applied.

$ php spark filter:check get {locale}/home CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 11:10:54 UTC+00:00 +--------+---------------+----------------+---------------+ | Method | Route | Before Filters | After Filters | +--------+---------------+----------------+---------------+ | GET | {locale}/home | !invalidchars | !toolbar | +--------+---------------+----------------+---------------+

Do not change the meaning of $uri. It is the literal URI path, no meta characters or placeholders.

So this is what we wanted, isn't it? Show POSSIBLE filters for a GROUP of languages. If you check the exact language everything will be fine

It is for routes command. Not for filter:check.
routes will show inaccurate but useful filter info.
But filter:check should always show accurate filter info.

neznaika0 · 2023-11-06T10:51:54Z

Sorry. I did not take a good look at what was inside.

Changes to the behavior of the FilterFinder are not acceptable. Please make the functionality happen without changing the FilterFinder.

Move to system/Commands/Utilities/Routes.php?

neznaika0 · 2023-11-06T12:18:30Z

Okay. I move code in spark routes.
Check results, pls.

Test with $routes->useSupportedLocalesOnly(false)
Test with $routes->useSupportedLocalesOnly(true)

$ ./spark routes

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 12:10:00 UTC+00:00

+--------+---------------+------+------------------------------+----------------+---------------+
| Method | Route         | Name | Handler                      | Before Filters | After Filters |
+--------+---------------+------+------------------------------+----------------+---------------+
| GET    | /             | »    | \App\Controllers\Home::index |                | toolbar       |
| GET    | {locale}/home | »    | \App\Controllers\Home::index |                | toolbar       |
+--------+---------------+------+------------------------------+----------------+---------------+

$ ./spark routes

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 12:10:09 UTC+00:00

+--------+---------------+------+------------------------------+----------------+---------------+
| Method | Route         | Name | Handler                      | Before Filters | After Filters |
+--------+---------------+------+------------------------------+----------------+---------------+
| GET    | /             | »    | \App\Controllers\Home::index |                | toolbar       |
| GET    | {locale}/home | »    | \App\Controllers\Home::index |                | !toolbar      |
+--------+---------------+------+------------------------------+----------------+---------------+

And check routes allowed locale:

$ ./spark filter:check get en/home

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 12:14:37 UTC+00:00

+--------+---------+----------------+---------------+
| Method | Route   | Before Filters | After Filters |
+--------+---------+----------------+---------------+
| GET    | en/home |                | toolbar       |
+--------+---------+----------------+---------------+

$ ./spark filter:check get en/home

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 12:14:45 UTC+00:00

+--------+---------+----------------+---------------+
| Method | Route   | Before Filters | After Filters |
+--------+---------+----------------+---------------+
| GET    | en/home |                | toolbar       |
+--------+---------+----------------+---------------+

Check routes unsupported locale:

$ ./spark filter:check get ja/home

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 12:16:21 UTC+00:00

+--------+---------+----------------+---------------+
| Method | Route   | Before Filters | After Filters |
+--------+---------+----------------+---------------+
| GET    | ja/home |                | toolbar       |
+--------+---------+----------------+---------------+

$ ./spark filter:check get ja/home

CodeIgniter v4.4.3 Command Line Tool - Server Time: 2023-11-06 12:16:28 UTC+00:00

Can't find a route: "GET ja/home"

kenjis · 2023-11-07T10:05:57Z

The following outputs are strange.

~~The first output seems to be a bug. It should be unknown.~~

$routes->useSupportedLocalesOnly(false);

+--------+---------------+------+------------------------------+----------------+---------------+
| Method | Route         | Name | Handler                      | Before Filters | After Filters |
+--------+---------------+------+------------------------------+----------------+---------------+
| GET    | /             | »    | \App\Controllers\Home::index |                | toolbar       |
| GET    | {locale}/home | »    | \App\Controllers\Home::index |                | toolbar       |
+--------+---------------+------+------------------------------+----------------+---------------+

$routes->useSupportedLocalesOnly(true);

+--------+---------------+------+------------------------------+----------------+---------------+
| Method | Route         | Name | Handler                      | Before Filters | After Filters |
+--------+---------------+------+------------------------------+----------------+---------------+
| GET    | /             | »    | \App\Controllers\Home::index |                | toolbar       |
| GET    | {locale}/home | »    | \App\Controllers\Home::index |                | !toolbar      |
+--------+---------------+------+------------------------------+----------------+---------------+

kenjis · 2023-11-07T10:56:09Z

From the start, if the route key contains a regular expression, then the filters in spark routes are not 100% accurate.
In contrast, spark filter:check must be 100% accurate.

So, with the current spec, the output of spark routes, regardless of the value of useSupportedLocalesOnly, should be toolbar, and not !toolbar.

If you want to be precise, you should change all filters for route keys containing regular expressions and {locale} to output like !toolbar.

kenjis · 2023-11-07T10:57:56Z

Until now, I have not correctly identified the problem.

Why don't we just change {locale} in SampleURIGenerator to the value of the default locale?

neznaika0 · 2023-11-07T11:30:58Z

In message #8152 (comment)
First output is standart behavior (without my changes) Because there is no check for valid locales. With the option enabled, we get <unknown> <unknown>
The filter:check command is not affected by this right now.

From the start, if the route key contains a regular expression, then the filters in spark routes are not 100% accurate.

In my opinion, it is normal that regular expressions have a toolbar filter. For a general understanding of this is good.
But when {locale} should not have any string [^/]+ indicates it !toolbar

I don't know what to do anymore. It seems that a small display causes such ambiguities. I think then let another person do it.

kenjis · 2023-11-07T11:42:12Z

I sent a PR #8167. Please try.

neznaika0 · 2023-11-08T19:16:35Z

You can close it. Functionality was not needed

kenjis · 2023-11-08T21:55:39Z

Closed by #8167

kenjis · 2023-11-08T22:41:09Z

I sent PR #8178 to update the docs.

kenjis added 4.5 enhancement PRs that improve existing functionalities docs needed Pull requests needing documentation write-ups and/or revisions. tests needed Pull requests that need tests labels Nov 6, 2023

neznaika0 force-pushed the fix-command-routes-with-locale branch from 4f7d7d0 to ebc2643 Compare November 6, 2023 07:50

neznaika0 commented Nov 6, 2023

View reviewed changes

system/Commands/Utilities/Routes/FilterFinder.php Outdated Show resolved Hide resolved

kenjis requested changes Nov 6, 2023

View reviewed changes

neznaika0 added 6 commits November 8, 2023 22:01

fix: Command routes with {locale} in route

c75b555

docs: Add note about filters

6695480

test: Add test for routes containing {locale}

117a2ee

fix: Correct the check condition

674d875

fix: Move code to routes command

231a748

test: Move code to RoutesTest.php

4e872f7

neznaika0 force-pushed the fix-command-routes-with-locale branch from 544d392 to 4e872f7 Compare November 8, 2023 19:02

kenjis closed this Nov 8, 2023

neznaika0 deleted the fix-command-routes-with-locale branch November 9, 2023 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [Commands] `routes` with `{locale}` in route #8152

fix: [Commands] `routes` with `{locale}` in route #8152

neznaika0 commented Nov 5, 2023

kenjis commented Nov 6, 2023 •

edited

Loading

neznaika0 commented Nov 6, 2023 •

edited

Loading

kenjis commented Nov 6, 2023 •

edited

Loading

kenjis commented Nov 6, 2023

kenjis left a comment •

edited

Loading

kenjis Nov 6, 2023

neznaika0 Nov 6, 2023

neznaika0 Nov 6, 2023 •

edited

Loading

kenjis Nov 6, 2023

neznaika0 Nov 6, 2023

neznaika0 Nov 6, 2023

kenjis Nov 6, 2023 •

edited

Loading

kenjis Nov 6, 2023

neznaika0 Nov 6, 2023

kenjis Nov 6, 2023 •

edited

Loading

neznaika0 commented Nov 6, 2023

neznaika0 commented Nov 6, 2023

kenjis commented Nov 7, 2023 •

edited

Loading

kenjis commented Nov 7, 2023 •

edited

Loading

kenjis commented Nov 7, 2023

neznaika0 commented Nov 7, 2023

kenjis commented Nov 7, 2023

neznaika0 commented Nov 8, 2023

kenjis commented Nov 8, 2023

kenjis commented Nov 8, 2023

fix: [Commands] routes with {locale} in route #8152

fix: [Commands] routes with {locale} in route #8152

Conversation

neznaika0 commented Nov 5, 2023

kenjis commented Nov 6, 2023 • edited Loading

neznaika0 commented Nov 6, 2023 • edited Loading

kenjis commented Nov 6, 2023 • edited Loading

kenjis commented Nov 6, 2023

kenjis left a comment • edited Loading

Choose a reason for hiding this comment

kenjis Nov 6, 2023

Choose a reason for hiding this comment

neznaika0 Nov 6, 2023

Choose a reason for hiding this comment

neznaika0 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

kenjis Nov 6, 2023

Choose a reason for hiding this comment

neznaika0 Nov 6, 2023

Choose a reason for hiding this comment

neznaika0 Nov 6, 2023

Choose a reason for hiding this comment

kenjis Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

kenjis Nov 6, 2023

Choose a reason for hiding this comment

neznaika0 Nov 6, 2023

Choose a reason for hiding this comment

kenjis Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

neznaika0 commented Nov 6, 2023

neznaika0 commented Nov 6, 2023

kenjis commented Nov 7, 2023 • edited Loading

kenjis commented Nov 7, 2023 • edited Loading

kenjis commented Nov 7, 2023

neznaika0 commented Nov 7, 2023

kenjis commented Nov 7, 2023

neznaika0 commented Nov 8, 2023

kenjis commented Nov 8, 2023

kenjis commented Nov 8, 2023

fix: [Commands] `routes` with `{locale}` in route #8152

fix: [Commands] `routes` with `{locale}` in route #8152

kenjis commented Nov 6, 2023 •

edited

Loading

neznaika0 commented Nov 6, 2023 •

edited

Loading

kenjis commented Nov 6, 2023 •

edited

Loading

kenjis left a comment •

edited

Loading

neznaika0 Nov 6, 2023 •

edited

Loading

kenjis Nov 6, 2023 •

edited

Loading

kenjis Nov 6, 2023 •

edited

Loading

kenjis commented Nov 7, 2023 •

edited

Loading

kenjis commented Nov 7, 2023 •

edited

Loading