Pager option 'Items per page' with empty value gives WSOD

Looking on this issue..

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

I have raised a Merge Request (MR) to address the TypeError that occurs when in view Items per page is left empty. This issue was causing unexpected behavior, and the MR ensures proper handling of such cases to prevent errors.

I added the required attribute in pager options form in views below is the patchhttps://git.drupalcode.org/project/drupal/-/merge_requests/11050.patch

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Is this not a duplicate of #3361177: An empty views pager offset field can cause a PHP type error to be triggered

Other one is RTBC what more would need to be done here?

This is for items per page, that issue is for offset (different settings)

fair enough.

Rebased as it was a few 100 back, but still green.
Then ran the test-only feature https://git.drupalcode.org/issue/drupal-3501659/-/jobs/7714753 and got coverage

Change seems straight forward just casting as (int), rest of the changes appear to be refinement as it was calling the same array key un-neccessarily.

LGTM

I used the following steps to test this:

1. Install 11.x with the Standard profile.
2. Go to /admin/structure/views/view/comment.
3. Click "Paged, 50 items", blank out items per page, and click "Apply".
4. Observe the following error message:
   
5. Click save, and observe the WSOD:

   

6. The view is perma-borked and cannot be edited -- always a WSOD whenever I visit its edit URL from that point on -- so pretty major data integrity bug that allows this to be saved at all.

However, I manually tested with the diff from the MR applied and it did not appear to resolve the issue. I no longer get a console error in step 4 when I blank out the field, but I do still get a WSOD when I save the View in step 5 or try to edit it in step 6. The log message is still:

TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, null given, called in /Users/xjm/git/drupal-test-site/core/lib/Drupal/Component/Render/FormattableMarkup.php on line 238 in Drupal\Component\Utility\Html::escape() (line 433 of /Users/xjm/git/drupal-test-site/core/lib/Drupal/Component/Utility/Html.php).

I retried a few times, including the classic echo "hi"; to make sure the code was actually applied. 😉

The fix from #3564838: Views: Fatal error when rendering NULL field values due to strict string typing in PHP 8.3+ appears to address that issue, should they be combined?

And I guess the test coverage needs to be a kernel or functional test to cover the WSOD?

Yeah, not sure if it should be combined or merely postponed, but we definitely need functional test coverage.

If this issue is only about the JS error, it's mis-titled, because the "WSOD" is the typecast problem and the major bug (possibly critical if it affects other elements besides this one).

Just actually read #3564838: Views: Fatal error when rendering NULL field values due to strict string typing in PHP 8.3+ and I think that's a wontfix. Instead, I think needs to be fixed somewhere lower in the Views plugin system, but Html::placeholderEscape() indiscriminately casting things to strings sounds kind of evil.

Funny enough I just got hit with the bug in #3564838: Views: Fatal error when rendering NULL field values due to strict string typing in PHP 8.3+ and I'm not using views, testing something with a route for an unrelated ticket.

We can jump over to that ticket but what if we don't case a string and instead just add ($value ?? '') that fixes it too

#3564838: Views: Fatal error when rendering NULL field values due to strict string typing in PHP 8.3+ and #3361177: An empty views pager offset field can cause a PHP type error to be triggered also make me think we need this fixed at the Views plugin level in two ways:

1. Converting empty user input to a plugin-appropriate empty value (zero or whatever).
2. On edit or save, handling existing null/wrongly empty-typed values and casting them to the plugin-appropriate empty value so the view isn't perma-fried because you paged your view by empty-string back in Drupal freaking 6 or something.

In the short term, we can fix it for just this plugin, because this specific bug is quite bad. In general, though, we should have a followup to fix this generically at a lower level in Views.

@smustgrave re: #31 I guess we can discuss over there and ask for FM input, but my initial reaction is extreme "yeahrghch". 😅

If we fix at the plugin level though won't we have to fix ALL the pager plugins, and all the instances where it checks $options['somekey'], and if anyone is doing a custom plugin hope they update too?

Updating the plugins to not be empty but 0 makes sense but would need an upgrade hook I imagine as the bad values are already stored.

Example in Full.php

    return $this->formatPlural($this->options['items_per_page'], '@count item', 'Paged, @count items', ['@count' => $this->options['items_per_page']]);

believe this is the code causing the WSOD because items_per_page is empty.

We don't need an upgrade hook necessarily; we've historically handled views data integrity issues along these lines in a gentle way where repair them on edit load or save. (Edit: That doesn't mean we shouldn't consider one; it might indeed be a good idea as we move to strict schemata.)

By the time we get to the line in #35, Views needs to have already converted the empty string to a zero. That's what we need to fix. (Versus changing FormattableMarkup to silently accept bad input which is an even bigger and lower-level change.)

So from what I can gather we need to:

1. Fix the WSOD for existing views as well (need to figure out why the fix wouldn't work for these)
2. Add test coverage with an existing view

I was wrong, something has definitely changed since I wrote the fix because I can reproduce the WSOD on new views too.

It's coming from the pager plugin's summaryTitle method. I agree that we should investigate if the options can be cast to integers directly.

Could make those number fields required then can’t trigger the WSOD

That is definitely something we should look at doing. That would not solve existing broken views though. I've debugged through the web of code to where $this->options actually comes from. It seems like it just grabs all the options directly from config and passes them down the chain.

E.g DisplayPluginCollection::initializePlugin is called for the default display (and other displays). The $display array comes from View::getDisplay (the config entity). $display['display_options']['pager']['options'] are the options that eventually get passed into the pager display plugin and that has the null in there already.

It looks like config validation doesn't save us here either, the views_pager data type already defines these as integers with a PositiveOrZero constraint.

This would at least solve the issue at a higher level:

--- a/core/modules/views/src/Plugin/views/pager/PagerPluginBase.php
+++ b/core/modules/views/src/Plugin/views/pager/PagerPluginBase.php
@@ -3,7 +3,9 @@
 namespace Drupal\views\Plugin\views\pager;
 
 use Drupal\Core\Form\FormStateInterface;
+use Drupal\views\Plugin\views\display\DisplayPluginBase;
 use Drupal\views\Plugin\views\PluginBase;
+use Drupal\views\ViewExecutable;
 
 /**
  * @defgroup views_pager_plugins Views pager plugins
@@ -60,6 +62,15 @@ abstract class PagerPluginBase extends PluginBase {
     'h6' => 'H6',
   ];
 
+  /**
+   * {@inheritdoc}
+   */
+  public function init(ViewExecutable $view, DisplayPluginBase $display, ?array &$options = NULL) {
+    $options['items_per_page'] ??= 0;
+    $options['offset'] ??= 0;
+    parent::init($view, $display, $options);
+  }
+

EDIT: Scratch that, that still ends up with Unsupported operand types: int * string as somehow items_per_page is an empty string.

FYI this same error is popping up in other places. https://www.drupal.org/project/drupal/issues/3577075

Should we fix at the FormattableMarkup just stop the bleed and maybe log errors to then fix the root causes like the ones here