[D7] PHP 7.1 warning: A non-numeric value encountered in theme_pager()

I don't know if there is a PHP 7.1 meta issue.

This warning is raised for all non-numeric values since PHP 7.1 (http://php.net/manual/en/migration71.other-changes.php#migration71.other...), so I think the best approach is to convert it to an integer; not just an empty value.

Patch #5 LGTM +1

Patch #5 is working fine. Thanks.

Thank you!!! It worked for me!

「 I replace the line where $quantity is set :

$quantity = $variables['quantity'];

with

$quantity = empty($variables['quantity']) ? 0 : $variables['quantity'];」

I did it onto the /includes/pager.inc and it works fine for me. Now I am using php 7.2.
Only this action solved my headache for a few months. Thanks so much!

Patch #5 worked for me (PHP 7.3)

rolled new patch based on both approaches from 5 and 14

this should be ultra bulletproof, in case for whatever reason quantity is null it will be cast to int and become 0, if it is empty false will become 0

Tested the patch in #16 and it fixes the issue for us, thanks.

Sorry to push back on what seems like a simple fix, but AFAICS there are no tests in core which fail under PHP 7.3 without this patch. Is that correct?

If that is the case, we'd ideally add one or more tests here to verify the issue and that this patch fixes it.

+  $quantity = (int) empty($variables['quantity']) ? 0 : $variables['quantity'];

I am not sure what the purpose of casting to an integer is, since that line would assign to $quantity a floating point value, if $variables['quantity'] contains a floating point value. (See https://3v4l.org/UITl6.)

Actually ceil($quantity / 2), where $quantity is a floating point value, doesn't cause any warning in any PHP version.

I also tried with:

• NULL
• A string containing a number
• An empty string
• A string that doesn't contain a number

The A non-numeric value encountered warning is only raised in the last two cases.

I didn't write tests, yet.

All the failing tests are because the session_id(): Cannot change session id when session is active warning.

Tests should pass under PHP 7.3 now.

I think casting to an int (per #24) should be sufficient in all but the most bizarre of edge cases.

I had a quick look for these errors on some live sites, and they weren't hard to find. It looks like they often come from two places in theme_pager.inc (with line numbers):

327   // Calculate various markers within this pager piece:                         
328   // Middle is used to "center" pages around the current page.                  
329   $pager_middle = ceil($quantity / 2);                                          
330   // current is the page we are currently paged to                              
331   $pager_current = $pager_page_array[$element] + 1;                             
332   // first is the first page listed by this pager piece (re quantity)           
333   $pager_first = $pager_current - $pager_middle + 1;                            
334   // last is the last page listed by this pager piece (re quantity)             
335   $pager_last = $pager_current + $quantity - $pager_middle; 

Line 329 is mentioned in the IS, but it looks like line 335 also generates the same warning/error when $quantity is non-numeric.

In general, I think we'll try to avoid adding too many "guard rails" for custom/contrib code calling core functions with incorrect parameters, especially where doing so might have an adverse affect on performance.

However, in this case casting $quantity to an int seems like it ought to be quite low cost, and if it avoids sites generating multiple PHP Warnings (which some sites will write to the db via dblog) for every call to this function... that seems like it's worth doing to me.

You could probably argue it's a sufficiently trivial change, and one that will only come into play when this function is called by custom/contrib code, that we might not require tests for this. Let's see what Fabianx thinks of that.

Thanks Drew.

Int cast should be a trivial performance hit that we can ignore, which would have been if we were to use `intval()` which is not the case.

I'd be happy to write a test covering non int-ish edge cases if Fabianx would prefer tests for this.

I generally agree here that the patch is trivial.

Performance is not a concern.

I also don’t think we need to guard against a regression of that line reverting back to something else. (Which tests would provide)

However it’s a bug fix for code that’s most probably in Drupal 8, so it needs to be checked if we need to fix it in D8 as well.

@FabianX,

This is not an issue in D8 core because the PagerParametersInterface getQueryParameters returned as an int:

see: core/lib/Drupal/Core/Pager/PagerParametersInterface.php

  public function getQueryParameters();

  /**
   * Returns the current page being requested for display within a pager.
   *
   * @param int $pager_id
   *   (optional) An integer to distinguish between multiple pagers on one page.
   *
   * @return int

The above RTBC patch /RTBM patch is ok but alternatively you could resolve this by doing the following:

edit pager_get_query_parameters in includes/pager.inc , for each parameter, check is_numeric , if the value is_numeric then cast it to an int.

diff --git a/includes/pager.inc b/includes/pager.inc
index c060d0e7cd..ddf1d54589 100644
--- a/includes/pager.inc
+++ b/includes/pager.inc
@@ -294,7 +294,14 @@ function pager_default_initialize($total, $limit, $element = 0) {
 function pager_get_query_parameters() {
   $query = &drupal_static(__FUNCTION__);
   if (!isset($query)) {
-    $query = drupal_get_query_parameters($_GET, array('q', 'page'));
+    $queryTemp = drupal_get_query_parameters($_GET, array('q', 'page'));
+    $query = array();
+    foreach($queryTemp as $item => $key) {
+      $query[$key] = $item;
+      if (is_numeric($query[$key])) {
+        $query[$key] = (int) $item;
+      }
+    }
   }
   return $query;
 }

Now, I haven't tested this.

otherwise, ya the existing patch is easy.
one line code change. that is likely sufficient and all that is needed. Just thought I'd show maybe what we might alternatively approach this.

So, maintain RTBC, I would recommend to jam this in.
1) This is not needed in D8, see my explanation above in #29
2) patch is very simple and has been reviewed by core maintainers.

Thanks @joseph.olstad

I'm not certain that \Drupal\Core\Pager\PagerParametersInterface guarantees that $variables['pager']['#quantity'] passed to template_preprocess_pager() will be an int (especially as I think the comments you quoted actually relate to \Drupal\Core\Pager\PagerParametersInterface::findPage).

The code in question in the preprocess function in pager.inc is in fact very similar between D7 and D8, and it may be possible to pass a non-numeric value.

On the other hand, I searched logs across around several thousand Drupal sites I help to look after, and I can see quite a few occurrences of the Warning coming from D7 sites, and none from D8. That's hardly scientific, but I'm not sure it's an issue in D8. If it's impossible for it to happen in D8, then we'll probably struggle to get this change committed there. I'll look into it a little more.

I am going by the D8 code. If @return int means something, then it's returning an integer. This probably explains why there is no existing issue for this in D8. I searched but could not find one. We have done due diligence, I think we can move forward.

see: core/lib/Drupal/Core/Pager/PagerParametersInterface.php

  public function getQueryParameters();

  /**
   * Returns the current page being requested for display within a pager.
   *
   * @param int $pager_id
   *   (optional) An integer to distinguish between multiple pagers on one page.
   *
   * @return int

The annotation / doxygen means nothing - it is by all means and purposes for PHP just a comment that is ignored.

Because it is just a hint and not interpreted by PHP.

Also the theme function could be misused in 8.x as well.

I think we need to at least consult the 8.x maintainers if they _want_ to fix it in 8.x template_preprocess_pager().

If they don't we can still fix it - given the impact, but we need to ask first.

Here is a drafty patch that should break (and prove the breakage) for the same reasons in D8, just to facilitate the discussion.

Second take

And a little visual till the test completes:

Right, so, as @Fabianx anticipated, D8 suffers from the same problem.

If there are no objections, I would open a ticket using this same dummy testing to demonstrate the problem and link that issue back here.

I could also provide some tests for this issue, unless Ayesh as mentioned in #26 wants to go ahead with that.
Scrap that part about tests, I agree with 27.

here you go

@Fabianx , doxygen should mean 'something, as-in, the return type should reflect what is said in doxygen. Otherwise, why write any doxygen?

I did check though, the PagerParameters calls a function called filterQueryParameters in core/lib/Drupal/Component/Utility/UrlHelper.php

however despite the semantic sounding name starting with 'filter' , there is no is_numeric check , and thus no subsequent (int) $value casting.

@joseph.olstad without wishing to "flog a dead horse" over this, the doxygen / annotation / comments you were referencing actually belong to a different method - this is the whole section from \Drupal\Core\Pager\PagerParametersInterface:

  /**
   * Gets all request URL query parameters that are unrelated to paging.
   *
   * @return array
   *   A URL query parameter array that consists of all components of the
   *   current page request except for those pertaining to paging.
   */
  public function getQueryParameters();

  /**
   * Returns the current page being requested for display within a pager.
   *
   * @param int $pager_id
   *   (optional) An integer to distinguish between multiple pagers on one page.
   *
   * @return int
   *   The number of the current requested page, within the pager represented by
   *   $element. This is determined from the URL query parameter
   *   \Drupal::request()->query->get('page'), or 0 by default. Note that this
   *   number may differ from the actual page being displayed. For example, if a
   *   search for "example text" brings up three pages of results, but a user
   *   visits search/node/example+text?page=10, this function will return 10,
   *   even though the default pager implementation adjusts for this and still
   *   displays the third page of search results at that URL.
   */
  public function findPage($pager_id = 0);

Because these are from an interface the functions / methods don't have curly braces enclosing any code; they have a signature and then end immediately with a semicolon. The relevant comments are above each function signature.

So it's \Drupal\Core\Pager\PagerParametersInterface::findPage which is supposed to return an int here.

As mentioned, however, this isn't enforced and is not by itself a guarantee.

Just wanted to make sure that was clear :)

Adding the [D7] prefix to the title here per the docs, as this is now a backport issue.

@stefanos.petrakis thanks for the test and the D8 parent issue. I've commented there to say I think the test should be included now that the work has been done.

Correspondingly it'd be good to include a test in this issue, even although we've said it's not strictly necessary. Perhaps it doesn't fit so easily into D7's testing framework, but ideally what we commit to D7 should match the D8 parent.

Anyone want to write a very basic test for this, please?

Useful feedback from @alexpott in the D8 parent:

I think we should do $quantity = empty($variables['pager']['#quantity']) ? 0 : $variables['pager']['#quantity'];. Using (int) potentially hides helpful warning if you do actually pass a value like 'a_string_value' in.

We also discussed this in slack:

alexpott
Well I’m happy to harden D8 and do
$quantity = empty($variables['pager']['#quantity']) ? 0 : $variables['pager']['#quantity'];

alexpott
But I agree that making the same fix in D7 is a higher priority as we have reports of the warning occurring in the wild where #quantity is an empty value.

mcdruid
Ok, that makes sense. So we won't hide nonsense being passed in, but not trigger warning/errors when it's empty.
:+1:

The test in the D8 parent also needs some work as it's failing as "Risky" because it doesn't make any assertions. I think @stefanos.petrakis's intention was just to illustrate the issue rather than propose the test be committed.

FWIW I think we're more likely to get the D8 patch in with a proper test, and that it makes sense for us to add a corresponding test here. Hopefully that's not adding a lot of extra work for what I know is a pretty trivial change. Tests are good though :)

Let's change this patch per the suggestion from @alexpott, and add a basic test for D7.

#3094536: PHP 7.1 Warning: A non-numeric value encountered in template_preprocess_pager() is looking in decent shape now, I think. If we get an RTBC over there, let's get this backport into a matching state...

Here we go.

wow yes that is much better, great work Stefanos and Alexpott.

Thank you!

   /**
+   * Tests theme_pager() when an empty quantity is passed.
+   */
+  public function testThemePagerQuantityNotSet() {
+    $variables = [
+      'element' => '',
+      'parameters' => [],
+      'quantity' => '',
+      'tags' => '',
+    ];
+    $rendered_output = theme_pager($variables);
+    $this->assertEqual(NULL, $rendered_output);
+  }

We need to use the old array() syntax in D7.

I think I'd like to see the assertion actually checking something positive (as mentioned by @alexpott over in the D8 parent).

It seems a bit arbitrary, but perhaps something like:

>>> $variables = array('element' => 0, 'parameters' => array(), 'quantity' => '', 'tags' => '');
=> [
     "element" => 0,
     "parameters" => [],
     "quantity" => "",
     "tags" => "",
   ]
>>> pager_default_initialize(100, 10);
=> 0
>>> theme_pager($variables);
=> """
   <h2 class="element-invisible">Pages</h2><div class="item-list"><ul class="pager"><li class="pager-ellipsis first">…</li>\n
   <li class="pager-ellipsis">…</li>\n
   <li class="pager-next"><a title="Go to next page" href="/node?page=1">next ›</a></li>\n
   <li class="pager-last last"><a title="Go to last page" href="/node?page=9">last »</a></li>\n
   </ul></div>
   """

We could check for the presence of - say - the string 'next' in the output from theme_pager().

That'd then be very similar to what's just been committed to D8.

Thanks for the feedback, will be posting the update here in a couple of minutes.

Updated patch-work.

+    $variables = [
+      'element' => 0,
+      'parameters' => array(),
+      'quantity' => '',
+      'tags' => '',
+    ];

There's still one array using the [] syntax :)

I may be missing something, but I don't think we need to set up the global variables ourselves; pager_default_initialize() will do that.

Of course there is one more array there :-)

Next round, hopefully final one.

Pretty sure I did that already in the previous comment. Whatever.

It seems tests are running slow. I added a patch in an issue for another project, and tests have been queued since 2 hours ago.

back to RTBCI
it's over the top good because the cast to int was doing the job in the wild, however yes this is better imho, both valid solutions. Tests backported , even better.

Nice work Stefanos and Alexpott!

It seems the tests fails to write the command output.

Failed to write file "/var/lib/drupalci/workspace/jenkins-drupal_d7-24069/artifacts/runcontainers/command_output"

It's not the patch that is causing the tests to fail.

Queued the patch we expect to pass for a retest; IIUC that way the issue won't be marked as failing tests (it's based on the last test to run).

With any luck I think we're ready to RTBC and ask for framework manager review, but I'll let that retest run first.

RTBM - let’s get that in

Adding credit from the D8 parent.

Thank you contributors!