Make WordPress Core

Opened 2 years ago

Last modified 7 weeks 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: has-patch
Focuses: Cc:

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

Download all attachments as: .zip

Change History (11)

@nosilver4u
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 notice in 5.3

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

#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. :)

This ticket was mentioned in PR #2025 on WordPress/wordpress-develop by xaqrox.


7 weeks ago

  • Keywords has-patch added; needs-patch removed

I believe @dshanke's PR Wordpress/wordpress-develop#495 also fixes https://core.trac.wordpress.org/ticket/48204 but there is a syntax error.

Trac ticket: https://core.trac.wordpress.org/ticket/48204

### Original PR description:

This patch removes uses of strtotime from image metadata functions, replacing them with the DateTime class. It introduces a new wp_exif_datetime function that can be used for this. wp_exif_date2ts if now a wrapper around it that generates a timestamp for backcompat only. It replaces the use of the old function in the read metadata functionality and to ensure timezone is respected, stores as a new property the full datetime with timezone.

Trac ticket: https://core.trac.wordpress.org/ticket/49413

Note: See TracTickets for help on using tickets.