API Add AbsoluteLink method to RequestHandler #10749
Conversation
This is a method that is commonly implemented on controllers, but it really doesn't need to be.
GuySartorelli
force-pushed
the
pulls/5/absolutelink-on-requesthandler
branch
from
b4ac201 to
567484f
Compare
|
Unit test added - good call. Also made the method correctly handle the scenario when |
| $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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Issue created: silverstripe/.github#40
| * Tests for RequestHandler and HTTPRequest. | ||
| * Tests for functionality related to handling requests - not unit tests for RequestHandler. |
There was a problem hiding this comment.
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.
| $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'); |
There was a problem hiding this comment.
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
This is a method that is commonly implemented on controllers, but it really doesn't need to be.
This is a method that is commonly implemented on controllers, but it really doesn't need to be.
Issue
AbsoluteLink()onRequestHandler#10753