Problem/Motivation

The Claro theme throws TypeError with a custom table block:
TypeError: in_array(): Argument #2 ($haystack) must be of type array, string given in in_array() (line 196 of /var/www/core/themes/claro/src/ClaroPreRender.php).

Steps to reproduce

  1. Enable Claro Theme as a default theme.
  2. Create a custom Block in a custom module to render a table with a class attribute as a string:
    class CustomBlock extends BlockBase
{
  /**
   * {@inheritdoc}
   */
  public function build() {
    return [
      '#type' => 'table',
      '#attributes' => [
        'class' => 'classname'
      ]
    ];
  }
}
  3. Place the block on the page.

Proposed resolution

Add a check to the tablePositionSticky method in the Claro theme to avoid calling the array function on a non-array variable.

Remaining tasks

  1. Review MR
  2. Commit
CommentFileSizeAuthor
#8 3419621-TEST-ONLY-FAIL.patch1.55 KBdburiak
#7 3419621-TEST-ONLY-FAIL.patch1.57 KBdburiak

Issue fork drupal-3419621

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ryanbuckley@gmail.com created an issue. See original summary.

ryanbuckley@gmail.com’s picture

ryanbuckley@gmail.com CreditAttribution: ryanbuckley@gmail.com commented
Issue summary: View changes

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

sijumpk opened merge request !6482

sijumpk’s picture

sijumpk CreditAttribution: sijumpk at QED42 commented
Status: Active » Needs review

'class' attribute should be an array. Seems like in case of Geofield Map, its passing a string instead of an array. Because of that this error is happening. Noticed that inside core code, there are some places where double checking for array elements are happening. Eg inside ClaroPreRender::verticalTabs

$groups_are_present = isset($element['group']['#groups']) && is_array($element['group']['#groups']);

So adding the same type of checking for $element['#attributes']['class'] also.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Seems like a bug on GeoMap module.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
FileSize
3419621-TEST-ONLY-FAIL.patch1.57 KB

Here is the test-only patch showing the issue.
Without patching, the test will fail on Drupal 10.2.x

In the CI I don't see an option to run a test with 10.2.x version. Only 10.1.x available.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
FileSize
3419621-TEST-ONLY-FAIL.patch1.55 KB

Updated test-only patch with fixed coding standards.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Status: Needs work » Needs review
Issue tags: -Needs tests
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Left a small comment but can you update MR for 11.x please

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

anpolimus’s picture

anpolimus
Primary language Ukrainian
Location Kyiv
CreditAttribution: anpolimus as a volunteer and at Drupal Ukraine Community, ImageX commented

@smustgrave, do you mean creating a new MR against 11.x or updating this one from 10.x to 11.x?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

Updating existing one to 11.x would be best.

dburiak opened merge request !6560

dburiak closed merge request !6560

Version: 10.2.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dburiak changed the visibility of the branch 3419621-typeerror-inarray-argument-d11 to active.

dburiak changed the visibility of the branch 3419621-typeerror-inarray-argument-d11 to hidden.

dburiak opened merge request !6703

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented

Added MR to 11.x: https://git.drupalcode.org/issue/drupal-3419621/-/tree/3419621-typeerror...
Please review.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Issue summary appears to be missing proposed solution section.

Also wonder if there's been a backtrace to see what's calling this with a string. In case we covering up a larger issue.

lpeabody’s picture

lpeabody CreditAttribution: lpeabody as a volunteer and at Digital Polygon commented

As an FYI this error is resolved in the case of the Geofield Map module in version 3.0.15 https://git.drupalcode.org/project/geofield_map/-/compare/3.0.14...3.0.1...

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Issue summary: View changes
dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented

@smustgrave The string in the original issue was calling from Geofield Map module (already fixed). The summary is updated with a proposed solution section.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

But what are the steps to reproduce in core without the geofield module?

There are 2 MRs up, can one please be hidden.

dburiak changed the visibility of the branch 3419621-typeerror-inarray-argument to hidden.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Issue summary: View changes
dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented

@smustgrave the summary is updated. Please review.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Status: Needs work » Needs review
pameeela’s picture

pameeela CreditAttribution: pameeela at Technocrat commented
Title: TypeError: in_array(): Argument #2 ($haystack) must be of type array, string given in in_array() » tablePositionSticky should not be called on a non-array variable
Issue summary: View changes
Issue tags: -Needs issue summary update +Bug Smash Initiative

I think the IS is OK now.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Thanks @pameeela for updating that.

Test only feature shows the issue

1) Drupal\KernelTests\Core\Theme\ClaroTableTest::testThemeTablePositionStickyPreRender
PHP Error was thrown.
/builds/issue/drupal-3419621/core/tests/Drupal/KernelTests/Core/Theme/ClaroTableTest.php:74
/builds/issue/drupal-3419621/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 2, Assertions: 3, Failures: 1.

Additional check seems to solve the issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

I left some small nits on the MR to address. Once they are this can go back to rtbc.

dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Status: Needs work » Needs review
dburiak’s picture

dburiak CreditAttribution: dburiak at ImageX commented
Status: Needs review » Reviewed & tested by the community
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Reviewed & tested by the community » Needs work

Still need to remove the try/catch. Tests are fast now, it's not a problem if it stops everything else after.

pameeela’s picture

pameeela CreditAttribution: pameeela at Technocrat commented
Status: Needs work » Needs review

Accepted the suggestion to remove the try/catch, the tests are failing but I can't see that it's related to this?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

MR just needed a rebase, all green now!

  • nod_ committed 7e28ed51 on 11.x 
    Issue #3419621 by dburiak, sijumpk, smustgrave, pameeela, alexpott:...

  • nod_ committed 9f50569b on 10.3.x 
    Issue #3419621 by dburiak, sijumpk, smustgrave, pameeela, alexpott:...

  • nod_ committed 4dd544e7 on 10.2.x 
    Issue #3419621 by dburiak, sijumpk, smustgrave, pameeela, alexpott:...

nod_ closed merge request !6703

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech for OPTASY, Palantir.net commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 7e28ed5 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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