Render using theme input and select instead of lists with links for checkboxes and dropdown

Happy to hear we will be able to use form elements!

There is however a potential web accessibility issue in play, please note WCAG 2.0 guideline 3.2.2.

3.2.2 On Input: Changing the setting of any user interface component does not automatically cause a change of context unless the user has been advised of the behavior before using the component. (Level A)

W3C WCAG 3.2.2

To be compliant we would need a submit button before triggering a page refresh or ajax-based context change.

For full disclosure: updating filter options or result counters within the component is OK.

Yes, this is something that should be fixed up. Can't we just have good old HTML output like https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox

I think there are issues with keyboard accessibility with this too. Not sure where focus is.

Wow, +1 on this.

Reroll against 8.x-1.x.

#7 / #8 Although this patch improves the HTML structure of the facets, it still redirects through JavaScript instead of a form submit. Wrapping the elements in a form requires more refactoring.

Fixed a few test failures. The unit tests and some FunctionalJavascript tests still need work.

This change is really good for a11y and it has been requested a lot of times. It is however a big change and I wonder if that means this should only get into a 8.2.x-branch of facets.
Especially because this might mean that other contributed/custom modules and themes don't work anymore with how this currently works.

I'd say if the final markup is the same, we only move some theming from client to server.
In other words, i don't see intermediate markup as part of a BC promise.
Of course we need a change record. (And we will surely do some testing...) YMMV though.

Amazing work Jefuri, just what I was looking for.

I was using this latest patch from #12, and noticed that the label of the renderend checkbox wasn't receiving the proper for attribute. which caused the checkbox to not be linked to it's label.

Instead of defining the checkbox type as follows in CheckboxWidget.php

$item = [
  '#type' => 'checkbox',
  '#title' => $this->renderer->render($item),
  '#checked' => $result->isActive() ? 1 : 0,
  '#return_value' => $result->getUrl()->toString(),
  '#label_attributes' => ['for' => $id],
  '#attributes' => [
    'id' => $id,
  ],
];

I am now adding the id like so:

$item = [
  '#type' => 'checkbox',
  '#title' => $this->renderer->render($item),
  '#checked' => $result->isActive() ? 1 : 0,
  '#return_value' => $result->getUrl()->toString(),
  '#id' => $id,
];

This is the only change I did to patch #12.

As feedback on #14 Please note that because we are rendering with templates, the output can be different, depending on theme overrides, so I would be in favor of adding it to the 8.2.x-branch as proposed in #14.

Found another bug (Sorry for the babysteps in comparison to the main patch):
When you choose a select, the option of showing the facet's title, wasn't working, so instead of:

return [
  '#type' => 'select',
  '#facet' => $facet,
  '#options' => array_merge($emptyOption, $options),
  '#value' => $defaultValue,
  '#attributes' => [
    'data-drupal-facet-id' => $facet->id(),
    'data-drupal-facet-alias' => $facet->getUrlAlias(),
    'class' => ['js-facets-dropdown-links'],
  ],
  '#context' => ['list_style' => $widget['type']],
  '#attached' => [
    'library' => [
      'facets/drupal.facets.dropdown-widget',
    ],
  ],
  '#cache' => [
    'contexts' => [
      'url.path',
      'url.query_args',
    ],
  ],
];

I am now doing this:

$item = [
  '#type' => 'select',
  '#facet' => $facet,
  '#options' => array_merge($emptyOption, $options),
  '#value' => $defaultValue,
  '#attributes' => [
    'data-drupal-facet-id' => $facet->id(),
    'data-drupal-facet-alias' => $facet->getUrlAlias(),
    'class' => ['js-facets-dropdown-links'],
  ],
  '#context' => ['list_style' => $widget['type']],
  '#attached' => [
    'library' => [
      'facets/drupal.facets.dropdown-widget',
    ],
  ],
  '#cache' => [
    'contexts' => [
      'url.path',
      'url.query_args',
    ],
  ],
];

if($facet->get('show_title')) {
  $item['#title'] = $facet->getName();
}

return $item;

Needs work, patch didn't apply.

My apologies, it has been a long time since I have created patches.

