Problem/Motivation

PHPStan performs static analysis on a code-base to find bugs. PHPStan is run in our CI pipeline. PHPStan groups its lints into rule levels.

Since #3586383: PHPStan level 3 (9.0.x and 8.0.x), the 9.0.x and 8.0.x branches are at Rule Level 3. As of commit b80bd68 in 9.0.x and commit e5f6e29 in 8.0.x, when I test the module at level 4 (i.e.: by running ddev phpstan --level=4, it shows 29 lints.

Proposed resolution

Fix the lints, then update phpstan.neon so new merge requests must conform to at least level 4 level 5.

Remaining tasks

  1. Merge request 9.0.x - merge request !209 created by @mparker17 in #3
  2. Merge request 8.0.x - merge request !210 created by @mparker17 in #10
  3. Merge to 9.0.x - merged by @mparker17 in #12
  4. Merge to 8.0.x - merged by @mparker17 in #14
  5. Release 9.0.x - released in 9.0.0-alpha3 by @mparker17
  6. Release 8.0.x - released in 8.0.0-alpha7 by @mparker17

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork elasticsearch_connector-3586504

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mparker17 created an issue. See original summary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

For reference, here is the list of errors (output from ddev phpstan --level=4 --error-format=raw but slightly formatted)...

  1. src/Plugin/ElasticSearch/Connector/ElasticCloudEndpointConnector.php:54:If condition is always true.
  2. src/Plugin/ElasticSearch/Connector/ElasticCloudIdConnector.php:55:If condition is always true.
  3. src/Plugin/search_api/backend/ElasticSearchBackend.php:283:Instanceof between Drupal\elasticsearch_connector\Connector\ElasticSearchConnectorInterface and Drupal\Core\Plugin\PluginFormInterface will always evaluate to true.
  4. src/Plugin/search_api/backend/ElasticSearchBackend.php:301:Instanceof between Drupal\elasticsearch_connector\Connector\ElasticSearchConnectorInterface and Drupal\Core\Plugin\PluginFormInterface will always evaluate to true.
  5. src/Plugin/search_api/backend/ElasticSearchBackend.php:335:Property Drupal\elasticsearch_connector\Plugin\search_api\backend\ElasticSearchBackend::$client (Elastic\Elasticsearch\Client) in isset() is not nullable.
  6. src/Plugin/search_api/backend/ElasticSearchBackend.php:348:Property Drupal\elasticsearch_connector\Plugin\search_api\backend\ElasticSearchBackend::$backendClient (Drupal\elasticsearch_connector\SearchAPI\BackendClientInterface) in isset() is not nullable.
  7. src/Plugin/search_api/processor/HighlightQueryBuilderTrait.php (in context of class Drupal\elasticsearch_connector\Plugin\search_api\processor\ElasticsearchHighlighter):26:Call to function is_int() with string will always evaluate to false.
  8. src/Plugin/search_api/processor/HighlightQueryBuilderTrait.php (in context of class Drupal\elasticsearch_connector\Plugin\search_api\processor\ElasticsearchHighlighter):26:Call to function is_string() with string will always evaluate to true.
  9. src/Plugin/search_api/processor/HighlightQueryBuilderTrait.php (in context of class Drupal\elasticsearch_connector\Plugin\search_api\processor\ElasticsearchHighlighter):26:Result of || is always true.
  10. src/SearchAPI/BackendClient.php:163:Negated boolean expression is always false.
  11. src/SearchAPI/Query/QueryParamBuilder.php:114:Offset 'query' on array{from: mixed, size: mixed, sort?: non-empty-array} in empty() does not exist.
  12. src/SearchAPI/Query/QueryParamBuilder.php:124:Offset 'fields' on array{from: mixed, size: mixed, sort?: non-empty-array, query?: non-empty-array} in isset() does not exist.
  13. src/SearchAPI/Query/QueryParamBuilder.php:124:Result of && is always false.
  14. src/SearchAPI/Query/QueryParamBuilder.php:132:Offset 'post_filter' on array{from: mixed, size: mixed, sort?: non-empty-array, query?: non-empty-array, post_filter: array{bool: array{must: mixed}}} in empty() always exists and is not falsy.
  15. src/SearchAPI/Query/QueryParamBuilder.php:132:Result of && is always false.
  16. tests/src/Functional/ElasticsearchBackendTest.php:89:Using nullsafe method call on non-nullable type Drupal\Core\Entity\EntityTypeManager. Use -> instead.
  17. tests/src/Functional/ElasticsearchBackendTest.php:90:Using nullsafe method call on non-nullable type Drupal\search_api\Entity\SearchApiConfigEntityStorage. Use -> instead.
  18. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\CustomAnalyzerBackendTest):212:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  19. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\CustomAnalyzerBackendTest):274:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  20. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\ElasticsearchConnectorBackendTest):212:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  21. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\ElasticsearchConnectorBackendTest):274:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  22. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\ElasticsearchHighlighterTest):212:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  23. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\ElasticsearchHighlighterTest):274:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  24. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\FacetsIntegrationTest):212:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  25. tests/src/Functional/ElasticsearchTestViewTrait.php (in context of class Drupal\Tests\elasticsearch_connector\Functional\FacetsIntegrationTest):274:Instanceof between Behat\Mink\Element\NodeElement and Behat\Mink\Element\ElementInterface will always evaluate to true.
  26. tests/src/FunctionalJavascript/ConnectorTypeSwitcherTest.php:36:Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'Drupal\\FunctionalJavascriptTests\\JSWebAssert' and Drupal\FunctionalJavascriptTests\WebDriverWebAssert will always evaluate to true.
  27. tests/src/Kernel/ElasticSearchBackendClientTest.php:109:Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'Elastic\\Elasticsearch\\Client' and Elastic\Elasticsearch\Client will always evaluate to true.
  28. tests/src/Unit/SearchAPI/Query/QueryResultParserTest.php:93:Call to method PHPUnit\Framework\Assert::assertNotNull() with Drupal\search_api\Query\ResultSetInterface will always evaluate to true.
  29. tests/src/Unit/SearchAPI/Query/QueryResultParserTest.php:104:Call to method PHPUnit\Framework\Assert::assertNotNull() with Drupal\search_api\Item\FieldInterface will always evaluate to true.

