WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#24724 closed defect (bug) (fixed)

_wpMediaViewsL10n not correctly enqueued in wp_enqueue_media()

Reported by: Fab1en Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: major Version: 3.5.2
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

I use wp_enqueue_media() to use the new Media Library on the front end side and I found a very weird case where _wpMediaViewsL10n is not correctly declared in javascript before media-views script enqueue.

In the wp_enqueue_media function, wp_localize_script( 'media-views', '_wpMediaViewsL10n', $strings ); is called before wp_enqueue_script( 'media-editor' ); (which is dependent on media-views script), so there is a risk that _wpMediaViewsL10n is not taken into account if media-views script has not been enqueued before.

If wp_enqueue_media() is called on the front-end with the admin-bar deactivated, you will have a js error saying Cannot read property 'id' of undefined when trying to read wp.media.view.settings.post.id.

This issue can be solved by placing the wp_localize_script call just after wp_enqueue_script( 'media-editor' ).

Attachments (3)

24724_demo.php (801 bytes) - added by Fab1en 7 years ago.
Basic plugin that show the issue
24724.diff (1.3 KB) - added by ericlewis 6 years ago.
24724.2.diff (644 bytes) - added by nacin 6 years ago.

Download all attachments as: .zip

Change History (17)

#1 @nacin
7 years ago

It seems that this could occur when wp_enqueue_media() is used after wp_head, and thus scripts start to immediately be printed rather than being enqueued.

I'm not sure, though, I understand. wp_localize_script() can be called at any time after the handle is registered. In this case, media-views and media-editor are both registered by wp-includes/script-loader.php.

By calling wp_localize_script() first, we ensure that 'media-views' is localized by the time it is enqueued (by way of being a dependency of media-editor).

If wp_localize_script() came after the enqueue, then I'd definitely understand the potential problem (as described in my first sentence). That's what's being suggested, but it seems wrong/backwards.

#2 @Fab1en
7 years ago

In my use case, wp_enqueue_media() is used before wp_head.

The behavior is the opposite of what you are saying nacin : check the documentation http://codex.wordpress.org/Function_Reference/wp_localize_script.

Function Reference/wp localize script

Description

Localizes a script, but only if script has already been added. Can also be used to include arbitrary Javascript data in a page.

wp_localize_script() must came after the enqueue, or else there is a potential problem.

#3 @nacin
7 years ago

In this case, "added" means registered, not enqueued.

While enqueueing a full URL automatically registers it, that's not what is occurring here. script-loader registers all core scripts; wp_enqueue_script( $handle ) then queues it for printing.

Not saying there isn't a potential bug here, I just can't replicate and I don't see a problem with the function call order.

@Fab1en
7 years ago

Basic plugin that show the issue

#4 @Fab1en
7 years ago

I have added a plugin that enqueues the media editor files on the frontend. You can use it with WP 3.5.2 and Twenty Twelve theme.

  1. Look at the generated source code for the home page, and note that media-views.min.js is present without the preceding _wpMediaViewsL10n declaration.
  2. Comment out the filter that removes the admin bar, and see that _wpMediaViewsL10n declaration is back.

#5 @ericlewis
6 years ago

Reproduced.

The issue is that wp_localize_script() doesn't instantiate $wp_scripts if it doesn't exist1, bailing silently.

On the admin side, $wp_scripts exists very early (before admin_init)2. But on the front-end, there's no guarantee$wp_scripts will ever be instantiated.

@ericlewis
6 years ago

#6 @ericlewis
6 years ago

  • Keywords has-patch added
  • Severity changed from trivial to major

In attachment:24724.diff, instantiate WP_Scripts in wp_localize_script() if it hasn't been already. Fixes using wp_enqueue_media() on the front-end.

#7 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 4.0

#8 @wonderboymusic
6 years ago

specific reason to remove the return?

#9 @wonderboymusic
6 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 28840:

In wp_localize_script(), instantiate the $wp_scripts global instead of bailing when it is called before wp_enqueue_scripts. This allows wp_enqueue_media() to be called on the front end with no JS errors.

Props ericlewis.
Fixes #24724.

#10 @nacin
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[28840] is the incorrect fix here.

wp_localize_script() should only be called on an already registered handle and only once translations are loaded. This return false has existed since the initial commit of the entire script loader ([7970]), so this smells like it is a developer error. Changing it now will allow plugin developers to start using this function earlier than intended and it will be near impossible to walk this back once it is released.

The issue is core is using its API incorrectly. wp_localize_script() should only be called on an already registered handle, but in core's case, we register our handles just-in-time. So we only instantiate WP_Scripts when we need it and our scripts are only registered when some code tries to enqueue something. Basically, we shouldn't be calling wp_localize_script( 'media-views' ) before any guarantee that anything — including 'media-views' — is enqueued.

Simply enqueuing media-editor before localizing media-views fixes everything without opening up wp_localize_script() up to abuse.

Again, as long as you are using it against an already registered handle, then this is only an issue when attaching something to a core script that hasn't yet been JIT registered, which is only something core should be doing. We can and should work around it.

@nacin
6 years ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


6 years ago

#12 @SergeyBiryukov
6 years ago

  • Keywords commit added

24724.2.diff fixes the issue (assuming [28840] is reverted) and looks good to me.

#13 @nacin
6 years ago

In 29677:

Revert [28840]; wp_localize_script() must be called on a registered handle.

see #24724.

#14 @nacin
6 years ago

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

In 29678:

Media: Ensure media-views is enqueued and registered before localizing.

fixes #24724.

Note: See TracTickets for help on using tickets.