[PHP 7.2] count() parameter must be an array or an object that implements Countable

@mondrake

when I trace the errors in

https://www.drupal.org/pift-ci-job/828584

They all seem to lead back to

./vendor/behat/mink/src/Element/Element.php

    public function find($selector, $locator)
    {
        $items = $this->findAll($selector, $locator);

        return count($items) ? current($items) : null;
    }

which is reasonable.

find is a common base class method for lots of things that query the DOM tree while testing

but it is not part of Drupal .. the solution involves creating a behat issue.

re #2 - @martin107 that's being caused by an out-of-date dependency. Being tackled here: #2929477: Update jcalderonzumba/mink-phantomjs-driver.

I suggest we use this issue to fix our own code.

The countable issues are in Drupal\Tests\big_pipe\Unit\Render\BigPipeResponseAttachmentsProcessorTest and seems to be because the test set up is not quite right.

Here's a patch the fixes the test to set up the attachments with the correct data type.

Here is another one https://www.drupal.org/pift-ci-job/828674

1) Drupal\Tests\dynamic_entity_reference\FunctionalJavascript\DynamicEntityReferenceTest::testFieldFormatterViewModes
count(): Parameter must be an array or an object that implements Countable

/var/www/html/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:262
/var/www/html/vendor/behat/mink/src/Element/Element.php:148
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:188
/var/www/html/modules/contrib/dynamic_entity_reference/tests/src/FunctionalJavascript/DynamicEntityReferenceTest.php:227

I think we should fix \Drupal\Tests\WebAssert::optionNotExists. I don't think we can fix it upstream for behat/mink.

@jibran how come - which count do you think is causing the problems? I think this is exactly the same issue as #2929477: Update jcalderonzumba/mink-phantomjs-driver

Check /vendor/behat/mink/src/Element/Element.php:148

@jibran yes and that is using the return value from the web driver which in the case of javascript tests is jcalderonzumba/mink-phantomjs-driver and this is fixed by https://github.com/jcalderonzumba/MinkPhantomJSDriver/commit/9458e357b22...

Some more found by https://www.drupal.org/pift-ci-job/830503

Whoops findAll() doesn't have all those params.

Missed one...

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
@@ -345,7 +345,7 @@ public function testFieldAdminHandler() {
     // The term should now exist.
     $term = taxonomy_term_load_multiple_by_name($term_name, 'tags')[1];
-    $this->assertIdentical(1, count($term), 'Taxonomy term was auto created when set as field default.');
+    $this->assertNotNull($term, 'Taxonomy term was auto created when set as field default.');
   }

we have had problems in the past with assert null/not null on entity objects because if they get printed then it can basically bring down the test runner, also possibly when trying to run with verbose and showing it in the UI.

My recommendation would be to convert this to an entity query which allows us to actually do a count on the query result, which contains just the ID.

The test also hardcodes that the term ID is 1. Alternatively, we could do $terms and remove the [1], then the count would work.

@Berdir good point. Let's improve the test.

I think we should do the entity query suggestion from #16 too.

Sure here's a version with entity query.

apcu pool failure.

+++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
@@ -332,6 +332,11 @@ public function testFieldAdminHandler() {
+    $this->assertIdentical(0, count($result), "No taxonomy terms exist with the name '$term_name'.");

@@ -344,8 +349,11 @@ public function testFieldAdminHandler() {
+    $this->assertIdentical(1, count($result), 'Taxonomy term was auto created when set as field default.');

Since we are updating these assertions, is there any reason not to use assertCount() directly?

@amateescu because this is still a WebTestBase test - class EntityReferenceAdminTest extends WebTestBase {

@Berdir suggested using \Drupal::entityQuery('taxonomy_term') for less verbosity.

1. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
   @@ -332,6 +332,11 @@ public function testFieldAdminHandler() {
        $term_name = $this->randomString();
   +    $result = \Drupal::entityQuery('taxonomy_term')
   +      ->condition('name', $term_name)
   +      ->condition('vid', 'tags')
   +      ->execute();
   

   While it's in a test and we know there's no entity access going on, this could probably use an ->accessCheck(FALSE) just for clarity - especially since it's testing the absence of something that could still be there and hidden behind access.

2. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
   @@ -344,8 +349,11 @@ public function testFieldAdminHandler() {
   +    $result = \Drupal::entityQuery('taxonomy_term')
   +      ->condition('name', $term_name)
   +      ->condition('vid', 'tags')
   +      ->execute();
   

   Same here.

Addresses #24

Can't see anything else to complain about.

Committed 968da88 and pushed to 8.5.x. Thanks!

Given all the changes are to tests and completely compatible with 8.4.x I think this might be work backporting.

Cherry-picked as 61dc040 and pushed to 8.4.x.

For Drupal 7 this issue and other php 7.2 incompatibility are adressed there https://www.drupal.org/project/drupal/issues/2925449#comment-12474294