Repository navigation
STF-2006: Add email verification inputs #295
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -487,14 +487,26 @@ public function withAccount( | |
| * @link https://dev.maxmind.com/minfraud/api-documentation/requests/?lang=en#schema--request--email | ||
| * minFraud email API docs | ||
| * | ||
| * @param array<string, mixed> $values An array of email data. The keys are the same as | ||
| * the JSON keys. You may use either this or the named | ||
| * arguments, but not both. | ||
| * @param string|null $address The email address used in the transaction. | ||
| * This field must be a valid email address. | ||
| * @param string|null $domain The domain of the email address used in the | ||
| * transaction. Do not include the `@` in this | ||
| * field. | ||
| * @param array<string, mixed> $values An array of email data. The keys are the same as | ||
| * the JSON keys. You may use either this or the named | ||
| * arguments, but not both. | ||
| * @param string|null $address The email address used in the transaction. | ||
| * This field must be a valid email address. | ||
| * @param string|null $domain The domain of the email address used in the | ||
| * transaction. Do not include the `@` in this | ||
| * field. | ||
| * @param string|null $verificationTime The date and time of the most recent | ||
| * attempt to verify the email address, if | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the This says "the most recent attempt to verify the email address, if known". The The two inputs have the same meaning: the time also applies when 🤖 Comment by Claude Opus 5.5.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is asking to change the phone text. So we can leave it I believe. |
||
| * known, in RFC 3339 date-time format. The | ||
| * time must not be before | ||
| * 1970-01-01T00:00:00Z or more than two days | ||
| * in the future. | ||
| * @param bool|null $wasVerificationSuccessful Whether the most recent verification | ||
| * that the end user controls the email | ||
| * address succeeded, for example by | ||
| * confirming a link sent to the address. | ||
| * Omit this if no verification was | ||
| * attempted. | ||
| * | ||
| * @throws InvalidInputException if a value has the wrong type, or if | ||
| * input validation is enabled and a value is | ||
|
|
@@ -507,6 +519,8 @@ public function withEmail( | |
| array $values = [], | ||
| ?string $address = null, | ||
| ?string $domain = null, | ||
| ?string $verificationTime = null, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: new parameters on
I reproduced this with PHP 8.3. The PR description says that positional callers do not break, which is correct, but it does not cover subclasses. The phone inputs cause the same break for 🤖 Comment by Claude Opus 5.5.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this is something we need to worry about.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should make it final in the next major version.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reply by Claude Opus 5.5 on Greg's behalf. Filed ENG-5698 to make the classes |
||
| ?bool $wasVerificationSuccessful = null, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: the PHP applies the caller's type mode to typed parameters. If the calling file does not declare The same problem exists in code that this PR did not add: I do not suggest a change in this PR. A fix would need a 🤖 Comment by Claude Opus 5.5.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a larger change not related exactly to this PR, if accurate and we wanted to do anything. |
||
| ): self { | ||
| if (\count($values) !== 0) { | ||
| if (\func_num_args() !== 1) { | ||
|
|
@@ -516,6 +530,12 @@ public function withEmail( | |
| } | ||
| $address = $this->remove($values, 'address'); | ||
| $domain = $this->remove($values, 'domain'); | ||
| $verificationTime = $this->remove($values, 'verification_time'); | ||
| $wasVerificationSuccessful = $this->remove( | ||
| $values, | ||
| 'was_verification_successful', | ||
| ['boolean'], | ||
| ); | ||
|
|
||
| $this->verifyEmpty($values); | ||
| } | ||
|
|
@@ -535,6 +555,15 @@ public function withEmail( | |
| $values['domain'] = $domain; | ||
| } | ||
|
|
||
| if ($verificationTime !== null) { | ||
| $this->verifyRfc3339DateTime($verificationTime); | ||
| $values['verification_time'] = $verificationTime; | ||
| } | ||
|
|
||
| if ($wasVerificationSuccessful !== null) { | ||
| $values['was_verification_successful'] = $wasVerificationSuccessful; | ||
| } | ||
|
|
||
| $new = clone $this; | ||
| $new->content['email'] = $values; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: in this example,
verificationTime(2026-10-01) is 14 years after the example eventtime(2012-04-12, line 168). The "most recent verification" then comes after the transaction that it describes.tests/data/minfraud/full-request.jsonhas the same mismatch:verification_time(line 16) is 2026-10-01 and the eventtime(line 6) is 2014-04-12.The
phoneVerificationTimevalues (lines 192 and 207) have the same problem. They are not new in this PR.Suggested fix: use a verification time before the event time, or move the event time to a recent date.
🤖 Comment by Claude Opus 5.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reply by Claude Opus 5.5 on Greg's behalf.
Fixed in f6695a7. The example event time is now 2026-10-05, after every verification time. This also fixes the phone examples.