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

Jump to bottom

Create none signer #160

Merged
merged 6 commits into from
Feb 9, 2017
Merged

Create none signer #160

merged 6 commits into from
Feb 9, 2017

Conversation

lcobucci
Copy link
Owner

@lcobucci lcobucci commented Feb 4, 2017

Fixes #152

@lcobucci lcobucci added this to the 4.0.0 milestone Feb 4, 2017
@lcobucci lcobucci self-assigned this Feb 4, 2017
@lcobucci lcobucci requested a review from Ocramius February 4, 2017 19:05
@lcobucci lcobucci changed the title Create none signer [WIP] Create none signer Feb 4, 2017
@lcobucci
Copy link
Owner Author

lcobucci commented Feb 4, 2017

@Ocramius I like the final result but still have mixed feelings regarding creating "useless" objects (Key and Signature).

I can also convert the new Signature('', '') to a named constructor. I don't think it makes sense to use inheritance there (to have a special case object)...

Ocramius
Ocramius previously requested changes Feb 4, 2017
View reviewed changes
Copy link
Collaborator

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

I can also convert the new Signature('', '') to a named constructor. I don't think it makes sense to use inheritance there (to have a special case object)...

That's indeed something I'd suggest as well. Creating "useless" objects shouldn't be a problem, given that crypto performance is much more relevant than no-crypto-performance in this lib.

test/unit/Validation/Constraint/ValidAtTest.php
@@ -38,6 +38,7 @@ public function createDependencies(): void
*
* @uses \Lcobucci\JWT\Token\DataSet
* @uses \Lcobucci\JWT\Token\Plain
* @uses \Lcobucci\JWT\Token\Signature
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed all these @uses: since I didn't notice before, what is the conveyed intent?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Although the objects are used I don't want to make PHPUnit mark them as covered on the coverage report.

@covers and @uses are a requirement since I configured PHPUnit to be strict about these annotations.

@lcobucci
Copy link
Owner Author

lcobucci commented Feb 4, 2017

That's indeed something I'd suggest as well.

@Ocramius any suggestion regarding the method name? Not so creative right now 😜

btw I was also thinking about adding $signingKey and $verificationKey (maybe with a different name) on the Configuration and change the method that configure the signer to configureSigner(Signer $signer, Key $signingKey, ?Key $verificationKey): void so people can configure these things in a single operation (since they're related), do you have any opinion regarding this?

@Ocramius
Copy link
Collaborator

Ocramius commented Feb 4, 2017

@Ocramius any suggestion regarding the method name?

Start from a sentence explaining what you are doing when creating an empty signature.

so people can configure these things in a single operation (since they're related), do you have any opinion regarding this?

Seems like a good idea, since you'd never want just one or two of the three.

@lcobucci lcobucci changed the title [WIP] Create none signer Create none signer Feb 6, 2017
@lcobucci lcobucci dismissed Ocramius’s stale review February 6, 2017 22:40

Feedback processed

@lcobucci
Copy link
Owner Author

lcobucci commented Feb 6, 2017 •
edited
Loading

@Ocramius I've added the named constructor, can you review the PR again please?

Seems like a good idea, since you'd never want just one or two of the three.

I'll create an issue to do that 😉 (#161 created)

@lcobucci lcobucci merged commit 7292df1 into master Feb 9, 2017
@lcobucci lcobucci deleted the create-none-signer branch February 9, 2017 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henriquemoody henriquemoody henriquemoody approved these changes

@Ocramius Ocramius Ocramius left review comments

Assignees

@lcobucci lcobucci

Labels
Improvement
Projects
None yet
Milestone
4.0.0-alpha1
Development

Successfully merging this pull request may close these issues.
8 participants
@lcobucci @Ocramius @henriquemoody @dobin @geggleto @schnittstabil @cybershambles @paragonie-scott