Redid the patch from #16!

Feel free to keep me out of contributions, as mine are tiny bits in comparison. :)

I attached an interdiff this time too!

#18 works applies cleanly and works well. Thanks! It's much easier to theme now.

I recently ran into the issue where the facets were displaying as links instead of checkboxes, even though i specified checkboxes in the facet configuration.

I decided to try this patch, and they are now checkboxes. But, when i click the checkbox or the label next to the checkbox, nothing happens. No javascript errors either.

My theme is a subtheme of stable, if that helps.

Re-rolling patch for 1.x-dev. [unstable]

Re-rolling patch for 1.x-dev, was missing the new checkbox twig template.

Not sure if I've done something wrong, but the patch doesn't seem to work properly for ajax views, it does a full page refresh instead of just reloading the view.

Not sure if I've done something wrong, but the patch doesn't seem to work properly for ajax views, it does a full page refresh instead of just reloading the view.

I believe that's an issue with facets + ajax in general.

https://www.drupal.org/project/facets/issues/3075378

I am going to do an iteration on this. I will start with a code review and manual test.

I'm going to do another attempt at moving this forward. Last time I was blocked by the test failures that were affecting the main development branch, but these have in the meantime been fixed in #3146507: Tests are failing on PHP 7.3.

Rerolled against latest 8.x-1.x-dev.

I have fixed some small errors, like an empty array element access, a case where the empty option for dropdowns was getting the wrong key when there is an active result, and I started to attack some of the test failures.

I'm not very confident making chances at the moment. I don't have a good view on how this reworking affects the rendering of the elements. I think it would be a good idea to extend the current functional test coverage to more completely assert the rendered HTML of the widgets after it has been transformed using JS, and then we can use these tests here as a baseline to work against - that would make it easier to ensure that we are ending up with exactly the same HTML. We can do this by extending WidgetJSTest::testCheckboxWidget() and WidgetJSTest::testDropdownWidget().

Also I think it would perhaps be more interesting to handle the conversion of the checkboxes and dropdowns in separate issues. There is practically no overlap between the code of both widgets, neither in the JS code nor in the backend code, so having separate issues would reduce the scope and make it easier to move this forward and test it manually.

I'm not very confident making chances at the moment. I don't have a good view on how this reworking affects the rendering of the elements.

When we first did the initial port of facet_api for drupal 7 to facets for drupal 8, we actually tried doing this with rendered items and a POST form. This ended up being really complicated for a couple of reasons, including cacheability. Nick_vh, Jur, StryKaizer and me have discussed this multiple times, we even got the help from Wim Leers to discuss this caching problem.
I probably have the most intimate knowledge of the codebase and I'm not confident either, so I completely understand your hesitance here.

I think it would be a good idea to extend the current functional test coverage to more completely assert the rendered HTML of the widgets after it has been transformed using JS, and then we can use these tests here as a baseline to work against - that would make it easier to ensure that we are ending up with exactly the same HTML. We can do this by extending WidgetJSTest::testCheckboxWidget() and WidgetJSTest::testDropdownWidget().

Great idea. Extended coverage here would also make it a lot easier / give us more confidence for the ajax facets.

Also I think it would perhaps be more interesting to handle the conversion of the checkboxes and dropdowns in separate issues. There is practically no overlap between the code of both widgets, neither in the JS code nor in the backend code, so having separate issues would reduce the scope and make it easier to move this forward and test it manually.

It would reduce scope, I agree. But they would need to be worked on/committed around the same time, to ensure that we do not get a release out where the behavior is different between checkboxes/dropdown.

I haven't looked at the code yet, so no feedback there - yet. @pfrenssen if you want to, it might be easier to walk/talk trough this together - you can find me on drupal slack.

@borisson_ Great idea! I will find you on Slack, probably tomorrow or on Thursday since I am dividing my time between several tickets ATM.

I agree with your argument of keeping the refactoring of both widgets in a single issue.

Created an issue to extend the current test coverage, so we can work on this and be confident that we will not cause unexpected regressions in existing site's theming: #3166983: Extend test coverage for the dropdown and checkbox widgets.

