Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates URL handling so that when a caller provides a URL containing a non-default port, the check/fetch logic preserves that port when constructing the /.well-known/security.txt and /security.txt fetch URLs.
Changes:
- Extend URL parsing tests to assert port extraction and default-port normalization.
- Update
SecurityTxtFetcher::fetch()to build well-known/top-level URLs from the providedUrl(preserving port) instead of reconstructing from host only. - Add a fetcher test ensuring constructed/final URLs include an explicitly specified port.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Parser/SecurityTxtUrlParser.php |
Rejects parsed URLs that have no host (prevents accepting inputs like example.com:4433 as a “scheme”). |
src/Fetcher/SecurityTxtFetcher.php |
Constructs fetch URLs via withScheme/withPath to preserve port; adds InvalidUrlException to the declared throws. |
src/Check/SecurityTxtCheckHost.php |
Updates doc comment to state that port is used. |
tests/Parser/SecurityTxtUrlParserTest.phpt |
Adds port expectations to URL parsing cases and adds a “no scheme with port” bad-input case. |
tests/Fetcher/SecurityTxtFetcherTest.phpt |
Adds coverage that fetch preserves a non-default port in constructed/final URLs. |
Comments suppressed due to low confidence (1)
src/Check/SecurityTxtCheckHost.php:87
SecurityTxtCheckHost::check()callsSecurityTxtFetcher::fetch(), which is now annotated to throwUri\WhatWg\InvalidUrlException, butcheck()'s docblock doesn't mention it and it isn't handled. Either handle/wrap it insidecheck()/fetch()or add the missing@throwsto keep the public API documentation accurate.
/**
* @param Url $url Only the host and port parts of the URL will be used
* @param non-negative-int|null $maxAllowedRedirects
* @throws SecurityTxtHostNotFoundException
* @throws SecurityTxtCannotParseHostnameException
* @throws SecurityTxtCannotOpenUrlExtensionNotLoadedException
* @throws SecurityTxtTooManyRedirectsException
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.