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

Use constant for supported formats #26323

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Conversation

J0WI
Copy link
Contributor

@J0WI J0WI commented Mar 26, 2021

Supported since PHP 5.6.

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
@@ -57,10 +57,20 @@
* @package OCA\Encryption\Crypto
*/
class Crypt {
public const SUPPORTED_CIPHERS_AND_KEY_SIZE = [
Copy link
Member

Choose a reason for hiding this comment

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

why does their visibility change all of a sudden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEFAULT_CIPHER and LEGACY_CIPHER are also public, so IMHO it makes sense to have the same visibility for all available ciphers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rullzer should I revert this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#26383 depends on this

@J0WI J0WI requested a review from rullzer April 12, 2021 12:22
@J0WI J0WI requested a review from LukasReschke April 20, 2021 12:53
@J0WI
Copy link
Contributor Author

J0WI commented May 18, 2021

asking @nextcloud/encryption for a second review 😃

@J0WI J0WI added this to the Nextcloud 22 milestone Jun 23, 2021
@blizzz blizzz mentioned this pull request Jun 23, 2021
39 tasks
@blizzz blizzz merged commit c6d5653 into nextcloud:master Jun 23, 2021
@J0WI J0WI deleted the crypt-const branch June 23, 2021 15:10
@J0WI J0WI mentioned this pull request Jun 24, 2021
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.

4 participants