#3166983: Extend test coverage for the dropdown and checkbox widgets is in so we can now check if this regresses the HTML.

Made some progress to restore the HTML to be as close as possible to how it was before. One thing that has now changed which is acceptable in my view is that in the checkboxes widget the hidden links are no longer rendered. These links were just used to store the data regarding the facet keywords. Now they are placed as data attributes on the checkboxes themselves. I think this doesn't count as a regression since these links are hidden from view using JS and unlikely to be made visible again by existing projects.

One thing I am now unsure about is the wrapper around the dropdown widget. Originally this wrapper was as follows:

<div class="facets-widget-dropdown">

While now it is like this:

<div class="js-form-item form-item js-form-type-select form-item-llama js-form-item-llama form-no-label facets-widget-dropdown">

I don't know whether these additional classes will cause regressions in existing projects, but I suspect that it will cause _some_ visual regressions. We might need to make our own template for this.

I am unassigning since the time that I got to work on this has run out.

I don't know whether these additional classes will cause regressions in existing projects, but I suspect that it will cause _some_ visual regressions. We might need to make our own template for this.

I agree, this will cause visual regressions. But it should also provide a better default styling for new projects. I'm not sure if the extra classes are a BC-break. At the very least we should add a change notice.

OK let's in scope of this make sure we don't output any unexpected additional classes, to avoid visual regressions.

@borisson_ is it acceptable for you to remove the invisible link that is currently included alongside checkboxes? I think the risk is very low, since this is made invisible using our JS code, so in order for this to regress it would mean that a project would override our JS to make the link visible again - I think this is a very far fetched scenario.

@borisson_ is it acceptable for you to remove the invisible link that is currently included alongside checkboxes? I think the risk is very low, since this is made invisible using our JS code, so in order for this to regress it would mean that a project would override our JS to make the link visible again - I think this is a very far fetched scenario.

If people need the links to be visibile, they can use/override the links widget. This does not feel like BC-break to me. +1

Going to move this forward. Started by creating an issue fork and rebased on the latest 8.x-1.x. The changes made to js/dropdown-widget.js in #3058431: Hierarchical Facets using Dropdown Widget have been lost. We will need to investigate how to restore the hierarchical indicator that was added in that issue.

I'm still learning how to work with merge requests. I don't seem to get test results, probably this works exactly like with the patch based workflow and I need to set the issue status to Needs Review.

The dropdown was not working as expected, the default option could not be used to reset the search filter. This has been fixed now. Also did some cleanups.

The dropdown widget is now getting near completion. I've restored the HTML output to be as close as possible to the original.

There is one deviation I have allowed: in the original implementation a hidden label was always output, marked with "aria-labelledby". While this was a good thing to have, it was also conflicting with the show_title option which outputs the "real" label, and it is conflicting with how core renders select dropdowns by default.

I have chosen to stay with the standard theming of dropdowns as is handled by core. This means that we will preserve any accessible labels that are defined by the theme.

Fixed some test failures. I'm now out of the time that I was granted to work on this so I will unassign. The next steps to take would be to take a very close look at the checkbox widget, and bring it as close as possible to the current outputted HTML and functionality. This should get most of the remaining tests green.

Taking this back for another round.

The checkboxes are now also complete. There is a difference with the original rendering, but in this case I think it is justified.

Current version of Facets (without this patch)

The HTML that is output by the JS code in the latest dev release of Facets looks like this:

<li class="facet-item">
  <input type="checkbox" class="facets-checkbox" id="content-type-page">
  <label for="content-type-page">
    <span class="facet-item__value">page</span>
  </label>
  <a href="/search-api-test-fulltext?f%5B0%5D=content_type%3Apage" rel="nofollow" data-drupal-facet-item-id="content-type-page" data-drupal-facet-item-value="page" data-drupal-facet-item-count="1" style="display: none;">
    <span class="facet-item__value">page</span>
  </a>
</li>

And it looks like this in the default theming of Drupal. This screenshot is taken using the Bartik theme:

With this patch

In this patch the checkbox facets are rendered using the standard theming functions of Drupal. This causes 1 div to be inserted in between the list item and the facet. Also the "hidden link" is removed as discussed and approved earlier (ref comment #40):

<li class="facet-item">
  <div class="facet-item js-form-item form-item js-form-type-checkbox form-type-checkbox js-form-item-content-type-page form-item-content-type-page">
    <input class="facets-checkbox form-checkbox" data-drupal-facet-item-id="content-type-page" data-drupal-facet-item-value="page" data-drupal-facet-item-count="1" type="checkbox" id="content_type-page" name="content_type-page" value="/search-api-test-fulltext?f%5B0%5D=content_type%3Apage">
    <label for="content_type-page" class="option">
      <span class="facet-item__value">page</span>
    </label>
  </div>
</li>

This looks like this in the Bartik theme:

Currently the checkbox facets looks terrible, _because_ the JS code is not constructing a standard checkbox and is lacking the div which have the classes that make a checkbox look "normal". These classes will be present in practically all Drupal themes.

On the other hand, because it looks so terrible, we can be certain that in the vast majority of current implementations this will have had custom styling applied to make it look acceptable. This change will impact a number of existing users. But it cannot be predicted in how far this will "break" the design. I assume probably themers will mimic what core is doing in most cases?

I think probably any negative impact in theming will be easy to fix: the themer will just have to remove their original "fix" and they will fall back to the core theming. Also the impact is mitigated by the fact that the existing classes that need to be targeted to fix the layout are all left intact (like .facet-item and .facets-checkbox). But of course there might be changes in font size and shifting elements due to the cascading nature of CSS.

BTW it's also not super trivial to get rid of this div because this is inserted at form item level by form-element.html.twig.

In my personal view I would propose to keep the div and create a change record. It feels wrong to keep this broken checkbox markup now that we are introducing this ticket (it is probably something we want to fix at some point anyway). And inserting a div is hardly a reason to move to a new major version :)

Unassigning, this is ready again for review.

I tried to run a test on Drupal 9, but this failed because I specified PHP 7.2 which is not supported in Drupal 9.

I tested this in Vanilla Drupal 8.9.x and works as expected. I tried to do multiple tests to check if it breaks anywhere. However, since I am not a maintainer of the facets project, it should probably be checked as an idea and also for possible nitpicks in the code.
Setting this to RTBC on my side.

The patch will render html special chars e.g. "&" as & which is not ideal.
We could wrap the return of (string) $option_text in htmlspecialchars_decode or decide not to use FormatableMarkup.

#52 occurs when the dropdown includes the count for the option label. This can be solved by removing the (string) typecast, so the output is not double escaped.

I have appended the merge request with the following changes:

1. Remove early rendering of facet labels to prevent double html escaping and loss of cache metadata
2. Replaced calls to window.location.href with the existing facets_filter event so the new widget rendering is compatible with facet sources that have AJAX enabled
3. Restored the js-facets-widget html class for the new widgets, so it is compatible with facet sources that have AJAX enabled

I think this needs a re-roll.

I think the approach from MR will break a lot of sites. I would suggest to create new widgets, e.g. SelectWidget, and deprecate the old ones. Then, in 2.x, we can remove them.

Attached a static copy of the current status of the MR as a patch.

You may want to consider https://www.drupal.org/project/facets_form, which simply renders the facets in a form. I know, there are some concerns related to caching. However, till now we didn't notice cache issues compared to the native JS rendered facets. But as the module is in a pre-alpha state, we'll dig deeper into the caching issue. We would be grateful if you can give it a try, maybe is what you need.

@claudiu.cristea this patch is also solving it to be rendered as a normal form element. The chances of visual regressions are low since we have taken care to keep the HTML as close as possible to the current HTML. We have even added additional test coverage that ensures that the resulting HTML stays exactly the same, except in one case where we will now align with the core HTML. See issue #3166983: Extend test coverage for the dropdown and checkbox widgets for the HTML test coverage, and comment #48 which outlines the change that is done and the potential impact.

Bumping priority since this bug is severe enough that there is a contrib module made specifically to fix it: https://www.drupal.org/project/facets_form

Let's see how this fares against 2.x.

Needs reroll for 2.0.x.

Reroll for 2.x.

The patch in #62 contains another patch file. Let's continue with the merge request as the leading source for code changes.

The merge request contains the latest changes in 2.0.x.

Updated patches from PR should they be useful to anyone else.

Reroll #66 against latest dev version.

I'm getting an error with the latest patch, and I haven't have time to dig on this, but my Drupal core and contributed modules are all up to date:

User error: Twig\Error\LoaderError: Template "modules/custom/facets/templates/facets-result-item--checkbox.html.twig" is not defined. in /vendor/twig/twig/src/Loader/ChainLoader.php:98 Stack trace: #0 /vendor/twig/twig/src/Environment.php(299): Twig\Loader\ChainLoader->getCacheKey('modules/custom/...') #1 /web/core/lib/Drupal/Core/Template/TwigEnvironment.php(205): Twig\Environment->getTemplateClass('modules/custom/...', NULL) #2 /vendor/twig/twig/src/Environment.php(381): Drupal\Core\Template\TwigEnvironment->getTemplateClass('modules/custom/...') #3 /web/core/themes/engines/twig/twig.engine(55): Twig\Environment->loadTemplate('modules/custom/...') #4 /web/core/lib/Drupal/Core/Theme/ThemeManager.php(384): twig_render_template('modules/custom/...', Array) #5 /web/core/lib/Drupal/Core/Render/Renderer.php(422): Drupal\Core\Theme\ThemeManager->render('facets_result_i...', Array) #6 /web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, false) #7 /web/modules/custom/facets/src/Plugin/facets/widget/CheckboxWidget.php(84): Drupal\Core\Render\Renderer->render(Array) #8 /web/modules/custom/facets/src/Widget/WidgetPluginBase.php(169): Drupal\facets\Plugin\facets\widget\CheckboxWidget->prepareLink(Object(Drupal\facets\Result\Result)) #9 /web/modules/custom/facets/src/Widget/WidgetPluginBase.php(68): Drupal\facets\Widget\WidgetPluginBase->buildListItems(Object(Drupal\facets\Entity\Facet), Object(Drupal\facets\Result\Result)) #10 [internal function]: Drupal\facets\Widget\WidgetPluginBase->Drupal\facets\Widget\{closure}(Object(Drupal\facets\Result\Result)) #11 /web/modules/custom/facets/src/Widget/WidgetPluginBase.php(70): array_map(Object(Closure), Array) #12 /web/modules/custom/facets/src/Plugin/facets/widget/LinksWidget.php(41): Drupal\facets\Widget\WidgetPluginBase->build(Object(Drupal\facets\Entity\Facet)) #13 /web/modules/custom/facets/src/FacetManager/DefaultFacetManager.php(406): Drupal\facets\Plugin\facets\widget\LinksWidget->build(Object(Drupal\facets\Entity\Facet)) #14 /web/modules/custom/facets/src/Plugin/Block/FacetBlock.php(91): Drupal\facets\FacetManager\DefaultFacetManager->build(Object(Drupal\facets\Entity\Facet)) #15 /web/core/modules/block/src/BlockViewBuilder.php(171): Drupal\facets\Plugin\Block\FacetBlock->build() #16 [internal function]: Drupal\block\BlockViewBuilder::preRender(Array) #17 /web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(101): call_user_func_array(Array, Array) #18 /web/core/lib/Drupal/Core/Render/Renderer.php(772): Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_ren...', 'exception', 'Drupal\\Core\\Ren...') #19 /web/core/lib/Drupal/Core/Render/Renderer.php(363): Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array) #20 /web/core/lib/Drupal/Core/Render/Renderer.php(201): Drupal\Core\Render\Renderer->doRender(Array, true) #21 /web/core/lib/Drupal/Core/Render/Renderer.php(157): Drupal\Core\Render\Renderer->render(Array, true) #22 /web/core/lib/Drupal/Core/Render/Renderer.php(564): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() #23 /web/core/lib/Drupal/Core/Render/Renderer.php(158): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #24 /web/core/lib/Drupal/Core/Render/Renderer.php(172): Drupal\Core\Render\Renderer->renderPlain(Array) #25 /web/core/modules/big_pipe/src/Render/BigPipe.php(693): Drupal\Core\Render\Renderer->renderPlaceholder('callback=Drupal...', Array) #26 /web/core/modules/big_pipe/src/Render/BigPipe.php(547): Drupal\big_pipe\Render\BigPipe->renderPlaceholder('callback=Drupal...', Array) #27 /web/core/modules/big_pipe/src/Render/BigPipe.php(305): Drupal\big_pipe\Render\BigPipe->sendPlaceholders(Array, Array, Object(Drupal\Core\Asset\AttachedAssets)) #28 /web/core/modules/big_pipe/src/Render/BigPipeResponse.php(112): Drupal\big_pipe\Render\BigPipe->sendContent(Object(Drupal\big_pipe\Render\BigPipeResponse)) #29 /vendor/symfony/http-foundation/Response.php(381): Drupal\big_pipe\Render\BigPipeResponse->sendContent() #30 /web/index.php(20): Symfony\Component\HttpFoundation\Response->send() #31 {main} in Drupal\big_pipe\Render\BigPipe->sendPlaceholders() (line 554 of /web/core/modules/big_pipe/src/Render/BigPipe.php)
#0 /web/core/includes/bootstrap.inc(347): _drupal_error_handler_real(256, 'Twig\\Error\\Load...', '/home/webdrips/...', 554)
#1 [internal function]: _drupal_error_handler(256, 'Twig\\Error\\Load...', '/home/webdrips/...', 554, Array)
#2 /web/core/modules/big_pipe/src/Render/BigPipe.php(554): trigger_error('Twig\\Error\\Load...', 256)
#3 /web/core/modules/big_pipe/src/Render/BigPipe.php(305): Drupal\big_pipe\Render\BigPipe->sendPlaceholders(Array, Array, Object(Drupal\Core\Asset\AttachedAssets))
#4 /web/core/modules/big_pipe/src/Render/BigPipeResponse.php(112): Drupal\big_pipe\Render\BigPipe->sendContent(Object(Drupal\big_pipe\Render\BigPipeResponse))
#5 /vendor/symfony/http-foundation/Response.php(381): Drupal\big_pipe\Render\BigPipeResponse->sendContent()
#6 /web/index.php(20): Symfony\Component\HttpFoundation\Response->send()
#7 {main}

