Skip to content

Simplify UriSigner when working with HttpFoundation's Request #35284

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.

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

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Jan 9, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

I'm using the UriSigner in my own projects from time to time and I've always wondered why I have to manually generate the URI from the Request instance in such a way that it is correctly validated.
Let's add a new checkRequest(Request $request) method to provide better DX.

@Toflar
Copy link
Contributor Author

Toflar commented Jan 9, 2020

Failing tests look unrelated.

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

with two minor comments

@@ -39,7 +41,7 @@ public function __construct(string $secret, string $parameter = '_hash')
*
* @return string The signed URI
*/
public function sign(string $uri)
public function sign(string $uri): string
Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted

@@ -59,7 +61,7 @@ public function sign(string $uri)
*
* @return bool True if the URI is signed correctly, false otherwise
*/
public function check(string $uri)
public function check(string $uri): bool
Copy link
Member

Choose a reason for hiding this comment

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

Should be reverted

@Toflar
Copy link
Contributor Author

Toflar commented Jan 10, 2020

Done. Thought they were missing accidentally during the upgrade to 5.0.

@fabpot fabpot force-pushed the simplify-urisigner-check-for-request branch from 7363ceb to 4887b4b Compare January 10, 2020 09:29
@fabpot
Copy link
Member

fabpot commented Jan 10, 2020

Thank you @Toflar.

@fabpot fabpot closed this in d099bc3 Jan 10, 2020
@fabpot fabpot merged commit 4887b4b into symfony:master Jan 10, 2020
@Toflar Toflar deleted the simplify-urisigner-check-for-request branch January 10, 2020 09:29
@fabpot fabpot mentioned this pull request May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants