Problem/Motivation

Discovered when starting to get PHPStan running against Drupal core in #3178534: Start running PHPStan on Drupal core (level 0), several errors were reported where the void return value of PHPUnit assertions were returned. These are bugs where the documentation says the method returns TRUE in the case of success, in fact this is not the case and the method returns NULL (and an exception is thrown if the assertion fails, so FALSE is never returned either).

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3204764-2.patch26.83 KBlongwave

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3204764-2.patch26.83 KB

There is an argument that each of these methods should always return TRUE as per the docs, but as they currently return NULL I think we should follow PHPUnit convention and also return NULL from our custom assertion helpers.

Status: Needs review » Needs work

The last submitted patch, 2: 3204764-2.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
Related issues: +#3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods

Sould be good to add a void typehint to the assertions then; it would be good to do here so the changes are enforced. Otherwise, #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods is already a follow-up.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3131900: Refactor assertions that assign return values to variables

Is #3131900: Refactor assertions that assign return values to variables now a duplicate/obsolete then?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

That is related but I don't think it's an exact duplicate? This one is hopefully more straightforward because the current docs are just wrong for each of these methods, so to me this is just a straight cleanup.

mondrake’s picture

mondrake
Location 🇮🇹
commented

#6 agree

  • catch committed 7e1043e on 9.2.x 
    Issue #3204764 by longwave, mondrake: PHPUnit assertions do not return a...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Makes sense to fix the documentation here then do actual refactoring in the other issue.

Committed 7e1043e and pushed to 9.2.x. Thanks!

  • catch committed 4a850c2 on 9.1.x 
    Issue #3204764 by longwave, mondrake: PHPUnit assertions do not return a...
mondrake’s picture

mondrake
Location 🇮🇹
commented 
+++ b/core/modules/system/tests/src/Functional/Module/ModuleTestBase.php
@@ -47,9 +47,9 @@ public function assertTableCount($base_table, $count = TRUE) {
     if ($count) {
-      return $this->assertNotEmpty($tables, new FormattableMarkup('Tables matching "@base_table" found.', ['@base_table' => $base_table]));
+      $this->assertNotEmpty($tables, new FormattableMarkup('Tables matching "@base_table" found.', ['@base_table' => $base_table]));
     }
-    return $this->assertEmpty($tables, new FormattableMarkup('Tables matching "@base_table" not found.', ['@base_table' => $base_table]));
+    $this->assertEmpty($tables, new FormattableMarkup('Tables matching "@base_table" not found.', ['@base_table' => $base_table]));
   }
 
   /**

Strange that this is not failing... now this is asserting both emptiness and non emptiness in case $count is true, one of the two should fail.

  public function assertTableCount($base_table, $count = TRUE) {
    $connection = Database::getConnection();
    $tables = $connection->schema()->findTables($connection->prefixTables('{' . $base_table . '}') . '%');

    if ($count) {
      $this->assertNotEmpty($tables, new FormattableMarkup('Tables matching "@base_table" found.', ['@base_table' => $base_table]));
    }
    $this->assertEmpty($tables, new FormattableMarkup('Tables matching "@base_table" not found.', ['@base_table' => $base_table]));
  }
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Oh yes, well spotted. The issue is this is only ever called once, the TRUE path is never exercised:

core/modules/system/tests/src/Functional/Module/DependencyTest.php
48:    $this->assertTableCount('language', FALSE);

Status: Fixed » Closed (fixed)

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