Problem/Motivation

When there are no available cores in the subscription, or the core id is not in the list of possible search core ids, a warning is thrown for passing null value to the foreach at line 152 of PreferredCodeService ([warning] foreach() argument must be of type array|object, null given inPreferredCoreService.php:152). This is not a problem for the possible cores, as the event receives an empty array for the constructor, however the getAvailableCores() method can return NULL when there is no subscription data provided (possibly when it can't connect to Acquia Cloud?)

Steps to reproduce

Proposed resolution

Update line 149 of PreferredCoreService to $available_cores = $this->getAvailableCores() ?? []; to handle cases when there is a NULL returned from getAvailableCores(). (We could change the return value to an empty array, but it could be confusing as we wouldn't know if the client could connect to Acquia Cloud, or there are just no available cores.)

Remaining tasks

User interface changes

-

API changes

-

Data model changes

-

Issue fork acquia_search-3313861

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

balintpekker created an issue. See original summary.

balintpekker opened merge request !6

balintpekker’s picture

balintpekker
he/him
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: balintpekker » Unassigned
Status: Active » Needs review
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Needs work

So I noticed this in another area of code:

  public function getListOfAvailableCores() {
    $cores = $this->getAvailableCores() ?? [];

This is all because of:


  public function getAvailableCores() {
    if (!$cores = $this->acquiaSearchApiClient->getSearchIndexes()) {
      // Throw exception?
      return NULL;
    }


    // Able to connect, however no cores were found return empty instead.
    if (empty($cores)) {
      return [];
    }

    // We user core id as a key.
    return $cores;

I honestly don't think we even hit the empty check. So instead of returning null we fix the function to always return an array and kill any getAvailableCores() ?? []

balintpekker’s picture

balintpekker
he/him
Primary language Hungarian
Location Budapest, Hungary
commented

That is actually my proposed resolution in the parenthesis, I just didn't want to do it without any maintainer approval, because I wasn't sure how they want to handle it (just like the comment says, do they want to throw an Exception?). But I feel like we could return an empty array there too.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Let's do the empty array return and fix the method.

balintpekker’s picture

balintpekker
he/him
Primary language Hungarian
Location Budapest, Hungary
commented
Status: Needs work » Needs review
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Needs work

One more nit

balintpekker’s picture

balintpekker
he/him
Primary language Hungarian
Location Budapest, Hungary
commented
Status: Needs work » Needs review

  • mglaman authored c85fb75 on 3.1.x 
    Issue #3313861 by balintpekker: Add null protector to...
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Fixed

Thanks!

balintpekker opened merge request !10

balintpekker closed merge request !10

Status: Fixed » Closed (fixed)

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

japerry closed merge request !6