Make WordPress Core

Opened 2 years ago

Last modified 2 years 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 2 years ago.
file with "blank" DateTimeDigitized

Download all attachments as: .zip

Change History (10)

2 years ago

file with "blank" DateTimeDigitized

#1 @joemcgill
2 years ago

  • Keywords reporter-feedback added

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

#2 @nosilver4u
2 years ago

oh sure, it's this:

Notice: Undefined offset: 2 in /var/www/html/wordpress/wp-admin/includes/image.php on line 637

FYI, I'm actually running 5.3-beta2-46373 on my dev site at the moment.

Last edited 2 years ago by nosilver4u (previous) (diff)

#3 @nosilver4u
2 years ago

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

Version 0, edited 2 years ago by nosilver4u (next)

#4 @joemcgill
2 years 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
2 years 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
2 years ago

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

#7 @joemcgill
2 years 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
2 years ago

@rarst Something you may be interested in ? 👆

#9 @Rarst
2 years ago

  • Component changed from Media to Date/Time

Will look into. :)

Note: See TracTickets for help on using tickets.