WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#36435 closed task (blessed) (maybelater)

Explicitly cast numeric values

Reported by: pento Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:
PR Number:

Description

PHP 7.1 will include warnings when invalid values are being used in numeric operations, per this RFC.

This is currently manifesting itself as a single unit test error against PHP Nightly, but it's likely that there are more places we'll see this warning, which aren't being covered by unit tests.

I'm not aware of any static analysis tools that are covering this case yet, but in the mean time, we could potentially look for problem code with some simple searching.

Attachments (3)

36435.diff (615 bytes) - added by lukecavanagh 3 years ago.
Patch for class-wp-comments-list-table.php
36435-random.diff (636 bytes) - added by joostdevalk 3 years ago.
Patch for compat/random.php
36435-PclZip.2.diff (1.3 KB) - added by ocean90 3 years ago.

Download all attachments as: .zip

Change History (21)

This ticket was mentioned in Slack in #core by pento. View the logs.


3 years ago

#2 @dd32
3 years ago

  • Milestone changed from Future Release to 4.6

Milestoning this as it's something we should really start looking into before the PHP 7.1 release, at least to know where/what will be affected.

#3 @pento
3 years ago

src$ ack --php --ignore-dir=ID3 --ignore-dir=wp-content/plugins --ignore-dir=SimplePie --ignore-dir=Requests '^\s*((?!//|\*).)*(\+\+|--)?\$[^ (),'\''";]+((\+\+|--[^>])| ([+*-]|/[^>]))' | wc -l
794

#4 @pento
3 years ago

In 37549:

Embeds: Ensure embed widths are integers.

This prevents a warning in PHP trunk when a non-integer width is passed.

See #36435.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


3 years ago

@lukecavanagh
3 years ago

Patch for class-wp-comments-list-table.php

#6 @lukecavanagh
3 years ago

Not sure if this was the direction you where thinking.

#7 @pento
3 years ago

That's the general idea. We should also be adding unit tests as we go.

PHP 7.1 is scheduled for release in November or maybe December, so we're going to need to make a serious dent in this task in 4.6.

@joostdevalk
3 years ago

Patch for compat/random.php

#9 follow-up: @joostdevalk
3 years ago

Added a patch, let me know if i got it right and I'll happily go over a few more files.

#10 in reply to: ↑ 9 ; follow-up: @SergeyBiryukov
3 years ago

Replying to joostdevalk:

Added a patch, let me know if i got it right and I'll happily go over a few more files.

Seems good, however random_compat is an external library, so the fix should probably be submitted upstream.

#11 in reply to: ↑ 10 @joostdevalk
3 years ago

Replying to SergeyBiryukov:

Replying to joostdevalk:

Added a patch, let me know if i got it right and I'll happily go over a few more files.

Seems good, however random_compat is an external library, so the fix should probably be submitted upstream.

You're right. done.

#12 @joostdevalk
3 years ago

The patch was accepted upstream, but of course there's no new release of it yet. What's the best way to go about that @SergeyBiryukov ?

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 years ago

#14 @ocean90
3 years ago

PclZip can produce a warning when it's used:

Notice: A non well formed numeric value encountered in /wp-admin/includes/class-pclzip.php on line 1849

Patched in 36435-PclZip.2.diff.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 years ago

#16 @ocean90
3 years ago

In 38101:

Filesystem API: Ensure memory limit calculations by PclZip are using integers.

This prevents a warning in PHP trunk, see https://wiki.php.net/rfc/invalid_strings_in_arithmetic.

See #36435.

#17 @ocean90
3 years ago

I'm using a fork of VVV which installs PHP 7.1 and haven't noticed any more warnings so far.

#18 @pento
3 years ago

  • Milestone 4.6 deleted
  • Resolution set to maybelater
  • Status changed from new to closed

I'm closing this for now, as we don't have a good way of detecting warnings. We can fix it piecemeal as bugs are found, or this ticket can be re-opened if a static analyser adds checking.

Note: See TracTickets for help on using tickets.