WordPress.org

Make WordPress Core

Opened 10 days ago

Last modified 10 days 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:
PR Number:

Description

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 10 days ago.
file with "blank" DateTimeDigitized

Download all attachments as: .zip

Change History (10)

@nosilver4u
10 days ago

file with "blank" DateTimeDigitized

#1 @joemcgill
10 days ago

  • Keywords reporter-feedback added

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

#2 @nosilver4u
10 days 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 10 days ago by nosilver4u (previous) (diff)

#3 @nosilver4u
10 days 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 10 days ago by nosilver4u (previous) (diff)

#4 @joemcgill
10 days 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
10 days 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
10 days ago

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

#7 @joemcgill
10 days 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
10 days ago

@rarst Something you may be interested in ? 👆

#9 @Rarst
10 days ago

  • Component changed from Media to Date/Time

Will look into. :)

Note: See TracTickets for help on using tickets.