Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 63 commits into from
Sep 17, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
7fdbbe7
Fix PrettyUrl legitimate path
abbadon1334 Aug 23, 2023
ba90e32
Add Unit tests for App::url
abbadon1334 Aug 25, 2023
f2dd8f3
add default excepted page and extension
abbadon1334 Aug 25, 2023
6713d61
fix 7.4 array merge with keys / spread operator
abbadon1334 Aug 25, 2023
04720c5
Add more test cases for App:url
abbadon1334 Aug 25, 2023
80b8fca
Reorganize code
abbadon1334 Aug 25, 2023
27d9cb4
Improve converage
abbadon1334 Aug 26, 2023
67f0594
Improve App::url tests
abbadon1334 Aug 28, 2023
b2810b8
Skip $useRequestUrl=true due to REQUEST_URI
abbadon1334 Aug 28, 2023
3997714
Change REQUEST_URI to Psr7 methods
abbadon1334 Aug 28, 2023
4348ff1
Allow $useRequestUrl=true
abbadon1334 Aug 28, 2023
e4c3428
clean up
abbadon1334 Aug 28, 2023
7895ec9
Improve error message
abbadon1334 Aug 28, 2023
f1fefac
CS Fix
abbadon1334 Aug 28, 2023
308cc87
Merge branch 'develop' into add_tests_app_url
abbadon1334 Sep 3, 2023
23a5a6b
Add App::$urlBuildingPage to customize url building page
abbadon1334 Sep 4, 2023
f3f099f
Comment out App::url failing tests
abbadon1334 Sep 4, 2023
3c36038
Merge branch 'develop' into fix_pretty_url_legitimate_path
abbadon1334 Sep 4, 2023
534cff4
Avoid change of global var $_SERVER
abbadon1334 Sep 4, 2023
a01c3bb
Fix missing request in App for Unit tests
abbadon1334 Sep 4, 2023
62f370b
Comment out not working tests
abbadon1334 Sep 4, 2023
645e0aa
Strong type for $urlBuildingXxx vars
abbadon1334 Sep 8, 2023
7022361
Use CreateApp
abbadon1334 Sep 8, 2023
0a1bd7f
Remove message from Assert
abbadon1334 Sep 8, 2023
95b7d1e
Cs Fix
abbadon1334 Sep 8, 2023
7bd47a2
Raise Nyholm/psr7 from 1.4 to 1.6
abbadon1334 Sep 8, 2023
d5b4224
Merge branch 'develop' into add_tests_app_url
mvorisek Sep 9, 2023
47d6032
DEBUG REVERT ALL
mvorisek Sep 9, 2023
d4a0810
introduce App::$urlBuildingIndexPage prop
mvorisek Sep 9, 2023
89e3efa
drop protected App::getRequestUrl() method
mvorisek Sep 9, 2023
b490c1f
replace $_SERVER['REQUEST_URI'] with PSR7 request
mvorisek Sep 9, 2023
4d0e4c7
fix cs
mvorisek Sep 9, 2023
8b3bd58
original AppTest changes
mvorisek Sep 9, 2023
fccada4
no manual assert context dump
mvorisek Sep 9, 2023
b14f5fe
allow to create test App with defaults
mvorisek Sep 9, 2023
c3de25c
always use test App in tests
mvorisek Sep 9, 2023
a4c193f
Merge branch 'develop'
mvorisek Sep 9, 2023
84cfbd0
Merge PR 2096
mvorisek Sep 9, 2023
547f2f4
remaining reverted changes from PR 2096
mvorisek Sep 9, 2023
9353bba
Merge develop
mvorisek Sep 9, 2023
b80942d
Merge branch 'develop'
mvorisek Sep 10, 2023
c9d06c6
Merge branch 'develop'
mvorisek Sep 14, 2023
bae7f69
Merge branch 'develop'
mvorisek Sep 14, 2023
9cff373
enable commented tests
mvorisek Sep 14, 2023
75efe8c
silence failing tests
mvorisek Sep 14, 2023
2c2b8ca
add wrongly removed check_unset_page test
mvorisek Sep 14, 2023
965926e
REVERT ALL (reset to develop)
mvorisek Sep 14, 2023
1fe9677
Merge branch 'develop'
mvorisek Sep 15, 2023
1d3df06
reland reverted changes in minimal fashion
mvorisek Sep 15, 2023
637f66a
Revert nonse App:url() path build
mvorisek Sep 15, 2023
8d7cab6
remove basename() from App::url() page build
mvorisek Sep 15, 2023
95b97e8
improve grammar
mvorisek Sep 17, 2023
91fa2a1
uncomment all tests
mvorisek Sep 17, 2023
01a7aac
simplify original URL build testing
mvorisek Sep 17, 2023
afd9810
refactor all providers to yield
mvorisek Sep 17, 2023
e4119fd
improve phpdoc
mvorisek Sep 17, 2023
1999c98
improve wording
mvorisek Sep 17, 2023
2176b1a
rework testing
mvorisek Sep 17, 2023
f0e9972
unset sticky is deleting extra
mvorisek Sep 17, 2023
1526d80
fix typo
mvorisek Sep 17, 2023
b7f8f96
sync before merge
mvorisek Sep 17, 2023
7de30d6
Merge branch 'develop'
mvorisek Sep 17, 2023
bbf9782
improve regex
mvorisek Sep 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 62 additions & 27 deletions src/App.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,15 @@ class App
/** @var array<string, View> Modal view that need to be rendered using JSON output. */
private $portals = [];

