Reintroduce App::$page as App::$urlBuildingIndexPage #2096

abbadon1334 · 2023-08-28T08:51:36Z

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

abbadon1334 · 2023-08-28T09:25:44Z

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 · 2023-09-03T16:41:53Z

@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 · 2023-09-03T18:38:38Z

@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 · 2023-09-04T06:48:01Z

@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 · 2023-09-04T07:03:39Z

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 · 2023-09-04T09:06:31Z

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 · 2023-09-04T11:38:46Z

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 · 2023-09-04T11:45:33Z

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

tests/AppTest.php

src/App.php

mvorisek · 2023-09-09T19:03:27Z

src/App.php


        // add sticky arguments
        foreach ($this->stickyGetArguments as $k => $v) {
-            if ($v && isset($_GET[$k])) {
-                $args[$k] = $_GET[$k];
+            if ($v && isset($queryParams[$k])) {


to be done in #2101

mvorisek · 2023-09-09T19:35:53Z

tests/AppTest.php

@@ -68,123 +67,4 @@ public function testUnexpectedOutputLateError(): void
            ob_end_clean();
        }
    }
-
-    public function provideUrlBuildingCases(): iterable


merged into #2095 and reverted from this PR

abbadon1334 added 2 commits August 28, 2023 10:49

Improve App::url tests

67f0594

Skip $useRequestUrl=true due to REQUEST_URI

b2810b8

abbadon1334 marked this pull request as draft August 28, 2023 08:53

abbadon1334 added 3 commits August 28, 2023 11:04

Change REQUEST_URI to Psr7 methods

3997714

Allow $useRequestUrl=true

4348ff1

clean up

e4c3428

abbadon1334 added the Tests label Aug 28, 2023

Improve error message

7895ec9

abbadon1334 added 2 commits August 28, 2023 11:42

CS Fix

f1fefac

Merge branch 'develop' into add_tests_app_url

308cc87

abbadon1334 requested a review from mvorisek September 3, 2023 16:41

Add App::$urlBuildingPage to customize url building page

23a5a6b

Comment out App::url failing tests

f3f099f

mvorisek changed the title ~~Improve unit test for App::url~~ Reintroduce App::$page as App::$urlBuildingPage Sep 4, 2023

mvorisek mentioned this pull request Sep 4, 2023

Narrow URL array phpdoc types #2065

Merged

mvorisek removed the Tests label Sep 4, 2023

mvorisek reviewed Sep 4, 2023

View reviewed changes

tests/AppTest.php Outdated Show resolved Hide resolved

mvorisek reviewed Sep 4, 2023

View reviewed changes

tests/AppTest.php Outdated Show resolved Hide resolved

mvorisek reviewed Sep 4, 2023

View reviewed changes

src/App.php Outdated Show resolved Hide resolved

mvorisek reviewed Sep 4, 2023

View reviewed changes

src/App.php Outdated Show resolved Hide resolved

mvorisek mentioned this pull request Sep 6, 2023

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

Merged

abbadon1334 marked this pull request as ready for review September 8, 2023 17:39

mvorisek added 5 commits September 9, 2023 19:46

Merge branch 'develop' into add_tests_app_url

d5b4224

DEBUG REVERT ALL

47d6032

introduce App::$urlBuildingIndexPage prop

d4a0810

drop protected App::getRequestUrl() method

89e3efa

replace $_SERVER['REQUEST_URI'] with PSR7 request

b490c1f

mvorisek reviewed Sep 9, 2023

View reviewed changes

mvorisek added 6 commits September 9, 2023 21:05

fix cs

4d0e4c7

original AppTest changes

8b3bd58

no manual assert context dump

fccada4

allow to create test App with defaults

b14f5fe

always use test App in tests

c3de25c

Revert AppTest changes

ce40c14

mvorisek reviewed Sep 9, 2023

View reviewed changes

mvorisek changed the title ~~Reintroduce App::$page as App::$urlBuildingPage~~ Reintroduce App::$page as App::$urlBuildingIndexPage Sep 9, 2023

mvorisek added the BC-break label Sep 9, 2023

mvorisek merged commit d40d1c3 into develop Sep 9, 2023
55 checks passed

mvorisek deleted the add_tests_app_url branch September 9, 2023 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce App::$page as App::$urlBuildingIndexPage #2096

Reintroduce App::$page as App::$urlBuildingIndexPage #2096

abbadon1334 commented Aug 28, 2023 •

edited by mvorisek

Loading

abbadon1334 commented Aug 28, 2023 •

edited

Loading

abbadon1334 commented Sep 3, 2023

mvorisek commented Sep 3, 2023

abbadon1334 commented Sep 4, 2023

mvorisek commented Sep 4, 2023 •

edited

Loading

abbadon1334 commented Sep 4, 2023

abbadon1334 commented Sep 4, 2023

mvorisek commented Sep 4, 2023

mvorisek Sep 9, 2023

mvorisek Sep 9, 2023

Reintroduce App::$page as App::$urlBuildingIndexPage #2096

Reintroduce App::$page as App::$urlBuildingIndexPage #2096

Conversation

abbadon1334 commented Aug 28, 2023 • edited by mvorisek Loading

abbadon1334 commented Aug 28, 2023 • edited Loading

abbadon1334 commented Sep 3, 2023

mvorisek commented Sep 3, 2023

abbadon1334 commented Sep 4, 2023

mvorisek commented Sep 4, 2023 • edited Loading

abbadon1334 commented Sep 4, 2023

abbadon1334 commented Sep 4, 2023

mvorisek commented Sep 4, 2023

mvorisek Sep 9, 2023

Choose a reason for hiding this comment

mvorisek Sep 9, 2023

Choose a reason for hiding this comment

abbadon1334 commented Aug 28, 2023 •

edited by mvorisek

Loading

abbadon1334 commented Aug 28, 2023 •

edited

Loading

mvorisek commented Sep 4, 2023 •

edited

Loading