Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The /core/lib/Drupal/Core/Test/AssertMailTrait.php()
uses the result of the preg_match()
function in assertTrue()
. However preg_match()
function returns 1, 0 or FALSE.
protected function assertMailPattern($field_name, $regex, $message = '', $group = 'Other') {
$mails = $this->getMails();
$mail = end($mails);
$regex_found = preg_match("/$regex/", $mail[$field_name]);
if (!$message) {
$message = new FormattableMarkup('Expected text found in @field of email message: "@expected".', ['@field' => $field_name, '@expected' => $regex]);
}
return $this->assertTrue($regex_found, $message, $group);
}
Proposed resolution
Use data type casting to boolean.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff_17-21.txt | 1.3 KB | munish.kumar |
#21 | 3082859-21.patch | 1.6 KB | munish.kumar |
#17 | interdiff_2-17.txt | 1.08 KB | munish.kumar |
#17 | 3082859-17.patch | 1.89 KB | munish.kumar |
#2 | 3082859-2.patch | 678 bytes | Krzysztof Domański |
Comments
Comment #2
Krzysztof DomańskiNote that the assertMailPattern is not used anywhere so I added them to
UserAdminTest.php
in test only.Comment #5
samuel.mortensonThis is breaking tests for me that pass in D8 but not in D9 ref #3121551: Confirm D9 tests pass
Comment #7
Kristen PolFixing outdated tags.
Comment #8
Kristen PolWhoops. The "Drupal 9 compatibility" tag is for contributed projects and not for core. Not sure if "Drupal 9 readiness" should be added since this is already marked for Drupal 9 so leaving it off. Sorry for the noise.
Comment #9
samuel.mortensonI had to override this method to port my module to D9, this change is pretty simple so I'm marking as RTBC.
Comment #10
xjmSo custom assertions returning a value and that value being meaningful is a legacy SimpleTest thing. PHPUnit fails the test as soon as an assertion fails, and PHPUnit 8 specifically specifies a void typehint as the return value for assertions.
I think what we should do here is actually deprecate there being a return value somehow. Contrib tests should not be relying on return values fmor assertions. This also came up in [edit, wrong link, looking for the right one].
That said, this assertion is clearly more broken ATM than just legacy, so followup?
Comment #11
mondrakeDefinitely +1 on #10!
Comment #12
samuel.mortensonThe return behavior of this method is not related to the bug, it's the assertion itself.
Comment #13
xjmRe: #12 Right, sorry for not being clear, the method is obvs. bugged ATM regardless of the
return
. It's just that returning true is also legacy.Which makes it occur to me that we also need test coverage (not for the return value, but that it passes when it should).
Comment #14
xjmIn #10, #3131900: Refactor assertions that assign return values to variables is the actual issue I had in mind. See also #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods.
Also this patch will conflict with #3129006: Modernize Drupal\KernelTests\AssertContentTrait but it's a one-line conflict to resolve manually for that patch, so not terrible either way.No it doesn't; it just conflicts with the different scope I had in my head.Comment #15
xjmLooks like there is already
core/tests/Drupal/KernelTests/Core/Test/AssertMailTraitTest.php
for this, so adding test coverage for this could be a good novice task. Thanks!Comment #16
shaktikComment #17
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @xjm,
Please review the patch as this is the first time I am writing the test cases, So please help me out with this. I have also gone through the Add a 'void' return typehint to custom assert* methods related issues. I have added a patch with the test cases only and not refactoring the return statement in
/core/lib/Drupal/Core/Test/AssertMailTrait.php()
Which will fix in other issues.Comment #18
shaktikUnassigned myself, Thanks @munish.kumar to fix this.
Comment #19
Krzysztof Domański@munish.kumar tests looks good. I have some suggestions for improvement.
1. Can we add a more complex regular expression here?
For example
"^.{32}@example\.com$"
2. Let's add tests for several parameters (e.g. "subject", "body", "to").
Comment #20
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #21
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @Krzysztof Domański, Thanks for your suggestions, I have made some improvements in test cases as mentioned in #19. Please review.
Comment #22
Krzysztof DomańskiTests looks good. Back to RTBC (#9). Follow-up has been created (#13, #14).
Comment #24
catchCommitted b829de6 and pushed to 9.1.x. Thanks!