Make WordPress Core

Opened 21 months ago

Last modified 21 months ago

#48204 new defect (bug)

wp_exif_date2ts throws notice on empty date

Reported by: nosilver4u Owned by:
Milestone: Future Release Priority: normal
Severity: minor Version: 5.2.3
Component: Date/Time Keywords: needs-patch
Focuses: Cc:


When an image has the EXIF field DateTimeDigitized defined, but set to an empty timestamp, like

    :  :     :  : 

a PHP notice is thrown during upload.

I don't know how common this is, but I've seen it on several family photos that were scanned by my aunt. The EXIF info mentions both HP Scanjet 4370, FinePixViewer Ver.4.2, and FUJIFILM.

Anyway, it can be pre-empted with this between the two calls to explode():

if ( ':' === $date ) {
    return '';

I've attached a sample file to replicate the problem.

Attachments (1)

1956 in Iowa.JPG (49.5 KB) - added by nosilver4u 21 months ago.
file with "blank" DateTimeDigitized

Download all attachments as: .zip

Change History (10)

21 months ago

file with "blank" DateTimeDigitized

#1 @joemcgill
21 months ago

  • Keywords reporter-feedback added

Thanks @nosilver4u, can you share actual message from the PHP Notice you experienced here?

#2 @nosilver4u
21 months ago

oh sure, it's this:

Notice: Undefined offset: 2 in /var/www/html/wordpress/wp-admin/includes/image.php on line 637
Version 0, edited 21 months ago by nosilver4u (next)

#3 @nosilver4u
21 months ago

just tested on 5.2.3, and don't see the error, so perhaps that actually is a new notice in 5.3

Last edited 21 months ago by nosilver4u (previous) (diff)

#4 @joemcgill
21 months ago

  • Keywords needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release

Perfect. Thank you. Tested and am seeing the same error. The problem here is the assumption being made in this function is that there will only be one whitespace character in the string, because we're expecting a string formatted like yyyy:MM:dd HH:mm:ss, so the first explode() statement should split the date from the time, and the second explode should return the date parts. If the original string includes any additional whitespaces, as is the case here, those assumptions are incorrect.

As a sidenote, the docblock for wp_exif_date2ts() says that this function will always return an int but it could also return false since it's just passing along the results of strtotime() — see: https://www.php.net/manual/en/function.strtotime.php

#5 @nosilver4u
21 months ago

Indeed, and I see why the notice was not displayed before, version 5.2.3 uses the error suppression operator:

        @list( $date, $time ) = explode( ' ', trim( $str ) );
        @list( $y, $m, $d )   = explode( ':', $date );

#6 @joemcgill
21 months ago

Ah good catch. Looks like they were removed as a part of coding standards cleanup for #47632. Specifically commit [45611].

#7 @joemcgill
21 months ago

Best next steps here would be to add a unit test for wp_exif_date2ts() (if none exists already) that ensures that strings that match our expected format return the expected value and also that strings that don't match our expected format return a either a false value or null, then update the internals of that function so that both tests pass.

#8 @jrf
21 months ago

@rarst Something you may be interested in ? 👆

#9 @Rarst
21 months ago

  • Component changed from Media to Date/Time

Will look into. :)

Note: See TracTickets for help on using tickets.