Preserve full page path in App::url() method

[mvorisek]

if Atk4 is used with pretty urls routing, App::url in case of legitimate path like https://domain/admin/users/ returns https://domain/admin/users/index

are you sure, isn't .../index.php returned?

there was something in 4.0, removed in actual development.

please point to an exact commit/PR

and/or is something wrong with https://github.com/atk4/ui/pull/2065?

[abbadon1334]

and/or is something wrong with #2065?

At the moment i see only the dropping of App::$page in this commit as a problem in case of routing

NOT using php routing

you are right about returning /index.php is the default behaviour of nearly every webserver but there is a directive configuration for every webserver:

• Apache https://httpd.apache.org/docs/2.4/mod/directive-dict.html#Description

• Nginx http://nginx.org/en/docs/http/ngx_http_index_module.html

A standard Atk4 App using App::$urlBuildingPage and App::$urlBuildingExt can be set the correct return of App::url even for non standard configuration

using php routing

If you put in front of Atk for example https://github.com/nikic/FastRoute will manage the application routing.

Excepted results: App::url('/') = '/' App::url('/admin/users/') = '/admin/users/'

Actual results: App::url('/') = '/index' App::url('/admin/users/') = '/admin/users/index'

Conclusion

I think the best option is not dropping it , but leave full control of default Index returning from building url, remove the needs of extend the App only to override one line in App::url

[mvorisek]

and/or is something wrong with #2065?

At the moment i see only the dropping of App::$page in this commit as a problem in case of routing

NOT using php routing

so App::$page was possible to be set as the newly introduced urlBuildingPage?

you are right about returning /index.php is the default behaviour of nearly every webserver but there is a directive configuration for every webserver:

* Apache https://httpd.apache.org/docs/2.4/mod/directive-dict.html#Description

* Nginx http://nginx.org/en/docs/http/ngx_http_index_module.html

AFAK the only issue is with php internal webserver as /virtual_route is not supported if not routed via single script.

But I agree, badly configured Apache/nginx should still be supported.

-> as both major webserver name it "index", the property should be named with "index" too

-> what is your opinion on empty string to be the default?

A standard Atk4 App using App::$urlBuildingPage and App::$urlBuildingExt can be set the correct return of App::url even for non standard configuration

I think the best option is not dropping it , but leave full control of default Index returning from building url, remove the needs of extend the App only to override one line in App::url

-> needs phpunit test

[abbadon1334]

so App::$page was possible to be set as the newly introduced urlBuildingPage?

LGTM

[abbadon1334]

AFAK the only issue is with php internal webserver as /virtual_route is not supported if not routed via single script.

But I agree, badly configured Apache/nginx should still be supported.

-> as both major webserver name it "index", the property should be named with "index" too

i think default must be page=index ext=.php to give excepted result (index.php) for previous version + badly configured web server as you pointed.

-> what is your opinion on empty string to be the default?

i think the correct default value is index

[abbadon1334]

-> needs phpunit test

i think is needed, i proceed adding functional test with App::url no behat or webserver only checking excepted output. What do you think?

[abbadon1334]

@mvorisek, I may have gone a bit overboard with the test cases, but during the process, I discovered some unexpected behavior that didn't give me enough confidence to close the PR. As a result, I've refactored the entire App::url method. I found that it was becoming too complex, so I've broken it down into multiple private sub-methods for better readability and maintainability.

Additionally, I believe I've resolved an issue where, under certain conditions, the URL returned was missing the relative path.

i see the demos are broken due to recent changes in Control::set method

[mvorisek]

i see the demos are broken due to recent changes in Control::set method

I belive that is because of a new release of phpstan :) - please fix it first in a separate PR

Additionally, I believe I've resolved an issue where, under certain conditions, the URL returned was missing the relative path.

then please split this PR to contain minimal fix + minimal "directory index" feature + minimal tests

once that is done, submit another refactoring PR incl. $_SERVER['REQUEST_URI'] change, the change might look small, but it can be huge, a few months ago I was fixing one URL /w realpath & symlink resolving issue and it took me half day, so these changes needs to be landed separately to be able to bisect/revert them if needed

[abbadon1334]

i see the demos are broken due to recent changes in Control::set method

I belive that is because of a new release of phpstan :) - please fix it first in a separate PR

Sure

Additionally, I believe I've resolved an issue where, under certain conditions, the URL returned was missing the relative path.

then please split this PR to contain minimal fix + minimal "directory index" feature + minimal tests

once that is done, submit another refactoring PR incl. $_SERVER['REQUEST_URI'] change, the change might look small, but it can be huge, a few months ago I was fixing one URL /w realpath & symlink resolving issue and it took me half day, so these changes needs to be landed separately to be able to bisect/revert them if needed

for sure, trying to test the results of the previous App::url gives inconsistent response, but for sake of testing i can checkout the develop branch and add only unit tests and we can discuss the inconsistency in every cases