/**
* @var string used in method App::url to build the url
*
* Used only in method App::url
* If filename part is missing during building of url, this page will be used
* Hint: if you use a routing system, you need to set this and urlBuildingExt to empty string
*/
protected $urlBuildingPage = 'index';

/**
* @var string used in method App::url to build the URL
*
Expand Down Expand Up @@ -689,35 +698,65 @@ public function stickyForget(string $name): void
* @param bool $useRequestUrl Simply return $_SERVER['REQUEST_URI'] if needed
mvorisek marked this conversation as resolved.
Show resolved Hide resolved
* @param array<string, string> $extraRequestUrlArgs additional URL arguments, deleting sticky can delete them
*/
public function url($page = [], $useRequestUrl = false, $extraRequestUrlArgs = []): string
public function url($page = [], bool $useRequestUrl = false, array $extraRequestUrlArgs = []): string
{
if ($useRequestUrl) {
$page = $_SERVER['REQUEST_URI'];
$page = $this->urlGetCurrentRequestPage();
}

$pagePath = '';
if (is_string($page)) {
$pageExploded = explode('?', $page, 2);
$pagePath = $pageExploded[0];
parse_str($pageExploded[1] ?? '', $page);
} else {
if (isset($page[0])) {
$pagePath = $page[0];
} else {
// use current page by default
$requestUrl = $this->getRequestUrl();
if (substr($requestUrl, -1, 1) === '/') {
$pagePath = 'index';
} else {
$pagePath = basename($requestUrl, $this->urlBuildingExt);
}
}
unset($page[0]);
if ($pagePath) {
$pagePath .= $this->urlBuildingExt;
}
$page = $this->urlSplitStringPageIntoArray($page);
}

$pagePath = $this->urlConstructPagePath($page[0] ?? $this->getRequestUrl());
unset($page[0]);

$args = $this->urlMergeArguments($page, $extraRequestUrlArgs);

$pageQuery = http_build_query($args, '', '&', \PHP_QUERY_RFC3986);

return $pagePath . ($pageQuery ? '?' . $pageQuery : '');
}

private function urlGetCurrentRequestPage(): array
{
$request = $this->getRequest();
$page = $request->getQueryParams();
$page[0] = $request->getUri()->getPath();

return $page;
}

