"A non-numeric value encountered" in PHP 7.1 #1272

Open
BrunoDeBarros opened this Issue Sep 27, 2016 · 39 comments

Projects

None yet
@BrunoDeBarros
Contributor
BrunoDeBarros commented Sep 27, 2016 edited

In PHP 7.1 there are a number of situations in the code where you'd get this error. I'm working through it right now to bring PHP 7.1 compatibility to dompdf (and will submit a PR when done), but I'm stumped by one thing:

The reason why this happens is because you've got things like $max_y = $frame->get_position("y") + $margin_height; in \Dompdf\FrameDecorator\Page, where $margin_height might be "auto". If it is "auto", obviously that's not a number, and that's the reason why we're getting these errors.

The default behaviour in PHP < 7.1 is to treat that as 0. So we could just look for situations where this happens and just manually force-convert "auto" to "0" and make the error go away.

However, that's not the best way to deal with this, as it'll lead to incorrect calculations. If it's "auto", then $max_y needs to take that into account, no? But looking through the code, it seems that $margin_height comes from $frame->get_margin_height(), which calls $style->length_in_pt(), which returns "auto" if the value is "auto". No calculation is done.

Would correcting this be a huge undertaking? Are the "auto" values calculated later on and I'm getting caught up in a situation where it just doesn't matter and can be ignored?

@bsweeney
Member

That definitely seems like incorrect handling though I'll need to take a look at the code to be sure. Seems like it would be more appropriate for length_in_pt to return zero because we're asking for a length. But I'd have to look through the code to see if we're using an "auto" return from that method in specific ways.

@bsweeney bsweeney added this to the 0.7.2 milestone Sep 27, 2016
@bsweeney bsweeney modified the milestone: 0.7.1, 0.7.2 Nov 11, 2016
@griffins

Just noticed there are a lot of places that are affected. Is there away we can surpress the error globally?

@BrunoDeBarros
Contributor
BrunoDeBarros commented Nov 28, 2016 edited

It only happens in $dompdf->render() (or ->stream(), if you use that), so if you use the silence operator @ there, you'll shut it up (and any other dompdf errors, so do be mindful of that). It's not ideal, but for the time being, it allows things to keep working until dompdf is updated.

@bsweeney
Member
bsweeney commented Dec 4, 2016

I'm looking at this right now and I can honestly say I'm not sure how much work this will require to address though it does seem to be potentially huge.

I'll probably start with the use of length_in_pt since that's the easiest to track down. We may have to iterate on this issue until we have fully canvased the code for any potential problems.

@BrunoDeBarros
Contributor

Yeah, I have no idea how to proceed because of the auto thing. It's easy to just let it keep behaving the way it has been all this time if ($var == "auto") {$var = 0;}, but I think this issue just highlighted a problem with the processing of units in dompdf in general, and that's likely to be a great big task. I wish I knew more about dompdf's workings to help out with this.

@griffins
griffins commented Dec 6, 2016

@bsweeney It seems to affect all places units are used. Any plans you have? i can push in some help since i use this library a lot!

@bsweeney
Member
bsweeney commented Dec 6, 2016

If length_in_pt is used prior to the calculation then a number should be returned (except for the special cases "auto" and "none").

