Problem/Motivation

The views exposed filter does not check if the passed query parameter is actually string, so whenever something else (foe example array) is passed, PHP fatal error is being raised: TypeError: Cannot access offset of type array in isset or empty in Drupal\views\Plugin\views\exposed_form\ExposedFormPluginBase->exposedFormAlter()

Steps to reproduce

  • Create a view with exposed sorting
  • curl 'https://example.com/views/ajax?_wrapper_format=drupal_ajax&sort_by[]=title&view_name=view_with_sort&view_display_id=main&_drupal_ajax=1'
TypeError: Cannot access offset of type array in isset or empty in Drupal\views\Plugin\views\exposed_form\ExposedFormPluginBase->exposedFormAlter() (line 225 of core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php).
Drupal\better_exposed_filters\Plugin\views\exposed_form\BetterExposedFilters->exposedFormAlter() (Line: 161)
Drupal\views\Form\ViewsExposedForm->buildForm()
call_user_func_array() (Line: 554)
Drupal\Core\Form\FormBuilder->retrieveForm() (Line: 303)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 134)
Drupal\views\Plugin\views\exposed_form\ExposedFormPluginBase->renderExposedForm() (Line: 1298)
Drupal\views\ViewExecutable->build() (Line: 1452)
Drupal\views\ViewExecutable->execute() (Line: 1516)
Drupal\views\ViewExecutable->render() (Line: 2488)
Drupal\views\Plugin\views\display\DisplayPluginBase->preview() (Line: 1718)
Drupal\views\ViewExecutable->preview() (Line: 169)
Drupal\views\Controller\ViewAjaxController->ajaxView()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->{closure:Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext():121}() (Line: 634)
Drupal\Core\Render\Renderer::{closure:Drupal\Core\Render\Renderer::executeInRenderContext():634}()
Fiber->resume() (Line: 649)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->{closure:Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber::onController():96}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 118)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 92)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 54)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 745)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Proposed resolution

Add is_string check

Remaining tasks

Review MR

User interface changes

None

Introduced terminology

None

API changes

None

Data model changes

None

Release notes snippet

None

Issue fork drupal-3605431

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

taran2l created an issue. See original summary.

taran2l opened merge request !16117

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Active » Needs review
drupal.ninja03’s picture

drupal.ninja03 commented
Status: Needs review » Needs work

I reviewed the MR changes and the current test failure.

The code change itself looks focused: `ExposedFormPluginBase::exposedFormAlter()` now checks that `$user_input['sort_by']` is a string before using it as a key in the exposed sort options. That seems like the right direction for handling malformed query parameters such as `sort_by[aaa]=bbb`.

However, the MR is not ready for RTBC because the added/updated functional test is currently failing.

The failure is in:

`Drupal\Tests\views\Functional\Plugin\ExposedFormTest::testExposedSortAndItemsPerPage`

The failing assertion is for cache contexts. The test expected contexts such as `url.query_args`, language contexts, theme, permissions, and authenticated role, but the actual cache contexts array is empty.

So I think the remaining work is to adjust the test so it correctly verifies the malformed `sort_by` case without introducing an incorrect cache context assertion. The regression test should confirm that passing `sort_by` as an array does not trigger a PHP error and that the expected result set is still rendered.

Suggested next step:

* Keep the `is_string()` guard.
* Fix the functional test failure around `assertCacheContexts()`.
* Confirm the pipeline is fully green afterward.

This should remain Needs work until the test failure is resolved.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

The failing of the pipeline is the test-only job, which we expect to fail, so that seems all good.

The added check makes sense. Test is straightforward. Left one tiny nitpick for the vcomment

Not sure this should be classified as a bug since there should be no normal way to click into this state as far as I know or is reported, but more of a task, but not too bothered.

taran2l’s picture

taran2l
Primary language Ukrainian
Location Lviv
commented
Status: Needs work » Needs review
smustgrave’s picture

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

Feedback from @Lendude appears to be addressed.

quietone’s picture

quietone commented

I didn't spot anything here that is amiss and I have updated credit.

  • godotislate committed 7355949b on main 
    fix: #3605431 Missing is_string check for the sort_by Views exposed form...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 7355949 to main and
bd4d699 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • godotislate committed bd4d699f on 11.x 
    fix: #3605431 Missing is_string check for the sort_by Views exposed form...