private function urlSplitStringPageIntoArray(string $page): array
{
$pageExploded = explode('?', $page, 2);
$arrayPage = [];
parse_str($pageExploded[1] ?? '', $arrayPage);
$arrayPage[0] = $pageExploded[0];

return $arrayPage;
}

private function urlConstructPagePath(string $pagePath): string
{
// Changed array string access to substr for PHP 7.4 compatibility
$lastChar = substr($pagePath, -1);
if ($lastChar === '/') {
return $pagePath . $this->urlBuildingPage . $this->urlBuildingExt;
}

$pagePathPart = trim(dirname($pagePath), '.');
Copy link
Member

@mvorisek mvorisek Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this is super wrong, we cannot just trim unlimited .

also dirname('/xxx') on Windows results \

Copy link
Member

Choose a reason for hiding this comment

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

@abbadon1334 please review the 4 last commits

The end changed LoC is minimal.

I have updated the PR title - is this what you wanted or something else/more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switching title to "preserve" make sense even for tests without full path.
LGTM and thanks to go ahead on my work and do it better

$pagePathFile = basename($pagePath, $this->urlBuildingExt);

if ($pagePathPart !== '') {
$pagePathPart .= '/';
}

return str_replace('//', '/', $pagePathPart . $pagePathFile) . $this->urlBuildingExt;
}

private function urlMergeArguments(array $page, array $extraRequestUrlArgs): array
{
$args = $extraRequestUrlArgs;

// add sticky arguments
Expand All @@ -738,11 +777,7 @@ public function url($page = [], $useRequestUrl = false, $extraRequestUrlArgs = [
}
}

// put URL together
$pageQuery = http_build_query($args, '', '&', \PHP_QUERY_RFC3986);
$url = $pagePath . ($pageQuery ? '?' . $pageQuery : '');

return $url;
return $args;
}

