PHP 7.1 and Drupal commerce, navigating to /admin/commerce/orders to view orders generates the following warning:
Warning: A non-numeric value encountered in theme_pager() (line 329 of /includes/pager.inc).
This is directly the result of the new warnings in php 7.1 performing arithmetic operations on variables that are empty or not integer. While I found this in Drupal commerce, the file generating the warning is in core so I suspect the same would occur outside diurnal commerce.
The code causing the problem is:
$pager_middle = ceil($quantity / 2);
and will generate the warning if $quantity is empty. To fix the problem, I replace the line where $quantity is set :
$quantity = $variables['quantity'];
with
$quantity = empty($variables['quantity']) ? 0 : $variables['quantity'];
I have attached the patch. This is the first time I submit a patch for core, apologies if I'm not following the correct protocol.
Comments
Comment #2
cilefen CreditAttribution: cilefen as a volunteer commentedI don't know if there is a PHP 7.1 meta issue.
Comment #4
SergFromSD CreditAttribution: SergFromSD commentedRecreated patch from root instead of from inside the includes directory.
Comment #5
Ayesh CreditAttribution: Ayesh as a volunteer commentedThis 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.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #8
sjerdoPatch #5 LGTM +1
Comment #9
caillou CreditAttribution: caillou commentedPatch #5 is working fine. Thanks.
Comment #10
MustangGB CreditAttribution: MustangGB commentedComment #11
Ruson CreditAttribution: Ruson commentedThank you!!! It worked for me!
Comment #12
joseph.olstadComment #13
mcdruidComment #14
Waichai CreditAttribution: Waichai commented「 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!
Comment #15
BartNijs CreditAttribution: BartNijs commentedPatch #5 worked for me (PHP 7.3)
Comment #16
joseph.olstadrolled 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
Comment #17
vinmassaro CreditAttribution: vinmassaro commentedTested the patch in #16 and it fixes the issue for us, thanks.
Comment #18
joseph.olstadComment #19
joseph.olstadComment #20
mcdruidSorry 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.
Comment #21
apadernoI 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.)Comment #22
apadernoActually
ceil($quantity / 2)
, where$quantity
is a floating point value, doesn't cause any warning in any PHP version.I also tried with:
NULL
The A non-numeric value encountered warning is only raised in the last two cases.
Comment #23
apadernoI didn't write tests, yet.
Comment #24
apadernoAll the failing tests are because the session_id(): Cannot change session id when session is active warning.
Comment #25
mcdruidTests 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):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.
Comment #26
Ayesh CreditAttribution: Ayesh as a volunteer commentedThanks 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.
Comment #27
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedI 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.
Comment #28
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedComment #29
joseph.olstad@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
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.
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.
Comment #30
joseph.olstadSo, 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.
Comment #31
mcdruidThanks @joseph.olstad
I'm not certain that
\Drupal\Core\Pager\PagerParametersInterface
guarantees that$variables['pager']['#quantity']
passed totemplate_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.
Comment #32
joseph.olstadI 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
Comment #33
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedThe 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.
Comment #34
stefanos.petrakis@gmail.comHere is a drafty patch that should break (and prove the breakage) for the same reasons in D8, just to facilitate the discussion.
Comment #35
stefanos.petrakis@gmail.comSecond take
And a little visual till the test completes:
Comment #37
stefanos.petrakis@gmail.comRight, 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.
Comment #38
stefanos.petrakis@gmail.comComment #39
joseph.olstadhere you go
Comment #40
joseph.olstad@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.
Comment #41
apadernoComment #42
mcdruid@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:
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 :)
Comment #43
mcdruidAdding 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?
Comment #44
mcdruidUseful feedback from @alexpott in the D8 parent:
We also discussed this in slack:
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.
Comment #45
mcdruid#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...
Comment #46
stefanos.petrakis@gmail.comHere we go.
Comment #48
joseph.olstadwow yes that is much better, great work Stefanos and Alexpott.
Comment #49
mcdruidThank you!
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:
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.
Comment #50
stefanos.petrakis@gmail.comThanks for the feedback, will be posting the update here in a couple of minutes.
Comment #51
stefanos.petrakis@gmail.comUpdated patch-work.
Comment #52
mcdruidThere'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.Comment #53
stefanos.petrakis@gmail.comOf course there is one more array there :-)
Next round, hopefully final one.
Comment #54
stefanos.petrakis@gmail.comPretty sure I did that already in the previous comment. Whatever.
Comment #55
apadernoIt seems tests are running slow. I added a patch in an issue for another project, and tests have been queued since 2 hours ago.
Comment #57
joseph.olstadback 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!
Comment #58
apadernoIt seems the tests fails to write the command output.
It's not the patch that is causing the tests to fail.
Comment #60
mcdruidQueued 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.
Comment #61
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedRTBM - let’s get that in
Comment #63
mcdruidAdding credit from the D8 parent.
Comment #65
mcdruidThank you contributors!