FIX Cast absoluteUrl() argument to string #10610

emteknetnz · 2022-12-08T21:09:39Z

Issue https://github.com/silverstripeltd/product-issues/issues/642

Similar to silverstripe/silverstripe-elemental#1029

GuySartorelli

See silverstripe/silverstripe-cms#2806 (comment) for a discussion on this approach. Strongly typing the return type of the given methods is out of scope for CMS 5.

Some to remove here where it's not necessary though. It's a fairly small PR so I think it's reasonable to only do this where the variable can actually be something other than a string.

GuySartorelli · 2022-12-13T03:32:40Z

tests/php/Control/DirectorTest.php

+            $this->assertEquals("http://www.mysite.com:9090/mysite/", Director::absoluteURL((string) $url, Director::BASE));
+            $this->assertEquals("http://www.mysite.com:9090/", Director::absoluteURL((string) $url, Director::ROOT));
+            $this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/", Director::absoluteURL((string) $url, Director::REQUEST));


Suggested change

$this->assertEquals("http://www.mysite.com:9090/mysite/", Director::absoluteURL((string) $url, Director::BASE));

$this->assertEquals("http://www.mysite.com:9090/", Director::absoluteURL((string) $url, Director::ROOT));

$this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/", Director::absoluteURL((string) $url, Director::REQUEST));

$this->assertEquals("http://www.mysite.com:9090/mysite/", Director::absoluteURL($url, Director::BASE));

$this->assertEquals("http://www.mysite.com:9090/", Director::absoluteURL($url, Director::ROOT));

$this->assertEquals("http://www.mysite.com:9090/mysite/sub-page/", Director::absoluteURL($url, Director::REQUEST));

Not necessary here.

GuySartorelli · 2022-12-13T03:33:24Z

tests/php/Control/ControllerTest.php

@@ -475,7 +475,7 @@ public function testRedirectBackByBackUrl()
        $externalAbsoluteUrl = 'http://myhost.com/some-url';
        $response = $this->get('TestController/redirectbacktest?BackURL=' . urlencode($externalAbsoluteUrl ?? ''));
        $this->assertEquals(
-            Director::absoluteURL($link),
+            Director::absoluteURL((string) $link),


Suggested change

Director::absoluteURL((string) $link),

Director::absoluteURL($link),

Not necessary here

GuySartorelli · 2022-12-13T03:35:24Z

src/Security/PasswordExpirationMiddleware.php

@@ -145,15 +145,15 @@ protected function handleExpiredPassword(HTTPRequest $request): ?HTTPResponse
            return null;
        }

-        $currentUrl = $this->absoluteUrl($request->getURL(true));
+        $currentUrl = $this->absoluteUrl((string) $request->getURL(true));


Suggested change

$currentUrl = $this->absoluteUrl((string) $request->getURL(true));

$currentUrl = $this->absoluteUrl($request->getURL(true));

Not necessary here

I'd be keen to understand how this cast-to-string is affecting that unit test - I would have expected the CI to fail by throwing an exception if the problem was that the wrong type is being passed to a strongly-typed parameter.

GuySartorelli · 2022-12-13T03:40:04Z

src/Forms/HTMLEditor/TinyMCEConfig.php

@@ -863,7 +863,7 @@ public static function get_tinymce_lang_url(): string
        $dir = static::config()->get('lang_dir');
        if ($lang !== 'en' && !empty($dir)) {
            $resource = ModuleResourceLoader::singleton()->resolveResource($dir);
-            return Director::absoluteURL($resource->getRelativeResource($lang . '.js')->getURL());
+            return Director::absoluteURL((string) $resource->getRelativeResource($lang . '.js')->getURL());


Suggested change

return Director::absoluteURL((string) $resource->getRelativeResource($lang . '.js')->getURL());

return Director::absoluteURL($resource->getRelativeResource($lang . '.js')->getURL());

Not necessary here

src/Forms/HTMLEditor/TinyMCEConfig.php

GuySartorelli · 2022-12-13T03:40:51Z

src/Forms/HTMLEditor/TinyMCEConfig.php

@@ -666,7 +666,7 @@ protected function getConfig()
        $plugins = [];
        foreach ($this->getPlugins() as $plugin => $path) {
            if ($path instanceof ModuleResource) {
-                $path = Director::absoluteURL($path->getURL());
+                $path = Director::absoluteURL((string) $path->getURL());


Suggested change

$path = Director::absoluteURL((string) $path->getURL());

$path = Director::absoluteURL($path->getURL());

Not needed here

GuySartorelli · 2022-12-13T03:41:09Z

src/Control/RequestHandler.php

@@ -581,7 +581,7 @@ public function Link($action = null)
     */
    public function redirect(string $url, int $code = 302): HTTPResponse
    {
-        $url = Director::absoluteURL($url);
+        $url = Director::absoluteURL((string) $url);


Suggested change

$url = Director::absoluteURL((string) $url);

$url = Director::absoluteURL($url);

Not needed here

GuySartorelli · 2022-12-13T03:43:54Z

src/Control/HTTP.php

     * function($url) {
-     *      return Director::absoluteURL($url, true);
+     *      return Director::absoluteURL((string) $url, true);


Lets do function(string $url) here instead to promote good standards

GuySartorelli · 2022-12-13T03:46:22Z

src/Control/HTTP.php

@@ -52,7 +52,7 @@ public static function filename2url($filename)
        }

        $relativePath = ltrim(substr($filename ?? '', $baseLength ?? 0), '/\\');
-        return Director::absoluteURL($relativePath);
+        return Director::absoluteURL((string) $relativePath);


Suggested change

return Director::absoluteURL((string) $relativePath);

return Director::absoluteURL($relativePath);

emteknetnz force-pushed the pulls/5/absolute-link branch from 10c7b02 to 2ffa27d Compare December 8, 2022 21:17

GuySartorelli requested changes Dec 13, 2022

View reviewed changes

emteknetnz force-pushed the pulls/5/absolute-link branch from 2ffa27d to 2e59d0f Compare December 13, 2022 21:57

FIX Cast absoluteUrl() argument to string

700288d

emteknetnz force-pushed the pulls/5/absolute-link branch from 2e59d0f to 700288d Compare December 13, 2022 22:24

GuySartorelli approved these changes Dec 14, 2022

View reviewed changes

GuySartorelli merged commit aefa37f into silverstripe:5 Dec 14, 2022

GuySartorelli deleted the pulls/5/absolute-link branch December 14, 2022 02:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Cast absoluteUrl() argument to string #10610

FIX Cast absoluteUrl() argument to string #10610

emteknetnz commented Dec 8, 2022

GuySartorelli left a comment

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

GuySartorelli Dec 13, 2022

emteknetnz Dec 13, 2022

	Director::absoluteURL((string) $link),
	Director::absoluteURL($link),

	$currentUrl = $this->absoluteUrl((string) $request->getURL(true));
	$currentUrl = $this->absoluteUrl($request->getURL(true));

	return Director::absoluteURL((string) $resource->getRelativeResource($lang . '.js')->getURL());
	return Director::absoluteURL($resource->getRelativeResource($lang . '.js')->getURL());

	$path = Director::absoluteURL((string) $path->getURL());
	$path = Director::absoluteURL($path->getURL());

	$url = Director::absoluteURL((string) $url);
	$url = Director::absoluteURL($url);

	return Director::absoluteURL((string) $relativePath);
	return Director::absoluteURL($relativePath);

FIX Cast absoluteUrl() argument to string #10610

FIX Cast absoluteUrl() argument to string #10610

Conversation

emteknetnz commented Dec 8, 2022

GuySartorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment