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

API Add AbsoluteLink method to RequestHandler #10749

Merged
merged 1 commit into from
Apr 26, 2023
Merged

API Add AbsoluteLink method to RequestHandler #10749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 21 additions & 2 deletions src/Control/RequestHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,13 @@ public function setRequest(HTTPRequest $request): static
}

/**
* Returns a link to this controller. Overload with your own Link rules if they exist.
* Returns a link to this controller.
* Returns null if no link could be generated.
*
* Overload with your own Link rules if they exist.
*
* @param string $action Optional action
* @return string
* @return ?string
*/
public function Link($action = null)
{
Expand All @@ -576,6 +579,22 @@ public function Link($action = null)
return null;
}

/**
* Get the absolute URL for this controller, including protocol and host.
* Returns null if no link could be generated.
*
* @param string $action See {@link Link()}
* @return ?string
*/
public function AbsoluteLink($action = '')
michalkleiner marked this conversation as resolved.
Show resolved Hide resolved
{
$link = $this->Link($action);
if ($link === null) {
return null;
}
return Director::absoluteURL((string) $link);
}

/**
* Redirect to the given URL.
*/
Expand Down
97 changes: 97 additions & 0 deletions tests/php/Control/RequestHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
<?php

namespace SilverStripe\Control\Tests;

use SilverStripe\Control\Controller;
use SilverStripe\Control\Director;
use SilverStripe\Control\RequestHandler;
use SilverStripe\Dev\SapphireTest;

/**
* Tests for the RequestHandler class
*/
class RequestHandlerTest extends SapphireTest
{
protected $usesDatabase = false;

public function provideTestLink(): array
{
return [
// If there's no url segment, there's no link
[
'urlSegment' => null,
'action' => null,
'expected' => null,
],
[
'urlSegment' => null,
'action' => 'some-action',
'expected' => null,
],
// The action is just addeed on after the url segment
[
'urlSegment' => 'my-controller',
'action' => null,
'expected' => 'my-controller',
],
[
'urlSegment' => 'my-controller',
'action' => 'some-action',
'expected' => 'my-controller/some-action',
],
];
}

/**
* @dataProvider provideTestLink
*/
public function testLink(?string $urlSegment, ?string $action, ?string $expected)
{
if ($urlSegment === null) {
$this->expectWarning();
$this->expectWarningMessage('Request handler SilverStripe\Control\RequestHandler does not have a url_segment defined. Relying on this link may be an application error');
Comment on lines +51 to +52
Copy link
Member Author

@GuySartorelli GuySartorelli Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These are deprecated and will be removed in PHPUnit 10 - but I think it's worth having them here while they work. We've got this sort of thing in a few places and eventually we'll have to think about what to do with it, but PHPUnit has a history of long support for not-current major releases so I suspect we won't be worrying about that until CMS 6.
This is the only way currently to test that the warning is being emitted, and we can't just change the warning to an exception here without breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

They're showing up as deprecation warnings as CI. https://github.com/silverstripe/silverstripe-framework/actions/runs/4748341443/jobs/8434430162?pr=10749#step:12:135 We shouldn't be adding tech debt in new PRs. At some point we'll remove the other warnings that are currently there

There is no migration path sebastianbergmann/phpunit#5062 (comment)

Can you please remove these assertions

Copy link
Member Author

Choose a reason for hiding this comment

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

For the reasons I've expressed above, I think it's worth keeping these here - we probably need another opinion at this stage.
@silverstripe/core-team any thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: If I remove the assertions I also can't keep the test scenario where the returned value should be null, because phpunit converts the warning into an exception and the test fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe create a card to reach some consensus on how to handle warnings going forward. Those new PHPUnit warning are annoying enough that I think we should aim to address them now rather than wait for PHPUnit 10.

On the narrow point of what we should do with this PR, I think it makes sense to use the "old" way and expect a warning ... just in case we decide that we really like those "expect warning" clause in the follow up card and hack them back in somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue created: silverstripe/.github#40

}

$handler = new RequestHandler();
RequestHandler::config()->set('url_segment', $urlSegment);
Controller::config()->set('add_trailing_slash', false);

$this->assertEquals($expected, $handler->Link($action));

// Validate that trailing slash config is respected
Controller::config()->set('add_trailing_slash', true);
if (is_string($expected)) {
$expected .= '/';
}

$this->assertEquals($expected, $handler->Link($action));
}

/**
* @dataProvider provideTestLink
*/
public function testAbsoluteLink(?string $urlSegment, ?string $action, ?string $expected)
{
if ($urlSegment === null) {
$this->expectWarning();
$this->expectWarningMessage('Request handler SilverStripe\Control\RequestHandler does not have a url_segment defined. Relying on this link may be an application error');
emteknetnz marked this conversation as resolved.
Show resolved Hide resolved
}

$handler = new RequestHandler();
RequestHandler::config()->set('url_segment', $urlSegment);
Controller::config()->set('add_trailing_slash', false);

if ($expected !== null) {
$expected = Director::absoluteURL($expected);
}
$this->assertEquals($expected, $handler->AbsoluteLink($action));

// Validate that trailing slash config is respected
Controller::config()->set('add_trailing_slash', true);
if (is_string($expected)) {
$expected = Director::absoluteURL($expected . '/');
}

$this->assertEquals(Director::absoluteURL($expected), $handler->AbsoluteLink($action));
}
}
2 changes: 1 addition & 1 deletion tests/php/Control/RequestHandlingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use SilverStripe\Security\SecurityToken;

/**
* Tests for RequestHandler and HTTPRequest.
* Tests for functionality related to handling requests - not unit tests for RequestHandler.
Copy link
Member Author

Choose a reason for hiding this comment

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

Modified this note because I was originally going to add the link tests in here, but then I noticed nothing in here is actually directly testing RequestHandler despite its name kiiinda suggesting that's what this class would be.

* We've set up a simple URL handling model based on
*/
class RequestHandlingTest extends FunctionalTest
Expand Down