Problem/Motivation

There are a number of instances where we are passing a callable that doesn't return bool to functions that expect callables that return bool. These were detected by enabling the checkFunctionArgumentTypes phpstan parameter.

Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'strlen' given.

  • modules/field/src/Plugin/migrate/process/d6/FieldInstanceOptionTranslation.php
  • modules/field/src/Plugin/migrate/process/d6/FieldOptionTranslation.php
  • modules/field/src/Plugin/migrate/process/d6/FieldSettings.php
  • modules/user/src/Plugin/migrate/process/d6/ProfileFieldOptionTranslation.php
  • modules/views/src/Plugin/views/HandlerBase.php

Parameter #2 $callback of function array_filter expects (callable(mixed): bool)|null, 'strlen' given.

  • core/lib/Drupal/Core/Datetime/Element/Datelist.php
  • core/modules/sqlite/src/Driver/Database/sqlite/Connection.php

Parameter #2 $callback of function array_filter expects (callable(string): bool)|null, 'mb_strlen' given.

  • core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php
  • core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php

Parameter #1 $callback of function set_error_handler expects (callable(int, string, string, int): bool)|null, 'var_dump' given.

  • core/lib/Drupal/Core/Utility/Error.php

Steps to reproduce

Add to phpstan.neon.dist:

parameters:
  checkFunctionArgumentTypes: true

Run phpstan

Grep output for something like this:
grep -e "callable\(.*\): bool.*,.*' given\." output.txt

Proposed resolution

We can fix the array_filter calls inline:

  • Replace 'strlen' with fn (string $value): bool => strlen($value) > 0
  • Replace 'mb_strlen' with fn (string $value): bool => mb_strlen($value) > 0

Or create a helper function isEmptyString(string $string): bool somewhere, e.g. in Drupal\Component\Utility\Unicode and use it as the callback.

Remaining tasks

For set_error_handler we probably need a wrapping function around var_dump that returns FALSE. I suggest we do a follow up issue for that since it's quite different to the rest of this issue.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3471932

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

mstrelan created an issue. See original summary.

mstrelan opened merge request !9404

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Active » Needs review

I went with the helper function in Unicode, then realised we need the reverse of that so I implemented that too. Since we're only checking if the string is empty it doesn't matter if we were using strlen or mb_strlen before, they would either return 0 or something other than 0 and that's all that matters.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Hmm, one could choose to use the helper function. Unicode is a weird place though. And a String class cant be called String i think.

But personally since the logic in the 2 methods is so simple, why not use 

$array = array_filter($a, fn($v) => $v !== '');<code>.

I'm also thinking though, we assume all are string in those arrays. But might not be. Seems that there is a slight change in output should for some reason a boolean FALSE be passed.

https://3v4l.org/O7KfE
<?php
array_filter($a, fn($v) => $v !== '')
array(7) {
  [1]=>
  string(1) " "
  [2]=>
  string(1) "0"
  [3]=>
  int(0)
  [4]=>
  int(12)
  [5]=>
  float(15.5)
  [6]=>
  bool(false)
  [7]=>
  bool(true)
}

array_filter($a, 'strlen');
array(6) {
  [1]=>
  string(1) " "
  [2]=>
  string(1) "0"
  [3]=>
  int(0)
  [4]=>
  int(12)
  [5]=>
  float(15.5)
  [7]=>
  bool(true)
}

?>

So I think we need to be carefull here. Not sure whats the smart way out, perhaps the other solution might be safer with <code>strlen($v) > 0
mstrelan’s picture

mstrelan commented

Thanks for looking at this @bbrala. I initially went down the simple arrow function route:

fn ($value) => strlen($value) > 0

But strlen is coercing values to a string. If we later add declare(strict_types=1) then we get a TypeError. So we'd need to cast to match the current logic:

fn ($value) => strlen((string) $value) > 0

But later we might decide arrow functions should be typed:

fn (mixed $value): bool => strlen((string) $value) > 0

We might also later decide to enable SlevomatCodingStandard.Functions.StaticClosure:

static fn (mixed $value): bool => strlen((string) $value) > 0

It's starting to get rather convoluted, and we need to reuse this in a number of different places. This is why I went with the helper function.

I guess we do need to allow mixed and do the casting, and find a better spot for it. Perhaps the helper function could do the array_filter part as well.

bbrala’s picture

bbrala
Location Netherlands
commented

Hmmz yeah that will get out of hand. The better question might be then if we should just do your current setup and technically be more strict (and correct) on the output.

We do need a new place for that though. Unicode feels wrong.

I did check, didn't find a better place. So guessing an ArrayFilter helper makes sense.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Nice timing, I was just finishing up the tests. What do you think of these changes?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Re-ran unit tests twice but they fail each time so seems like could be related.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

The array to string conversion is bogus, it doesn't work with the strlen we had before so it doesn't need to work now.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

Feedback and conceirns addressed. All good.

quietone’s picture

quietone commented

I read the IS, comments and the MR. thanks for the easy to understand issue summary. All questions answered. Just needs another committer to look at this.

Leaving at RTBC

catch made their first commit to this issue’s fork.

  • catch committed 3a3c8164 on 11.x 
    Issue #3471932 by mstrelan, bbrala, quietone: Fix callables that are...

  • catch committed 6884e9bc on 10.4.x 
    Issue #3471932 by mstrelan, bbrala, quietone: Fix callables that are...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.4.x, thanks!

catch closed merge request !9404

Status: Fixed » Closed (fixed)

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