/**
Expand Down
139 changes: 139 additions & 0 deletions tests/AppTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
namespace Atk4\Ui\Tests;

use Atk4\Core\Phpunit\TestCase;
use Atk4\Ui\App;
use Atk4\Ui\Exception\LateOutputError;
use Atk4\Ui\HtmlTemplate;
use Nyholm\Psr7\Factory\Psr17Factory;

class AppTest extends TestCase
{
Expand Down Expand Up @@ -68,4 +70,141 @@ public function testUnexpectedOutputLateError(): void
ob_end_clean();
}
}

public function provideUrlBuildingCases(): iterable
{
return [
// simple cases
[[], false, [], '/', '/index.php', '/default.php8', '/'],
[[], false, [], '/test/', '/test/index.php', '/test/default.php8', '/test/'],
[[], true, [], '/', '/index.php', '/default.php8', '/'],
[[], true, [], '/test/', '/test/index.php', '/test/default.php8', '/test/'],

// simple cases with query args in request
[[], false, [], '/?test=atk4', '/index.php?test=atk4', '/default.php8?test=atk4', '/?test=atk4'],
[[], false, [], '/test/?test=atk4', '/test/index.php?test=atk4', '/test/default.php8?test=atk4', '/test/?test=atk4'],
[[], true, [], '/?test=atk4', '/index.php?test=atk4', '/default.php8?test=atk4', '/?test=atk4'],
[[], true, [], '/test/?test=atk4', '/test/index.php?test=atk4', '/test/default.php8?test=atk4', '/test/?test=atk4'],

// simple cases with extra query args in request
[[], false, ['extra_args' => 'atk4'], '/?test=atk4', '/index.php?extra_args=atk4&test=atk4', '/default.php8?extra_args=atk4&test=atk4', '/?extra_args=atk4&test=atk4'],
[[], false, ['extra_args' => 'atk4'], '/test/?test=atk4', '/test/index.php?extra_args=atk4&test=atk4', '/test/default.php8?extra_args=atk4&test=atk4', '/test/?extra_args=atk4&test=atk4'],
[[], true, ['extra_args' => 'atk4'], '/?test=atk4', '/index.php?extra_args=atk4&test=atk4', '/default.php8?extra_args=atk4&test=atk4', '/?extra_args=atk4&test=atk4'],
[[], true, ['extra_args' => 'atk4'], '/test/?test=atk4', '/test/index.php?extra_args=atk4&test=atk4', '/test/default.php8?extra_args=atk4&test=atk4', '/test/?extra_args=atk4&test=atk4'],

// simple cases with page as string
['test', false, [], '/', 'test.php', 'test.php8', 'test'],
['test/test/a', false, [], '/', 'test/test/a.php', 'test/test/a.php8', 'test/test/a'],
['test/index', false, [], '/', 'test/index.php', 'test/index.php8', 'test/index'],
['test/index', false, [], '/test/', 'test/index.php', 'test/index.php8', 'test/index'],
['test', true, [], '/', '/index.php', '/default.php8', '/'],
['test', true, [], '/request-url', '/request-url.php', '/request-url.php8', '/request-url'],
['test', true, [], '/request-url/', '/request-url/index.php', '/request-url/default.php8', '/request-url/'],
['test/index', true, [], '/test/', '/test/index.php', '/test/default.php8', '/test/'],
['test', false, [], '/', 'test.php', 'test.php8', 'test'],

// simple cases with page as array with 0 => string
[['test'], false, [], '/', 'test.php', 'test.php8', 'test'],
[['test/test/a'], false, [], '/', 'test/test/a.php', 'test/test/a.php8', 'test/test/a'],
[['test/index'], false, [], '/test/', 'test/index.php', 'test/index.php8', 'test/index'],
[['test'], true, [], '/', '/index.php', '/default.php8', '/'],
[['test'], true, [], '/request-url', '/request-url.php', '/request-url.php8', '/request-url'],
[['test'], true, [], '/request-url/', '/request-url/index.php', '/request-url/default.php8', '/request-url/'],
[['test/index'], true, [], '/test/', '/test/index.php', '/test/default.php8', '/test/'],
[['test'], false, [], '/', 'test.php', 'test.php8', 'test'],

// query args in page cases
[['test', 'extra_args' => 'atk4'], false, [], '/', 'test.php?extra_args=atk4', 'test.php8?extra_args=atk4', 'test?extra_args=atk4'],
[['test/test/a', 'extra_args' => 'atk4'], false, [], '/', 'test/test/a.php?extra_args=atk4', 'test/test/a.php8?extra_args=atk4', 'test/test/a?extra_args=atk4'],
[['test/index', 'extra_args' => 'atk4'], false, [], '/test/', 'test/index.php?extra_args=atk4', 'test/index.php8?extra_args=atk4', 'test/index?extra_args=atk4'],
[['test', 'extra_args' => 'atk4'], true, [], '/', '/index.php', '/default.php8', '/'],
[['test', 'extra_args' => 'atk4'], true, [], '/request-url', '/request-url.php', '/request-url.php8', '/request-url'],
[['test', 'extra_args' => 'atk4'], true, [], '/request-url/', '/request-url/index.php', '/request-url/default.php8', '/request-url/'],
[['test/index', 'extra_args' => 'atk4'], true, [], '/test/', '/test/index.php', '/test/default.php8', '/test/'],
[['test', 'extra_args' => 'atk4'], false, [], '/', 'test.php?extra_args=atk4', 'test.php8?extra_args=atk4', 'test?extra_args=atk4'],

// extra query args cases
[['test'], false, ['extra_args' => 'atk4'], '/', 'test.php?extra_args=atk4', 'test.php8?extra_args=atk4', 'test?extra_args=atk4'],
[['test/test/a'], false, ['extra_args' => 'atk4'], '/', 'test/test/a.php?extra_args=atk4', 'test/test/a.php8?extra_args=atk4', 'test/test/a?extra_args=atk4'],
[['test/index'], false, ['extra_args' => 'atk4'], '/test/', 'test/index.php?extra_args=atk4', 'test/index.php8?extra_args=atk4', 'test/index?extra_args=atk4'],
[['test'], true, ['extra_args' => 'atk4'], '/', '/index.php?extra_args=atk4', '/default.php8?extra_args=atk4', '/?extra_args=atk4'],
[['test'], true, ['extra_args' => 'atk4'], '/request-url', '/request-url.php?extra_args=atk4', '/request-url.php8?extra_args=atk4', '/request-url?extra_args=atk4'],
[['test'], true, ['extra_args' => 'atk4'], '/request-url/', '/request-url/index.php?extra_args=atk4', '/request-url/default.php8?extra_args=atk4', '/request-url/?extra_args=atk4'],
[['test/index'], true, ['extra_args' => 'atk4'], '/test/', '/test/index.php?extra_args=atk4', '/test/default.php8?extra_args=atk4', '/test/?extra_args=atk4'],
[['test'], false, ['extra_args' => 'atk4'], '/', 'test.php?extra_args=atk4', 'test.php8?extra_args=atk4', 'test?extra_args=atk4'],

// query args in page cases and query args in request cases and extra query args cases
[['test', 'page_args' => 'atk4'], false, ['extra_args' => 'atk4'], '/?extra_args=atk4&query_args=atk4&page_args=atk4', 'test.php?extra_args=atk4&query_args=atk4&page_args=atk4', 'test.php8?extra_args=atk4&query_args=atk4&page_args=atk4', 'test?extra_args=atk4&query_args=atk4&page_args=atk4'],
[['test/test/a', 'page_args' => 'atk4'], false, ['extra_args' => 'atk4'], '/?extra_args=atk4&query_args=atk4&page_args=atk4', 'test/test/a.php?extra_args=atk4&query_args=atk4&page_args=atk4', 'test/test/a.php8?extra_args=atk4&query_args=atk4&page_args=atk4', 'test/test/a?extra_args=atk4&query_args=atk4&page_args=atk4'],
[['test/index', 'page_args' => 'atk4'], false, ['extra_args' => 'atk4'], '/test/?extra_args=atk4&query_args=atk4&page_args=atk4', 'test/index.php?extra_args=atk4&query_args=atk4&page_args=atk4', 'test/index.php8?extra_args=atk4&query_args=atk4&page_args=atk4', 'test/index?extra_args=atk4&query_args=atk4&page_args=atk4'],
[['test', 'page_args' => 'atk4'], true, ['extra_args' => 'atk4'], '/', '/index.php?extra_args=atk4', '/default.php8?extra_args=atk4', '/?extra_args=atk4'],
[['test', 'page_args' => 'atk4'], true, ['extra_args' => 'atk4'], '/request-url', '/request-url.php?extra_args=atk4', '/request-url.php8?extra_args=atk4', '/request-url?extra_args=atk4'],
[['test', 'page_args' => 'atk4'], true, ['extra_args' => 'atk4'], '/request-url/', '/request-url/index.php?extra_args=atk4', '/request-url/default.php8?extra_args=atk4', '/request-url/?extra_args=atk4'],
[['test/index', 'page_args' => 'atk4'], true, ['extra_args' => 'atk4'], '/test/', '/test/index.php?extra_args=atk4', '/test/default.php8?extra_args=atk4', '/test/?extra_args=atk4'],
[['test', 'page_args' => 'atk4', 'check_unset_page' => false], false, ['extra_args' => 'atk4'], '/?extra_args=atk4&query_args=atk4&page_args=atk4', 'test.php?extra_args=atk4&query_args=atk4&page_args=atk4', 'test.php8?extra_args=atk4&query_args=atk4&page_args=atk4', 'test?extra_args=atk4&query_args=atk4&page_args=atk4'],
];
}

/**
* @dataProvider provideUrlBuildingCases
*
* @param string|array<0|string, string|int|false> $page URL as string or array with page name as first element and other GET arguments
* @param bool $useRequestUrl Simply return $_SERVER['REQUEST_URI'] if needed
* @param array<string, string> $extraRequestUrlArgs Additional URL arguments, deleting sticky can delete them
*/
public function testUrlBuilding($page, bool $useRequestUrl, array $extraRequestUrlArgs, string $requestUrl, string $exceptedStd, string $exceptedCustom, string $exceptedRouting): void
{
$factory = new Psr17Factory();
$request = $factory->createRequest('GET', 'http://127.0.0.1' . $requestUrl);

$_SERVER = [
mvorisek marked this conversation as resolved.
Show resolved Hide resolved
'REQUEST_METHOD' => 'GET',
'HTTP_HOST' => $request->getUri()->getHost(),
'REQUEST_URI' => $requestUrl,
'QUERY_STRING' => $request->getUri()->getQuery(),
];

parse_str($request->getUri()->getQuery(), $_GET);

foreach ($extraRequestUrlArgs as $key => $value) {
$_GET[$key] = $value;
}

$stickyGetArguments = [
'__atk_json' => false,
'__atk_tab' => false,
];

foreach ($_GET as $key => $value) {
$stickyGetArguments[$key] = $value;
}

$app = new App([
abbadon1334 marked this conversation as resolved.
Show resolved Hide resolved
'stickyGetArguments' => $stickyGetArguments,
'catchExceptions' => false,
'alwaysRun' => false,
]);
self::assertSame($exceptedStd, $app->url($page, $useRequestUrl, $extraRequestUrlArgs), 'App::url test error case: standard');
self::assertSame($exceptedStd, $app->jsUrl($page, $useRequestUrl, $extraRequestUrlArgs), 'App::jsUrl test error case: standard');

$app = new App([
'stickyGetArguments' => $stickyGetArguments,
'urlBuildingPage' => 'default',
'urlBuildingExt' => '.php8',
'catchExceptions' => false,
'alwaysRun' => false,
]);
self::assertSame($exceptedCustom, $app->url($page, $useRequestUrl, $extraRequestUrlArgs), 'App::url test error case: custom page/ext');
self::assertSame($exceptedCustom, $app->jsUrl($page, $useRequestUrl, $extraRequestUrlArgs), 'App::jsUrl test error case: custom page/ext');

$app = new App([
'stickyGetArguments' => $stickyGetArguments,
'urlBuildingPage' => '',
'urlBuildingExt' => '',
'catchExceptions' => false,
'alwaysRun' => false,
]);
self::assertSame($exceptedRouting, $app->url($page, $useRequestUrl, $extraRequestUrlArgs), 'App::url test error case: routing');
self::assertSame($exceptedRouting, $app->jsUrl($page, $useRequestUrl, $extraRequestUrlArgs), 'App::jsUrl test error case: routing');
}
}
4 changes: 2 additions & 2 deletions tests/CallbackTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ public function testViewUrlCallback(): void
$this->simulateCallbackTriggering($cbApp);
$this->simulateCallbackTriggering($cb);

$expectedUrlCbApp = '?' . Callback::URL_QUERY_TRIGGER_PREFIX . 'aa=callback&' . Callback::URL_QUERY_TARGET . '=aa';
$expectedUrlCb = '?' . /* Callback::URL_QUERY_TRIGGER_PREFIX . 'aa=1&' . */ Callback::URL_QUERY_TRIGGER_PREFIX . 'bb=callback&' . Callback::URL_QUERY_TARGET . '=bb';
$expectedUrlCbApp = '/index.php?' . Callback::URL_QUERY_TRIGGER_PREFIX . 'aa=callback&' . Callback::URL_QUERY_TARGET . '=aa';
$expectedUrlCb = '/index.php?' . /* Callback::URL_QUERY_TRIGGER_PREFIX . 'aa=1&' . */ Callback::URL_QUERY_TRIGGER_PREFIX . 'bb=callback&' . Callback::URL_QUERY_TARGET . '=bb';
self::assertSame($expectedUrlCbApp, $cbApp->getUrl());
self::assertSame($expectedUrlCb, $cb->getUrl());

Expand Down