Reintroduce App::$page as App::$urlBuildingIndexPage

[abbadon1334]

BC break: remove App::getRequestUrl() protected method which was misleading alias for request URL path

[abbadon1334]

some results from tests:

App::url test error case: standard (from: / and $useRequestUrl=0)
Failed asserting that two strings are identical.
Expected :'/index.php'
Actual   :'index.php'

App::url test error case: standard (from: /test/ and $useRequestUrl=0)
Failed asserting that two strings are identical.
Expected :'/test/index.php'
Actual   :'index.php'

thinking about the behaviour of url building in browser, writing index.php or /index.php is the same if the actual document.location = '/'

same if document.location='/test/ and url is without / calling index.php became in browser /test/index.php so why not build it directly in App::url returning the correct path and make it consistent with browser?

The same happens when useRequestUrl=true (the reference path is taken from request url):

App::url test error case: standard (from: / and $useRequestUrl=1)
Failed asserting that two strings are identical.
Expected :'/index.php'
Actual   :'/'

App::url test error case: standard (from: /test/ and $useRequestUrl=1)
Failed asserting that two strings are identical.
Expected :'/test/index.php'
Actual   :'/test/'

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

[abbadon1334]

@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

[mvorisek]

@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?

[abbadon1334]

@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 :

1 - App::url('/') -> /index.php
2 - App::url('/test/') -> /test/

If in first case returns /index.php the second case must return /test/index.php

I think I can :

• Remove the tests from this PR, leaving only the addiction of App::$urlBuildingPage
• Merge this PR
• Make a new PR with App::url refactor and this tests which at that point will be consistent.
• Test if all is working and add more tests if I find more inconsistency

[mvorisek]

Remove the tests from this PR, leaving only the addiction of App::$urlBuildingPage

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:

• to keep both in this PR
• + a separate PR for the fix/consistency later - for now, you can comment the failing tests out and link a draft PR that will be merged later with "BC break" tag and BC break described concisely in the PR description

[abbadon1334]

OK, so:

• this PR, i just comment the failing tests
• open another with all tests and App::url rewrite with BC Break tag

OK i do it later today and close without merging the other PR

[abbadon1334]

OK, this add successful tests + App::$urlBuildingPage @mvorisek i think this can be merged

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

[mvorisek]

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

updated title + marked #2065 as BC-break, release notes will be rebuilt once this PR is merged