There are a few problems we should definitely fix.

CommentFileSizeAuthor
#1 2284397_encrypt_codercs.patch12.31 KBgreggles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
FileSize
2284397_encrypt_codercs.patch12.31 KB

I didn't touch encryptfapi.
I didn't touch the .test files.
There are several warnings about the order of arguments and defaults...not sure how to handle these:

phpseclib.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
20 | ERROR | Arguments with default values must be at the end of the argument
| | list

We could just remove the defaults on the first arguments in that function. You can't actually use them since the $key is required.
We could also reorder the arguments, but that would require changes in other modules which I don't really want to do.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Active » Needs review

Forgot status.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
CreditAttribution: rlhawk commented

This patch looks good to me.

For the arguments: $key could be assigned a default, the way the default encryption method does it:

$op = 'encrypt', $text = '', $key = '', $options = array()

I think removing the defaults on the first arguments is a better solution, though.

  • rlhawk committed 2bd9cf1 on 7.x-2.x 
    Issue #2284397 by greggles: Code style cleanup.
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
CreditAttribution: rlhawk commented
Status: Needs review » Fixed

This patch has been committed. I'll create a separate issue for fixing the default function arguments.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.