WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

#39250 closed defect (bug) (fixed)

Undefinded Variable in Media-Modal

Reported by: arkonisus Owned by: dd32
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: fixed-major needs-unit-tests
Focuses: ui Cc:

Description

If you try to place some images and open the media-modal you will get the following error if there are also PDFs or other filetypes than 'image' in the list:

PHP Notice:  Undefined variable: base_url in ... /wp-includes/media.php on line 3204

The problem is that the variable was declared inside an IF-clause some lines before (line 3172ff.):

} elseif ( isset( $meta['sizes'][ $size ] ) ) {
    if ( ! isset( $base_url ) )
        $base_url = str_replace( wp_basename( $attachment_url ), '', $attachment_url );

Change History (12)

#1 @Fencer04
6 months ago

So if I am editing a page and I click Add Media and the list has a PDF in it you are seeing the error? I just want to make sure I'm replicating it correctly because I have an image, a PDF and an mp3 and am not getting the error with the steps I mentioned above.

#2 @arkonisus
6 months ago

Sorry, it is a little bit strange ... I installed a blank WP and yes, it is all fine - no error! So i deleted all the plugins and theme data, posts and media in the faulty installation but the error was still present. OK, my last attempt was to reset the whole database ... and now all runs fine! It seems the reason was a buggy option in my database. But:

During my certain attempts i found out that it shows the default PDF-thumbnail in the media-modal and so i got the error. In the 'normal' media-view (upload.php) the thumbnail was correctly rendered and there was also no error message.

#3 follow-up: @Fencer04
6 months ago

  • Resolution set to invalid
  • Status changed from new to closed

Ok, this doesn't appear to be an error in the standard code base. I will close it.

#4 in reply to: ↑ 3 ; follow-up: @arkonisus
6 months ago

Replying to Fencer04:

Ok, this doesn't appear to be an error in the standard code base. I will close it.

Just after you closed this ticket i found out why this error appears ... :)
In my installation i set all image-sizes to '0' (settings->media) because i don't need any default thumbnails.

#5 in reply to: ↑ 4 @joemcgill
6 months ago

  • Keywords needs-patch needs-unit-tests good-first-bug added
  • Milestone changed from Awaiting Review to 4.7.1
  • Resolution invalid deleted
  • Status changed from closed to reopened

Replying to arkonisus:

In my installation i set all image-sizes to '0' (settings->media) because i don't need any default thumbnails.

This seems like a valid use case that WordPress should handle. The bug was introduced in [38949].

#6 @Fencer04
6 months ago

Interesting, I can't replicate this on my end but I agree that it is a valid use case.

#7 @dd32
6 months ago

Note; You'll need WP_DEBUG enabled to see PHP Notices.
That normally means we leave it for the next major release and don't do the .1, especially considering how edge case this is.

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


6 months ago

#9 @dd32
6 months ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from reopened to closed

In 39612:

Media: Move a variable definition outside of conditionals to ensure it's always available.
This fixes cases where the URL to a PDF preview may be incorrectly calculated when no thumbnails were generated for the PDF (and avoids a PHP Notice at the same time).

Fixes #39250.

#10 @dd32
6 months ago

  • Keywords fixed-major added; needs-patch good-first-bug removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

I never actually reproduced seeing a PHP Notice, but i did run into the side effects of it and reproduce the code paths.

Re-opening for 4.7.1, as it's a minor change and we've already modified the file, so no harm in including it.

Due to the complexities of testing it, I've not created a unit test for it, but if someone would like to attempt one, please feel free to.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 months ago

#12 @dd32
6 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39654:

Media: Move a variable definition outside of conditionals to ensure it's always available.
This fixes cases where the URL to a PDF preview may be incorrectly calculated when no thumbnails were generated for the PDF (and avoids a PHP Notice at the same time).

Merges [39612] to the 4.7 branch.
Fixes #39250.

Note: See TracTickets for help on using tickets.