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.

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

Configuration options modified #195

Merged
merged 1 commit into from
Jul 9, 2016
Merged

Configuration options modified #195

merged 1 commit into from
Jul 9, 2016

Conversation

Spomky
Copy link
Contributor

@Spomky Spomky commented Jul 9, 2016

This PR modifies the following configuration option:

  • the encryption_algorithm is renamed into signature_algorithm.
    Documentation is updated accordingly.

This PR includes minor modifications on the service.xml file.

@Spomky
Copy link
Contributor Author

Spomky commented Jul 9, 2016

@chalasr as discussed yesterday

@chalasr
Copy link
Collaborator

chalasr commented Jul 9, 2016

@Spomky I just opened #196 that makes the *_key_path options nullable.

Sorry for that you worked on this here before.
Fortunately it is not the only thing you did, the change from encryption_algorithm to signature_algorithm is legit to me 👍 .

Could you please revert the changes you did that are done by #196?
I'll properly review the rest of this and be back.

Also the StyleCI analysis doesn't pass, you should be able to fix it by running vendor/bin/php-cs-fixer fix (be careful that it doesn't modify files untouched by this PR, but normally it shouldn't).

Thank you

@chalasr
Copy link
Collaborator

chalasr commented Jul 9, 2016

Here are the changes that need to be reverted:

This commit modifies the following configuration option:
* the encryption_algorithm is renamed into signature_algorithm.
Documentation is updated accordingly.

This commit includes minor modifications on the service.xml file.
@Spomky
Copy link
Contributor Author

Spomky commented Jul 9, 2016

Hi @chalasr
I reverted my changes according to your previous comment.
Commits squashed.

@chalasr chalasr merged commit 72c65b7 into lexik:2.0 Jul 9, 2016
@chalasr
Copy link
Collaborator

chalasr commented Jul 9, 2016

Thank you @Spomky.
Could you please review #196?

@chalasr chalasr mentioned this pull request Aug 22, 2016
chalasr added a commit that referenced this pull request Sep 1, 2016
This PR was merged into the 2.0 branch.

Discussion
----------

Next to #195 where we changed `encryption_algorithm` to `signature_algorithm`, this adapt the (forgotten) existing code, changing `encryptionAlgorithm` to `signatureAlgorithm` everywhere.

2.0 being not yet released, we must profit to ensure that the introduced features are following good practices, conventions and naming.

@Spomky as you are quite aware of the good practices in term of encoding in general, it would be awesome if you could do a quick review of the terms we use in the corresponding part of our code base, especially on the `2.0` branch after #162.

For instance I thought about `encryption_engine`, is it always correct in the contexts we use it?
Otherwise, what about simply name it `engine`? That would give `encoder.engine` as option name.

Let me know about your suggestions!

Commits
-------

a0b4430 Change encryptionAlgorithm to encryptionEngine Use 'crypto' term for keys
gzim324 pushed a commit to gzim324/LexikJWTAuthenticationBundle that referenced this pull request Jan 10, 2025
* Revert "Specific ocramius/package-versions version"

* Try to fix Travis mongodb install
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.

2 participants