ViewsExposedFilterBlock::build must always return array

I'll do the review.

I tested the changes that @Chi made and it works fine, it's a small change the patch is fine.

I'll try to work on this issue.

Here is a quick change to the code for this issue. I just added a conditional to check if the value is null.

Thank you @andregp for working at this. But the #2 does exactly the same but it's more concise. Drupal 9 shouldn't support php lower than 7.3, so we can use that short syntax return $output ?? [] .
I think the cause of the test errors for #2 was not in the patch itself, but rather some issue with the test system, like this https://www.drupal.org/project/drupal/issues/3203712 . I rerun tests and it passes all without errors now. So, I think we should go with #2.

The patch from @2 looks good, and as the tests now passed, changing this issue back to RTBC.

There was a problem in testing due to Composer update to 2.2 https://www.drupal.org/project/drupal/issues/3255836

Getting back to RTBC.

This could use some test coverage.

Adding tests

I'm not sure this change is necessary or actually buying us anything. The test is certainly giving us more to maintain.

In \Drupal\block\BlockViewBuilder::preRender() we do

    $content = $build['#block']->getPlugin()->build();
    // Remove the block entity from the render array, to ensure that blocks
    // can be rendered without the block config entity.
    unset($build['#block']);
    if ($content !== NULL && !Element::isEmpty($content)) {

So we explicit handle block plugins that return a NULL on the build.

I've looked at quite a few blocks in core (& contrib) and they do all return arrays.

I think the we should consider making the following change:

$output = $this->view->display_handler->viewExposedFormBlocks() ?? [];

I.e. always define $output as an array. And not add a test and wait for return typehints to be added everywhere for compiler based enforcement. Going to ping @catch because this is different from #14

Looking a bit more... the code would become:

    $output = $this->view->display_handler->viewExposedFormBlocks() ?? [];
    // Provide the context for block build and block view alter hooks.
    // \Drupal\views\Plugin\Block\ViewsBlock::build() adds the same context in
    // \Drupal\views\ViewExecutable::buildRenderable() using
    // \Drupal\views\Plugin\views\display\DisplayPluginBase::buildRenderable().
    if (!empty($output)) {
      $output += [
        '#view' => $this->view,
        '#display_id' => $this->displayID,
      ];
    }

So less logic in the if for the win.

Addressing #18 and #19.
Changed the code on ViewsExposedFilterBlock.php and left it without tests.

Sorry, I did a silly mistake on patch #20.

Answering @Chi: The code previously was

if (is_array($output) && !empty($output)) {

Checking if $output was an array and if it was not empty. As now we convert $output to [] when it's null it will be always an array (either empty or not), so we just remove the is_array($output) from the if statement.

Return type hints (eventually) instead of tests to maintain sounds like a good plan here.

Hi, I'll do the review!

Assigning it!

I reviewed 3230681-22.patch, it looks nice to me, it guarantees the variable $output to be always an array. As mentioned on comment #23, I also inserted a return type hint to build() method.

@jobsons #27 is missing the change from #22, this needs one patch with all necessary changes in order to be committed. Additionally, core issues need review by someone other than the patch author or a committer before they can be marked RTBC (i.e. in general, if you wrote the patch, especially the most recent version of it, you shouldn't also RTBC it).

Not sure why #27 isn't applying, but the return type hint looks right to me otherwise.

Re-rolling and Updating patch.

Unassigning the task

I applied patch in #29 for branch 9.5.x and it worked for me.

- Changes from #22 were included, as reviewed in #28;

- @hmendes' test was replaced for type-hints, as suggested in #18 and #23;

- Tests are passing and no code standards problems were introduced with the changes;

- The proposed resolution to return empty array instead of NULL in ViewsExposedFilterBlock was performed.

Taking this into account, I think it can be mark as RTBC.

Discuseed with @catch we agreed to add the return typehint in 10.x because it is a major release and allowed because it is a plugin and therefore internal. Backported to 9.5.x without the typehint.

Committed and pushed f6bd3a527a to 10.1.x and fd9ecc1746 to 10.0.x. Thanks!
Committed 80e342f and pushed to 9.5.x. Thanks!