You are missing templates/facets-result-item--checkbox.html.twig changes
Here is a new patch that solves the issue

Please ignore my previous patch, here is the right one

Rerolled the patch to support D10 as well as ensure that the tests pass.

Upload latest patch with all the tests passing. Patch #75 had a bug where it was making use of the jQuery :checkbox pseudo-selector.

This is great! I realised that javascript was being used to render this facet style ... which was no good for us on this project which is making heavy use of tailwind and associated templates.

The patch applies cleanly and appears to do what it say on the tin, so far so good.

Edit: Spoke too soon. This appears to work for non hierarchical checkboxes but one particular facet set to hierarchical now no longer renders out the hierarchy and is only doing the top level bits.. will investigate.

Edit #2: This issue I described above is because facets is setting the children property and input theme elements expect #children by default, @see template_preprocess_input() in order for $variables['children'] to be populated. So i've got this working by adding some additional local in my theme's already existing <theme>_preprocess_input() implementation to poke the children into the $variables array :)

So what I have described here is more of a byproduct of having the Drupal theme system take over rendering as input.

I guess the question is, Should we be modifying other Facets code to change children to #children to work better 'out of the box'? in Drupal\facets\Widget\WidgetPluginBase::buildListItems() is where it sets the children.

Maybe Drupal\facets\Plugin\facets\widget\CheckboxWidget should now override buildListItems() in it's class and change to set #children instead of children?

Using patch #76 to have the select element instead of generating it through javascript. Works fantastic and is so much cleaner!
It is also compatible with the Chosen module (tested with 4.x), out of the box.

