Skip to content

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

Merged
mvorisek merged 21 commits intodevelopfrom
add_tests_app_url
Sep 9, 2023
Merged

Reintroduce App::$page as App::$urlBuildingIndexPage#2096
mvorisek merged 21 commits intodevelopfrom
add_tests_app_url

Conversation

@abbadon1334
Copy link
Copy Markdown
Collaborator

@abbadon1334 abbadon1334 commented Aug 28, 2023

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

@abbadon1334 abbadon1334 marked this pull request as draft August 28, 2023 08:53
@abbadon1334
Copy link
Copy Markdown
Collaborator Author

abbadon1334 commented Aug 28, 2023

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
Copy link
Copy Markdown
Collaborator Author

@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

@abbadon1334 abbadon1334 requested a review from mvorisek September 3, 2023 16:41
@mvorisek
Copy link
Copy Markdown
Member

mvorisek commented Sep 3, 2023

@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
Copy link
Copy Markdown
Collaborator Author

@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
Copy link
Copy Markdown
Member

mvorisek commented Sep 4, 2023

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
Copy link
Copy Markdown
Collaborator Author

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
Copy link
Copy Markdown
Collaborator Author

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 mvorisek changed the title Improve unit test for App::url Reintroduce App::$page as App::$urlBuildingPage Sep 4, 2023
@mvorisek
Copy link
Copy Markdown
Member

mvorisek commented Sep 4, 2023

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

@mvorisek mvorisek removed the Tests label Sep 4, 2023
@abbadon1334 abbadon1334 marked this pull request as ready for review September 8, 2023 17:39
src/App.php Outdated
foreach ($this->stickyGetArguments as $k => $v) {
if ($v && isset($_GET[$k])) {
$args[$k] = $_GET[$k];
if ($v && isset($queryParams[$k])) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be done in #2101

}
}

public function provideUrlBuildingCases(): iterable
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merged into #2095 and reverted from this PR

@mvorisek mvorisek changed the title Reintroduce App::$page as App::$urlBuildingPage Reintroduce App::$page as App::$urlBuildingIndexPage Sep 9, 2023
@mvorisek mvorisek merged commit d40d1c3 into develop Sep 9, 2023
@mvorisek mvorisek deleted the add_tests_app_url branch September 9, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants