Problem/Motivation

With the introduction of the facet hierarchies in #3005040: Add functionality for getHierarchy() to get different plugin types the facet form got broken for facets that use a non-Search API facet source. This is because the submit assumes a hierarchy plugin is always there, but it only gets set for Search API facets source (for which it defaults to taxonomy for some reason, even for facets that have nothing to do with it. This I guess is something else..)

Steps to reproduce

Create a different facet source in code and a facet for it. Saving the form throws an exception that the hierarchy plugin ID is missing.

Proposed resolution

Ensure that the submit handler accounts for what exists in the actual form.

API changes

Data model changes

Comments

Upchuk created an issue. See original summary.

upchuk’s picture

Status: Needs work » Needs review
StatusFileSize
new8.64 KB
new10.48 KB

Here is a fix and a test that proves. I wrote a brand new test, maybe you can move it or advise for where it would be a better place. I am not seeing any test coverage for custom facet sources.

Hope it's green elsewhere.

The last submitted patch, 2: 3005040-2-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 2: 3005040-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

upchuk’s picture

Status: Needs work » Needs review
StatusFileSize
new10.57 KB

Ah lol, made a small mistake. Here we go.

mkalkbrenner’s picture

Thanks for the patch. But I wonder if this is the correct solution.
Neither the HierarchyProcessor nor the Hierarchy Plugin should be limited to Search API. So the problem might be the something else:

This is because the submit assumes a hierarchy plugin is always there, but it only gets set for Search API facets source

The question is, why isn't it set?

mkalkbrenner’s picture

function facets_update_8008() {
  $config_factory = \Drupal::configFactory();

  foreach ($config_factory->listAll('facets.facet.') as $facet_config_name) {
    $facet = $config_factory->getEditable($facet_config_name);
    $facet->set('hierarchy', ['type' => 'taxonomy', 'config' => []]);
    $facet->save(TRUE);
  }
}

Previously the hierarchy was hardcoded to be taxonomy. The update hook above should set the plugin instead for all existing facets. So the question is under which circumstances the exception is thrown?

upchuk’s picture

The question is, why isn't it set?

I wouldn't know, I didn't work on that. But the issue is on the form itself not necessarily the entity, as shown in the test and I describe above.

So if hierarchy is supposed to work with other facet sources as well, fine, but at the moment, the form doesn't and the submit handler of the form assumes that it does. So that needs to be fixed or otherwise facet that use non search api sources wont be saveable.

mkalkbrenner’s picture

StatusFileSize
new3.1 KB

The taxonomy plugin is currently limited to the Search API backend. But as we now allow 3rd party hierarchy plugins we must not limit this functionality to Search API anymore.

Here's a quick and dirty patch. It would be better to introduce a feature to let plugins declare their backend compatibilty. But there's no list of well-known backends yet.

upchuk’s picture

No issue that I see here. But I am not knowledgeable about the hierarchy stuff of facets.

mkalkbrenner’s picture

Status: Needs review » Reviewed & tested by the community

  • mkalkbrenner committed bceb483 on 2.0.x
    Issue #3279221 by Upchuk, mkalkbrenner: Facet form broken for non-Search...
mkalkbrenner’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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