One remark though: it is not compatible with selecting multiple values, so in the facet config Ensure that only one result can be displayed should be set to TRUE to avoid unexpected results. Not sure how to fix this, since the page already reloads when selecting the first item, so you have no time to select the second item. (unless you do this after the page reload) Maybe we should only reload if the select element becomes unfocused AND changed AND isMultiple?

The patch failed to apply.

The patch applies for me ok on Facets 2.0.7. This patch is very useful as I have several sites where bots are aggressively indexing all search facet links. I hope that changing the facets to genuine form checkboxes will reduce the bot impacts.

fwiw, this change has stopped the bots smashing our facet links. It would be great if this patch could be committed.

Id like to echo #81. This patch really helps with bots hitting our site as well. Can this be merged?

Patch #76 doesn't apply anymore due to conflicting tests in v2.0.8. Attaching a patch without changed tests.
Tests should be looked into, marked the issue as "Needs work".

Patch applies to Facets 2.0.9, thanks @matthiasm11

Patch #76 doesn't apply anymore due to conflicting tests in v2.0.8. Attaching a patch without changed tests.
Tests should be looked into, marked the issue as "Needs work".

The latest commits to MR 146 resolve the conflicts in the tests; additionally, commit https://git.drupalcode.org/project/facets/-/merge_requests/146/diffs?com... addresses an oversight where the new checkboxes rendering did not properly retain the "Reset all" configuration option.

Setting back to "Needs review"...

After resolving merge conflicts in the tests, there remain some failing tests that look to be valid divergences from the intended design. Specifically, it looks like the original Links facet plugin may behave differently than it used to, per this code change (though at the same time, that shouldn't be the case because this only modifies the Checkbox and Dropdown plugins, which extend that Links plugin...)

In any case, more investigation is needed. Setting back to Needs Work.

Noting that the observation in #77, namely, that the staged code does not yet support Facets' taxonomy hierarchy display, qualifies as another element that "Needs Work" before this is ready for prime-time.

I've reverted the change made in e49931def619c244d72fe6f12eb9bacd5311bf14 that was subsequently bugged in the rebase and causing a test failur - that test change is unrelated to this MR and the existing test from the 2.0.x branch should still pass as there is no expected behaviour change here.

There is another unrelated test change there that should be reverted - missed it before I pushed my change.

This needs to be reworked for v3.

Am working on porting it to v3.

A patch for v3.0.x - I tried to create a merge request for it but gitlab wouldn't let me pick 3.0.x as a destination branch, even after I updated the issue fork from the main repo.

Fixed some copy pasta on CheckboxWidget.php.

This breaks the taxonomy hierarchy display.

A structure like this:

- Term 1 (weight 0)
  - Term 1.1 (weight 0)
    - Term 1.1.1 (weight 0)
  - Term 1.2 (weight 1)
- Term 2 (weight 1)
- Term 3 (weight 2)
  - Term 3.1 (weight 0)

This turns into the following:

- Term 1 (weight 0)
- Term 1.1 (weight 0)
- Term 1.1.1 (weight 0)
- Term 3.1 (weight 0)
- Term 2 (weight 1)
- Term 1.2 (weight 1)
- Term 3 (weight 2)

(or worse)

Hey, after doing everything I could from installing firewalls, banning search engine bots or manually renaming urls I found out in a slack group that facets are causing my page to be constantly scanned by bots....

I get on a university page (not a lot of traffic) each second several request to my search page with access to the facets url. I would love to use facets blocks but the current situation is causing the site cache to be overloaded in few days. I could really need a solution to this.

Some details:

## Urls that are called

- - [02/Apr/2025:11:00:29 +0200] "GET /de/suche?f%5B0%5D=tags_und_themen%3A345&f%5B1%5D=tags_und_themen%3A479&f%5B2%5D=tags_und_themen%3A717 HTTP/1.1" 200 6325
- - [02/Apr/2025:11:00:31 +0200] "GET /en/suche?title=open-access&f%5B0%5D=format%3A591 HTTP/1.1" 200 5596
- - [02/Apr/2025:11:00:31 +0200] "GET /de/suche?f%5B0%5D=tags_und_themen%3A345&f%5B1%5D=tags_und_themen%3A790&f%5B2%5D=tags_und_themen%3A956 HTTP/1.1" 200 6078
- - [02/Apr/2025:11:00:34 +0200] "GET /de/suche?f%5B0%5D=tags_und_themen%3A126&f%5B1%5D=tags_und_themen%3A348&f%5B2%5D=tags_und_themen%3A761&f%5B3%5D=tags_und_themen%3A808 HTTP/1.1" 200 6022
- - [02/Apr/2025:11:00:34 +0200] "GET /de/suche?f%5B0%5D=tags_und_themen%3A346&f%5B1%5D=tags_und_themen%3A752&f%5B2%5D=tags_und_themen%3A832 HTTP/1.1" 200 5807
- - [02/Apr/2025:11:00:37 +0200] "GET /index.php/en/lernangebot/selbstlektuerekurs-buddhismus-suedasien HTTP/1.1" 200 5890

... those every second several times

@saif al-dilaimi We've had success against the same scenario you describe by updating Facets to the 3.x release and then rather than using normal Facets, configuring your view to use Facets as views exposed filters which is a new feature in the 3.x branch. Users have to select from a drop down (in our case) and submit the form. It's not quite as elegant or convenient, but so far has prevented the bots from using the filters. The URL format is slightly different so we also were able to block the bot from accessing the old facet URLs with .htaccess rules.

RewriteCond %{HTTP_HOST} HOSTNAME_HERE
RewriteCond %{REQUEST_URI} ^/OPTIONAL/PATH [OR]
RewriteCond %{REQUEST_URI} ^/OPTIONAL/SECOND/PATH
RewriteCond %{QUERY_STRING} f%5B
RewriteRule ^.* - [R=404,L]

People coming here with problems with spider traps on the facets should look at the newly released https://www.drupal.org/project/facet_bot_blocker . I haven't tested it yet, but its methodology allows still using checkbox-style facets (instead of dropdowns -- #99) by rate-limiting repeated queries for facets.

While this is still being worked on, I created a separate MR that improves the JS output of the checkbox widget: #3528483: Add missing classes to checkbox widget so it renders correctly

I have seen an increase in traffic recently, hitting a maximum yesterday and today, with Load Average hitting 97 on the server!

Interestingly, the visits now come from a varied range of IP's, so it's impossible to block them by IP, making it look like a DDOS. Maybe a new generation of multi-IP bots have been released?

So if anyone is able to do the required Git Magic to add a GitLab MR for Facets 3 based on the patch #93 (thanks @damienmckenna!) it would be awesome.

I created a fresh issue, to allow creating a GitLab MR (#95 comment patch) for Facets 3:

#3542180: [Facets 3] Render using theme input and select instead of lists with links for checkboxes and dropdown.

According to @ressa's comment on #3542180-5: [Facets 3] Render using theme input and select instead of lists with links for checkboxes and dropdown, this problem should only impact Facets 2.x but Facets 2 with BEF.

I have also tried to cross reference similar issues with "bad bots" issue tag.

Thanks @mrx576, since then I have tried many different approaches (all the ones mentioned on Block bad bots and crawlers) and so far Crawler Rate Limit is by far the most efficient bot-blocker, and combined with Solr and Redis, the bad bots are under control.

Attached a static patch of #83 that applies on top of the #3567669: The template_preprocess_item_list() function is deprecated in Drupal 11.3 issue for 2.0.x.

I have implemented hierarchy support for the Checkbox and Dropdown widgets.

Tests are currently failing on 2.0.x, see #3581851: [2.0.x] phpunit tests are failing. I'll clean the new test failures once 2.0.x is green.

I can't get the patch from MR 146 to apply to either Facets 2.0.10 or 2.0.x-dev. It looks like the MR needs to be rebased with 2.0.x-dev because the patch currently includes changes to tests/src/FunctionalJavascript/AjaxBehaviorTest.php which was deleted in 2.0.10.

In case someone in this Facets 2 issue is interested, there's an MR ready for review for Facets 3. Thank you @idebr!

#3542180: [Facets 3] Render using theme input and select instead of lists with links for checkboxes and dropdown