Repository navigation
Add a Contract for URL Generation Services - #387
Merged
Merged
Conversation
abdulmajidcse
left a comment
Member
There was a problem hiding this comment.
@techmahedy Thanks for the PR! A couple of things should be fixed before merging:
isValid(string $path)breaks on null (UrlGenerator.php:288).URL::isValid($request->input('website'))with a missing field used to returnfalse; now it throws aTypeError. Consider acceptingmixedand returningfalsefor non-strings.- Aliasing
'url'to the interface (FacadeLauncher.php:79). Becausebind()follows aliases, any laterbind('url', ...)overwrites theUrlGeneratorInterfacebinding. Several tests already do this (e.g.tests/Model/CastSystemTest.php:196), after which type-hinting the contract fails.
Smaller points:
- The
url()helper resolvesUrlGenerator::class, which isn't aliased, so it may not get the shared singleton. Addingalias(UrlGeneratorInterface::class, UrlGenerator::class)would fix it. - The
full()docblock says "without query parameters", butfullUrl()includes them. - The
URLfacade@methoddocs still reference the concrete class and old signatures. - Existing issue, worth considering before the contract locks it in: the singleton builder keeps query/expiration state between calls (e.g.
URL::signed(...)thenroute('home')leaks?id=..&signature=..).
4 tasks done
Member
Author
|
@abdulmajidcse thanks for the review, but i am going to merge cause
|
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.
Pull Request Checklist
Description
Introduce a URL generator contract so Doppar services can depend on an abstraction rather than the concrete
UrlGeneratorimplementation.Changes
Phaseolies\Support\Contracts\UrlGeneratorInterface.UrlGeneratorto implement the contract.isValid(string $path).urlcontainer key as an alias.Backward Compatibility
Existing
app('url'), URL helpers, route URL generation, fluent URL construction, signed URLs, and security settings continue to work. The contract and legacyurlservice key resolve to the same singleton instance.Checklist