Ultimately I'm thinking maybe length_in_pt should only return a number. In that case we'll need to add pre-call check to see if what we're passing is numeric. And if what's passed isn't numeric ... I'm not sure what would be best. We'll probably want to check for the special cases and only call length_in_pt only if they're not present. Though it may also be worthwhile to consider returning zero (the value in calculations of "auto" and "none) or the sum of the numeric values (as the minimum length). Really it'll require a bit of time to evaluate the current use cases across the library.

As an intermediate solution I'm leaning towards a post-call check against the special cases and using zero in calculations when there's a match.

@twify93
twify93 commented Dec 16, 2016 edited

I also encountered this issue in: \Dompdf\Css\Style::get_computed_border_radius() for if statement if ($rTL + $rTR + $rBL + $rBR == 0) when these values are empty strings.

And also for: $ret += (mb_substr($l, 0, $i) * 72) / $dpi;

@bbashy
bbashy commented Dec 20, 2016

One reason not to use 7.1 at the moment :(

@bsweeney
Member

Sadly, yes. If you really want to upgrade you can, as mentioned earlier, suppress these warnings by prefixing your dompdf calls with an @ ... but it's not really the best option. You could also temporarily disable warnings by changing the error reporting level using:

$error_level = error_reporting();
error_reporting(E_ALL & ~E_NOTICE & ~E_WARNING);
// dompdf code
error_reporting($error_level);
@bbashy
bbashy commented Dec 20, 2016

Yeah, thanks for the overview of solutions but I only used 7.1 locally. I'm running 7.0 for this now.

@manogi
manogi commented Dec 22, 2016 edited

I changed 6 files in my local dompdf to fix this (at least for my situation). Are you far into this yourself already or should I make a pull request with my files?

@manogi
manogi commented Dec 22, 2016

Ah OK thanks.

@bsweeney
Member

I've started work ... just haven't gotten very far yet. If you have found additional areas that need to be updated beyond those in 1331 please feel free to submit a pull request.

@bsweeney bsweeney added a commit that referenced this issue Jan 8, 2017
@bsweeney bsweeney Type cast to float
Fix-up for commit 5eb0558
Partial fix for #1272
e5ea4b4
@bsweeney bsweeney added a commit that referenced this issue Jan 13, 2017
@bsweeney bsweeney Use float values in arithmetic calculations
For compatibility with PHP 7.1 we need to be mindful to not use string
values in arithmetic calculations. For the most part we can just cast
to float, which comprises most of this commit. The majority of
instances where this was implemented were around usage of length_in_pt.

Some methods were not returning a float as indicated in the phpdoc.
This has been corrected where found.

Includes new helper methods to determine if the height/width are auto
sized.

Partially address #1272
06c9ebe
@bsweeney
Member

The just-pushed commit addresses this issue everywhere I found it. No doubt I missed a place or two, so if anyone has time please try it against some sample HTML you're likely to use.

@manogi
manogi commented Jan 13, 2017 edited

Thank you for looking into this. I tested this using the 0.7.x-dev branch right now.
The errors have disappeared in my PDFs, but I encountered new (smaller) problems.

I know it is not very helpful, but right now I only have the time to say that it seems that text-align: right in the CSS does not have an effect anymore. Also in one PDF it added two blank pages before the content started (even less helpful comment, since this only happened in one of my many PDFs, which are all the same kind of invoice).

I'm sorry I can't look deeper into this! I will when I get more time, maybe over the weekend.

@danielfnz

A have some error "A non-numeric value encountered"
"at HandleExceptions->handleError('2', 'A non-numeric value encountered', '/app/vendor/dompdf/dompdf/src/FrameDecorator/Page.php', '494', array('frame' => object(Inline), 'p' => null, 'margin_height' => 'auto')) in Page.php line 494"

@steffenweber

The PHP 7.1 compatibility issue I reported is solved in the develop branch. Thank you!

@bsweeney
Member

@danielfnz hm ... guess I shoulda left in the casing for that calculation. I'll take a closer look. get_margin_height is supposed to return a float.

@bsweeney
Member

@manogi I didn't see any issues with text alignment. If you can't find where the problem is feel free to post some sample HTML.

@bsweeney
Member

@danielfnz are you using the the code in the develop branch or one of the releases?

@v-wolf v-wolf referenced this issue in niklasravnsborg/laravel-pdf Jan 14, 2017
Closed

PHP 7.1 A non-numeric value encountered #21

@Twinski Twinski added a commit to Twinski/laravel-dompdf that referenced this issue Jan 17, 2017
@Twinski Twinski Update composer.json
Latest dompdf/dompdf addresses issue:
ErrorException in Block.php line 714:
A non-numeric value encountered

— dompdf/dompdf#1272
c6b9379
@graddus-corp

A have some error "A non-numeric value encountered" in Block.php line 51:

at HandleExceptions->handleError('2', 'A non-numeric value encountered', '/app/vendor/dompdf/dompdf/src/Positioner/Block.php', '51', array('frame' => object(Block), 'style' => object(Style), 'cb' => array('x' => '34.015748031496', 'y' => '34.015748031496', 'w' => '543.96850393701', 'h' => '723.96850393701', '34.015748031496', '34.015748031496', '543.96850393701', '723.96850393701'), 'p' => null, 'y' => '34.015748031496', 'x' => '34.015748031496', 'top' => 'auto', 'left' => 'auto')) in Block.php line 51

@twify93
twify93 commented Jan 18, 2017 edited

I presume it's because these lines:
Css/Style.php:513 and Css/Style.php:517

@bsweeney
Member

@graddus-corp have you tried the develop branch?

@bsweeney
Member

@twify93 those are definitely problematic lines and that method should probably only return a numeric value (see my earlier comment. The develop branch should sufficiently address the issue. I've been waiting for some feedback to see if I missed any code before I push to master.

@psybaron

The issue is solved with 0.7.x-dev. Waiting for push to master.

Thank you.

@graddus-corp

@bsweeney The issue is solved with the develop branch. But, in Absolute.php, line 96, I needed to put the code (casting variables to float):

$y += (float)$h - $height - (float)$bottom;

@bsweeney
Member

@graddus-corp thanks for the follow-up. I'll take another look at that file.

@sisve
sisve commented Jan 19, 2017

I'm still having issues at Block.php mentioned in #1339 (comment)

@bsweeney bsweeney added a commit that referenced this issue Jan 20, 2017
@bsweeney bsweeney Use float values in arithmetic calculations, part 2
Additional type casting to compliment the work in commit 06c9ebe.

Partially addresses #1272
18bcfb7
@bsweeney
Member

Thanks for the feedback, it definitely helped find some issues I missed previously. How do things look now?

@sisve
sisve commented Jan 20, 2017

Huzzah! All my [dompdf-related] issues are resolved! Rejoice! 🎉

@danielfnz

@bsweeney i change the dompdf version to dev-master and show a new error in a composer.

danielfnz/laravel-dompdf dev-master requires dompdf/dompdf dev-master -> satisfiable by dompdf/dompdf[dev-master] but these conflict with your requirements or minimum-stability.

do you know what is it?

@BrunoDeBarros
Contributor
BrunoDeBarros commented Jan 20, 2017 edited

Daniel,

That's an issue with your composer.json not being set to allow you to use unstable packages (dev-master is not a released version of dompdf), not related to dompdf and not dompdf's fault. Check out the documentation for more information https://getcomposer.org/doc/04-schema.md#minimum-stability. This Composer issue is also helpful for some more context and alternatives: composer/composer#5211

And just in case you're not aware: You shouldn't hijack issues; your error has nothing to do with this issue and is just spamming the rest of us who are following this issue and getting notifications for it. Next time, just start a new issue — they're free! And don't forget to Google first, too. 😉

@johnkwoods

I've also been getting this error, downloaded the master branch this morning, and it's still occurring. The full backtrace is too long for this conversation, so I'm only including the first line...

Warning in ./libraries/DisplayResults.php#871
A non-numeric value encountered

Backtrace

./libraries/DisplayResults.php#4943: PMA\libraries\DisplayResults->_getTableNavigation(...

@BrunoDeBarros
Contributor

There is no DisplayResults.php file in dompdf; this issue you're having is not related to dompdf.

@bsweeney
Member

@BrunoDeBarros thanks for triaging

@klopal
klopal commented Jan 21, 2017

@bsweeney do you have plans for releasing this update anytime soon?

@bsweeney
Member

I'll likely merge this to the master branch this week. At that point if you're using Composer you can switch from the versioned release to dev-master. As an official release I'm not sure yet as I'm trying to work through some other issues. Though I do understand this is a high priority for release so I'm pushing some issues to 0.7.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment