Reintroduce App::$page as App::$urlBuildingIndexPage#2096
Conversation
|
some results from tests: thinking about the behaviour of url building in browser, writing same if The same happens when useRequestUrl=true (the reference path is taken from request url): i have taken only the first test results but all the inconsistency comes from relative/absolute uri paths and when to added index.php Let's discuss about this this must be merged before #2095 |
|
@mvorisek what do you think about this? align the test results of App::url to old response or rework it to make it consistent? You can see them in the tests |
definitely to be consistent/correct in a long term - is this how this PR is coded now? |
Sorry, added now App::$urlBuildingPage with default value = index to get test results. IMHO even minimal tests with the actual url method are tests against inconsistent results, for example : If in first case returns I think I can :
|
This is basically a few LoC with no BC-break. Separate PR is fine, but one PR together with the tests is fine as well. So I propose:
|
|
OK, so:
OK i do it later today and close without merging the other PR |
|
OK, this add successful tests + App::$urlBuildingPage Even if App::$page was not used so much by the community, i think must be add a comment for the release, something like: [BC] App::$page changed to App::$urlBuildingPage i go ahead with the other PR |
updated title + marked #2065 as BC-break, release notes will be rebuilt once this PR is merged |
src/App.php
Outdated
| foreach ($this->stickyGetArguments as $k => $v) { | ||
| if ($v && isset($_GET[$k])) { | ||
| $args[$k] = $_GET[$k]; | ||
| if ($v && isset($queryParams[$k])) { |
tests/AppTest.php
Outdated
| } | ||
| } | ||
|
|
||
| public function provideUrlBuildingCases(): iterable |
BC break: remove
App::getRequestUrl()protected method which was misleading alias for request URL path