mparker17 opened merge request !209

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Title: PHPStan level 4 (9.0.x and 8.0.x) » PHPStan levels 4 and 5 (9.0.x and 8.0.x)
Issue summary: View changes
Status: Active » Needs review

Turns out the fixes I made in the branch also fixed the lints for level 5, so we can go to level 5 in this issue.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

There were a few non-comment changes to production code in this patch worth noting. I'm going to discuss them (and provide some reasoning) in the next few comments (i.e.: to make it easy for the community to discuss if necessary).

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented 
diff --git a/src/SearchAPI/BackendClient.php b/src/SearchAPI/BackendClient.php
index 710a4692a257bb9dbd767dbb66fe575a79d329ae..1f9dc9e92007a13638171fc48c542de40a1a01f7 100644
--- a/src/SearchAPI/BackendClient.php
+++ b/src/SearchAPI/BackendClient.php
@@ -160,7 +160,7 @@ class BackendClient implements BackendClientInterface {
     ];
     try {
       // Check index exists.
-      if (!$this->client->indices()->exists($params)) {
+      if (!$this->client->indices()->exists($params)->asBool()) {
         $this->logger->warning('Index "%index" does not exist.', ["%index" => $indexId]);
         return $resultSet;
       }

$this->client->indices()->exists($params) returns either a \Http\Promise\Promise object (if async is set in the Elasticsearch client, which we don't do), or an \Elastic\Elasticsearch\Response\Elasticsearch object.

The problem here is that objects are truthy, so this would never log the warning, even if the index did not exist. Instead, code later in the same function would try to send a query, and fail with a ElasticSearchException, which get re-thrown as a SearchApiException.

The outcome of an "index exists" call (i.e.: either TRUE => "index exists" or FALSE => "index does not exist") is captured inside the response wrapped by the \Elastic\Elasticsearch\Response\Elasticsearch object.

Testing locally, I could see that calling \Elastic\Elasticsearch\Response\Elasticsearch::asBool() would return TRUE when the index exists, and FALSE when the index does not exist, which seems to be what we're actually trying to check here, so I changed this to simply call ->asBoo() on the old code.

The outcome of this was a test failure in \Drupal\Tests\elasticsearch_connector\Kernel\ElasticSearchBackendTest::checkModuleUninstall(), which deleted the index and expected the re-thrown SearchApiException mentioned earlier. ElasticSearchBackendTest::checkModuleUninstall overrides \Drupal\Tests\search_api\Kernel\BackendTestBase::checkModuleUninstall() which is empty (i.e.: a no-op). Anyway, now searching on a deleted index results in a log message and not a SearchApiException, I needed to make the following change to the test...

diff --git a/tests/src/Kernel/ElasticSearchBackendTest.php b/tests/src/Kernel/ElasticSearchBackendTest.php
index 31b0abd9037534f949d472a70cfa2c5042f93101..3b64e39b84bbc963a6931d49865ca152a514f3ec 100644
--- a/tests/src/Kernel/ElasticSearchBackendTest.php
+++ b/tests/src/Kernel/ElasticSearchBackendTest.php
@@ -306,8 +305,9 @@ class ElasticSearchBackendTest extends BackendTestBase {
     $server->getBackend()->removeIndex($index);
 
     $query = $this->buildSearch();
-    $this->expectException(SearchApiException::class);
-    $query->execute();
+    $results = $query->execute();
+
+    $this->assertEquals(0, $results->getResultCount());
   }
 
 }
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented 
diff --git a/src/SearchAPI/Query/QueryParamBuilder.php b/src/SearchAPI/Query/QueryParamBuilder.php
index 7785baa6ca8516eaf90cf63103d8f7426cbd5653..2e5229f7a75d0048d2e8d2b2534a830879d9227f 100644
--- a/src/SearchAPI/Query/QueryParamBuilder.php
+++ b/src/SearchAPI/Query/QueryParamBuilder.php
@@ -111,25 +111,18 @@ class QueryParamBuilder {
       $body['query']['bool']['filter'] = $filters["filters"];
     }
     elseif (!empty($searchParams)) {
-      if (empty($body['query'])) {
-        $body['query'] = [];
-      }
+      $body['query'] = [];
       $body['query'] += $searchParams;
     }
     elseif (!empty($filters["filters"])) {
       $body['query']['bool']['filter'] = $filters["filters"];
     }
 
-    // @todo Handle fields on filter query.
-    if (isset($body['fields']) && empty($body['fields'])) {
-      unset($body['fields']);
-    }
-
     if (!empty($filters["post_filters"])) {
       $body['post_filter']['bool']['must'] = $filters["post_filters"];
     }

This code is a bit convoluted; my guess is that it evolved a bit and we left some defensive checks even though they're no longer necessary.

In the following change...

-      if (empty($body['query'])) {
-        $body['query'] = [];
-      }
+      $body['query'] = [];

... this was the first time that $body['query'] would be set in this branch of the if/elseif, so the inner "if" statement wasn't necessary because it would always evalute to run the inner code.

Likewise, in the following change...

-    // @todo Handle fields on filter query.
-    if (isset($body['fields']) && empty($body['fields'])) {
-      unset($body['fields']);
-    }

... there was no way that body['fields'] could exist yet in this branch of the if/elseif, so the inner "if" statement wasn't necessary because it would always evaluate to skip the inner code.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented 
diff --git a/src/SearchAPI/Query/QueryParamBuilder.php b/src/SearchAPI/Query/QueryParamBuilder.php
index 7785baa6ca8516eaf90cf63103d8f7426cbd5653..2e5229f7a75d0048d2e8d2b2534a830879d9227f 100644
--- a/src/SearchAPI/Query/QueryParamBuilder.php
+++ b/src/SearchAPI/Query/QueryParamBuilder.php
@@ -111,25 +111,18 @@ class QueryParamBuilder {
     if (!empty($filters["post_filters"])) {
       $body['post_filter']['bool']['must'] = $filters["post_filters"];
     }
 
-    if (isset($body['post_filter']) && empty($body['post_filter'])) {
+    if (empty($body['post_filter'])) {
       unset($body['post_filter']);
     }

In this case, empty($body['post_filter']) won't throw a Warning: Undefined array key 'post_filter' if the post_filter key doesn't exist, and instead it returns the exact same thing as isset($body['post_filter']).

But if $body['post_filter'] does exist we still need to check whether its empty.

This meant that the call to \isset() was unnecessary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented 
diff --git a/src/Plugin/search_api/backend/ElasticSearchBackend.php b/src/Plugin/search_api/backend/ElasticSearchBackend.php
index f1b4aeb602398495a5552971650c1d80099ff533..ccd0ae40a965f9a46ce55baa16fc08351bdf3fd3 100644
--- a/src/Plugin/search_api/backend/ElasticSearchBackend.php
+++ b/src/Plugin/search_api/backend/ElasticSearchBackend.php
@@ -279,13 +279,14 @@ class ElasticSearchBackend extends BackendPluginBase implements PluginFormInterf
 
     // Check before loading the backend plugin so we don't throw an exception.
     $this->configuration['connector'] = $form_state->get('connector');
-    $connector = $this->getConnector();
-    if (!$connector instanceof PluginFormInterface) {
+    try {
+      $connector = $this->getConnector();
+      $connector_form_state = SubformState::createForSubform($form['connector_config'], $form, $form_state);
+      $connector->validateConfigurationForm($form['connector_config'], $connector_form_state);
+    }
+    catch (InvalidConnectorException) {
       $form_state->setError($form['connector'], $this->t('The connector could not be activated.'));
-      return;
     }
-    $connector_form_state = SubformState::createForSubform($form['connector_config'], $form, $form_state);
-    $connector->validateConfigurationForm($form['connector_config'], $connector_form_state);
   }
 
   /**
@@ -297,13 +298,18 @@ class ElasticSearchBackend extends BackendPluginBase implements PluginFormInterf
     $values['advanced']['synonyms'] = empty($synonyms) ? [] : explode(\PHP_EOL, $synonyms);
     $this->setConfiguration($values);
     $this->configuration['connector'] = $form_state->getValue('connector');
-    $connector = $this->getConnector();
-    if ($connector instanceof PluginFormInterface) {
+    try {
+      $connector = $this->getConnector();
       $connector_form_state = SubformState::createForSubform($form['connector_config'], $form, $form_state);
       $connector->submitConfigurationForm($form['connector_config'], $connector_form_state);
+
       // Overwrite the form values with type casted values.
       $form_state->setValue('connector_config', $connector->getConfiguration());
     }
+    catch (InvalidConnectorException) {
+      // No-op.
+    }
+
   }
 
   /**

In both of these cases, calling $this->getConnector() would throw an exception if there was a problem, and (uncaught) exceptions pop stack, so...

  1. if there was an error, PHP would never get to the if (!$connector instanceof PluginFormInterface) statement
  2. if there was not an error, the if (!$connector instanceof PluginFormInterface) would always evaluate to skip the inner code

So for this change...

-    $connector = $this->getConnector();
-    if (!$connector instanceof PluginFormInterface) {
+    try {
+      $connector = $this->getConnector();
+      $connector_form_state = SubformState::createForSubform($form['connector_config'], $form, $form_state);
+      $connector->validateConfigurationForm($form['connector_config'], $connector_form_state);
+    }
+    catch (InvalidConnectorException) {
       $form_state->setError($form['connector'], $this->t('The connector could not be activated.'));
-      return;
     }
-    $connector_form_state = SubformState::createForSubform($form['connector_config'], $form, $form_state);
-    $connector->validateConfigurationForm($form['connector_config'], $connector_form_state);

... it seemed like the desired error-behavior was to set an error on the form; so I moved the call that had the potential to fail and all the code that assumes it passes into the "try" and moved the contents of the "if" statement (i.e.: behavior on failure) into the "catch" statement.

For this change...

-    $connector = $this->getConnector();
-    if ($connector instanceof PluginFormInterface) {
+    try {
+      $connector = $this->getConnector();
       $connector_form_state = SubformState::createForSubform($form['connector_config'], $form, $form_state);
       $connector->submitConfigurationForm($form['connector_config'], $connector_form_state);
+
       // Overwrite the form values with type casted values.
       $form_state->setValue('connector_config', $connector->getConfiguration());
     }
+    catch (InvalidConnectorException) {
+      // No-op.
+    }

... it seemed like the desired error-behavior was to do nothing (because there was no "else" or anything afterwards before the end of the function), so I moved the call that had the potential to fail and all the code that assumes it passes into the "try" and left a no-op in the "catch".

mparker17 opened merge request !210

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

A line in ConnectorTypeSwitcherTest...

$this->assertInstanceOf(JSWebAssert::class, $session);

... causes an PHPStan lint in Drupal 11 and PHPStan 2.1...

Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'Drupal\FunctionalJavascriptTests\JSWebAssert' and Drupal\FunctionalJavascriptTests\WebDriverWebAssert will always evaluate to true.

... but not Drupal 10 and PHPStan 1.12.

But making the recommended change (i.e.: deleting that assertInstanceOf line) causes new PHPStan lints to appear in Drupal 10 and PHPStan 1.12 (but not Drupal 11 and PHPStan 2.1)...

Call to an undefined method Drupal\Tests\WebAssert::assertWaitOnAjaxRequest().

Ignoring the "will always evaluate to true" lint causes it to pass in Drupal 11 and PHPStan 2.1; but report that it didn't match an error in Drupal 10 and PHPStan 1.12; likewise ignoring the "Call to an undefined method" in D10/PHPStan1 will make it pass there, but report that it doesn't match in D11/PHPStan2.

Originally, I tried setting reportUnmatchedIgnoredErrors, but that felt wrong (we want to know when we've fixed something and don't have to ignore it anymore)

Asking in the Drupal Slack, @jonathan1055 came up with another solution: disabling the "phpstan (previous major)" job in .gitlab-ci.yml. I'm okay with that, because I expect...

  1. the newer version of PHPStan to give better / more relevant lints
  2. any phpstan suggestions to apply to all versions of the module
  3. the automated tests to catch any backwards-compatibility problems

... although at time-of-writing, it's not clear why the gitlab_template maintainers run a copy of phpstan for each variant (e.g.: why 2 phpstans, but only 1 phpcs, for example).

  • mparker17 committed 926ba162 on 9.0.x 
    feat: #3586504 PHPStan levels 4 and 5

By: mparker17
By: jonathan1055
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 9.0.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Needs review » Patch (to be ported)
Issue tags: -needs backport to 8.0.x

Merged to 9.0.x, merging to 8.0.x shortly.

  • mparker17 committed 2d583622 on 8.0.x 
    feat: #3586504 PHPStan levels 4 and 5

By: mparker17
By: jonathan1055
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Patch (to be ported) » Fixed

Merged to 8.0.x. I'll update this issue when the code in this issue has been released.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

The changes in this issue were released in elasticsearch_connector-9.0.0-alpha3, and elasticsearch_connector-8.0.0-alpha7

Status: Fixed » Closed (fixed)

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