Follow-up to #1423244: #allowed_formats property for #type 'text_format' to filter available formats
The attached patch is for the followup: The return should be documented. Also, I think the return value should be the result of the assertFalse() not $select.
Ideally, all assertSomething() functions should return a bool indicating pass or fail. However, currently, assertSomething() functions exist all over Core that do not return a value, not just the Filter module.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2284383-8-assert-return.patch | 5.19 KB | tstoeckler |
| assertions-return-value.patch | 812 bytes | lokapujya |
Comments
Comment #1
lokapujyaComment #2
lokapujyaComment #3
gábor hojtsyWell, if we use the return value then we should rename the method. We can still assert the textarea there.
Comment #4
Crell commentedWe're trying to move away from Simpletest anyway toward PHPUnit, at which point this is no longer our problem. Should we even bother? This seems like unnecessary busy work.
Comment #5
lokapujyaYeah, the wider scope of updating asserts would be most efficiently tackled as we move the tests to phpUnit.
As is, the missing return documentation that is fixed by this patch causes a code standards warning in some modern IDE's. If we want the patch as exists, someone can RTBC it, or we can postpone that also.
Comment #6
lokapujyaComment #7
gábor hojtsyThe remaining asserts added in #1423244: #allowed_formats property for #type 'text_format' to filter available formats would need to be covered as well. All need the return value documented and actually returned as well.
Comment #8
lokapujyaSetting to Novice for fixing the remaining asserts() mentioned in #7. (meaning just the new asserts added by that issue to the Filter module.)
Comment #9
tstoecklerRe #3: The return value is actually never used (and pointless). No clue why I put that there, that shouldn't be there.
Here's an updated patch which fixes this issue for
FilterFormTest. Not sure if we want to make this a fix-all issue or not. As mentioned above (almost ?) allassertXmethods are currently lacking return value documentation.Comment #10
gábor hojtsyLooks good.
Comment #11
alexpottCommitted 47b25e9 and pushed to 8.x. Thanks!