Problem/Motivation

I found this bug while looking into the following php warning I was getting.
Invalid argument supplied for foreach() in Drupal\search_api_federated_solr\Form\SearchApiFederatedSolrSearchAppSettingsForm->buildForm() (line 325)

Changes

Update the getting of that config

src/Form/SearchApiFederatedSolrSearchAppSettingsForm.php:321 uses global $config to check for overrides.

This is a great idea to make sure we don't ignore the overrides but it may be better to use \Drupal::config('')->get().
This way, we still check for overrides but if there are none in settings.php, it loads whatever is in the saved search_api_federated_solr.search_app.settings.yml configuration.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonnyeom created an issue. See original summary.

jonnyeom’s picture

Status: Active » Needs review
FileSize
1.64 KB

This works great for me!

jonnyeom’s picture

Title: Invalid argument supplied warning > SearchApiFederatedSolrSearchAppSettingsForm » Settings Form does not load 'site_list' from saved config.
Issue summary: View changes
agentrickard’s picture

How did you first encounter this error? I'd like to be able to reproduce it.

agentrickard’s picture

Status: Needs review » Postponed (maintainer needs more info)
agentrickard’s picture

To do that, we'd have to save site_list in config as well, which I am hesitant to do. The problem here -- see the comment on line 43 -- is that $this->config in the form context is immutable and _not_ subject to overrides.

var_dump($this->config('search_api_federated_solr.search_app.settings')->get('facet.site_name.site_list'));

array()

var_dump($config['search_api_federated_solr.search_app.settings']['facet']['site_name']['site_list']);

array (size=2)
  0 => string 'Drupal 7' (length=8)
  1 => string 'Federated Search Demo' (length=21)

We have to have a consistent means of telling all sites in the network what the potential values are. The safest way to do that is to force loading in settings.php as a config override.

So I agree we should get rid of the foreach() error but disagree that the proposed patch solves the root issue.

agentrickard’s picture

Status: Postponed (maintainer needs more info) » Needs review

Apologies. I should have looked at the patch more closely. It's loading the mutable config.

agentrickard’s picture

With this patch, I also changed $configuration back to $config for consistency.

  • agentrickard committed 4def9f3 on 8.x-2.x authored by jonnyeom
    Issue #3096072 by jonnyeom: Settings Form does not load 'site_list' from...
agentrickard’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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