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
As title
Proposed resolution
Example:
- $this->assertTrue(is_numeric($checked_time));
+ $this->assertIsNumeric($checked_time);
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#30 | 3131817-28-9.1.patch | 4.79 KB | quietone |
#30 | interdiff-25-28.txt | 641 bytes | quietone |
#30 | 3131817-28-8.9.patch | 5.16 KB | quietone |
Comments
Comment #2
jungleComment #3
quietone CreditAttribution: quietone as a volunteer commentedI used this
egrep -r ".*assert.*is_numeric" core | awk -F: '{print $1}' | nl
to find lines to change.Comment #4
quietone CreditAttribution: quietone as a volunteer commentedNR for testing.
Comment #5
jungleThanks, @quietone!
Checked with regex
assert.+is_numeric
, no more assertions to replace. But setting back to NW to remove the assertion messages. See #3130811: Remove redundant $message from assert(Not)InstanceOf calls and #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages for more.Comment #6
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedPlease review the patch.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedOh my, I was commenting and uploading a my new patch when I found out it was already done. @Suresh Prabhu Parkala, to avoid more than one person working on a patch please assign it to yourself. Or, in this case, since it was only an hour since I uploaded my patch maybe ask first or wait a day?
@jungle, I have looked at the two issues referred to plus the Meta for this one and I don't see any decision in those issues that states that messages are to be moved from all assertions. This isn't an area I know much about, maybe that requirement should be added to the Meta?
Comment #9
jungle@quietone, yes, it's still unclear. But #3130811 got committed already which did remove assertion messages. and @catch as the committer said before +1 to doing so.
Setting back to NW, as testing in #3 failed, and #6 should not pass
Comment #10
jungleComment #11
quietone CreditAttribution: quietone as a volunteer commentedI was a bit overzealous in converting the assertions.
Comment #12
jungleLGTM, thanks @quietone!
Comment #13
catchI think the assertion message here is worth converting into a comment - it's a long comparison to scan.
Comment #14
jungleThank you @catch!
Checking the whole method, found that if we add an inline comment to ② or ③, it is redundant with the method level comment ①
Asserts that numbers are either both negative, both positive or both zero.
I am setting back to RTBC, if you think it's still necessary, I am happy to submit a new patch.
Comment #15
jungleSetting back to NW waiting for testings finishing running, and checking next if a new patch is necessary for 8.x
Comment #16
daffie CreditAttribution: daffie commentedThe method
assertIsNumeric()
does not exist for Drupal 8 and the patch does not apply for 8.8.Comment #17
jungleRe #16, forwards-compatibility shim for assertIsNumeric() added in 8.x, see #3126787: [D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7
Comment #18
daffie CreditAttribution: daffie commentedPostponing this issue on #3126787: [D8 only] Add forwards-compatibility shim for assertInternalType() replacements in phpunit 6&7.
Comment #19
daffie CreditAttribution: daffie commentedPatch looks good for 9.0. Just 1 nitpick:
Double round brackets are not needed.
Comment #20
xjmThe shim is committed to 8.9.x, which might be as far as it goes. So this can continue.
Comment #21
mondrakeThese can be
$this->assertGreaterThan(0, $test_mtime);
Removal of the custom $message is out of scope of this patch. (It could be part of a general issue chain to remove redundant messages, but not here specifically)
Comment #22
quietone CreditAttribution: quietone as a volunteer commentedFixes for #19 and #21.1.
#21,2. No change. In #5 it was requested that messages are to be removed. See also #8 and #9.
Comment #23
mondrakeRe. #21.2, in this case this is in an assert method. If you drop the message entirely, all you'll get in case of test failure will be
Failed asserting that false is true.
which is very obscure since you were calling
$this->assertBothNegativePositiveOrZero($expected, $result);
.So probably we may take PHPUnit's way here and come up with sth like
Failed asserting that both numbers are negative, or both positive, or both zero.
But again IMHO this is out of scope in this issue.
Comment #25
quietone CreditAttribution: quietone as a volunteer commentedYes, I see. This fixes 21.2
And for my own sanity I see that was introduced in #6 and I missed it.
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedArg, I didn't mean to abort the test.
Comment #27
mondrakeThis is a testing a component, and extending the test directly from PHPUnit\Framework\TestCase, so by itself does not have the logic to load the compatibility methods.
To get this running on D8.9, we'll have to add the trait
or, just leave this untouched in D8.9.
Comment #28
mondrakeI think this is OK for D9 where it’s not been committed, yet. Needs a specific D8.9 patch.
Comment #29
mondrakeComment #30
quietone CreditAttribution: quietone as a volunteer commentedThanks. Decided to make a patch for 8.9.x. The 9.1.x patch here is the same as the one in #25, just renamed.
Comment #31
mondrakeComment #32
xjmAlright, I'm OK with the comment from #14 and looked at the method myself, and I'm confortable since the method is so short and it's documented in the docblock that that is what it does.
Saving credit while I've pushed 9.1.x only to do an experiment about a problem I'm getting with crossposts from the bot...
Comment #36
xjmCommitted to 9.1.x, 9.0.x, and 8.9.x. Thanks!
Comment #37
xjm