Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#35873 closed defect (bug) (fixed)

wp_register_script grandchild level dependency not rendered

Reported by: dima-stefantsov's profile Dima Stefantsov Owned by: ocean90's profile ocean90
Milestone: 4.5 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch has-unit-tests needs-testing
Focuses: javascript Cc:

Description

http://wordpress.stackexchange.com/questions/218230/wp-enqueue-script-with-dependencies-doesnt-work

I'm having this code:

wp_register_script('parent','parent.js', array('child'), '1', true);
wp_register_script('child', 'child.js', array('grandchild'), '1', true);
wp_register_script('grandchild', 'grandchild.js', array(), '1', true);
wp_enqueue_script('parent');

and it works fine, rendering grandchild.js, then child.js, then parent.js in footer.

Every combination of just parent and child works fine, regardless of TRUE or FALSE 'render in footer'. But when I register 'child' to be rendered in head instead:

wp_register_script('parent','parent.js', array('child'), '1', true);
wp_register_script('child', 'child.js', array('grandchild'), '1', FALSE);
wp_register_script('grandchild', 'grandchild.js', array(), '1', true);
wp_enqueue_script('parent');

Then child.js gets rendered in head, parent.js gets rendered in footer, and grandchild.js is not rendered at all ''' Even though there is obviously a dependency for it.

Looks like a bug to me.

On a first glance, it looked like a https://core.trac.wordpress.org/ticket/35643 , but I have just downloaded night wp build https://wordpress.org/nightly-builds/wordpress-latest.zip and problem still persists.

Attachments (4)

35873.patch (1.3 KB) - added by ocean90 8 years ago.
35873.2.patch (1.4 KB) - added by ocean90 8 years ago.
35873.3.patch (1.8 KB) - added by ocean90 8 years ago.
35873.4.patch (3.6 KB) - added by ocean90 8 years ago.
Includes tests by @gitlost

Download all attachments as: .zip

Change History (17)

@ocean90
8 years ago

#1 @ocean90
8 years ago

  • Focuses template removed
  • Keywords needs-patch has-unit-tests added
  • Version trunk deleted

Hello Dima, welcome to Trac!

Thanks for your report. What do you think should be the expected output? Load all scripts in header like 35873.patch?

#2 follow-up: @Dima Stefantsov
8 years ago

Hi Dominik. Thanks!

I expect same behaviour as when two scripts queued: wordpress tries to render everything where it was registerd to be, moving footer children to header if parent requires header.

$expected_head  = "<script type='text/javascript' src='http://example.org/grandchild.js?ver=1'></script>\n"; 
$expected_head .= "<script type='text/javascript' src='http://example.org/child.js?ver=1'></script>\n"; 

$expected_footer = "<script type='text/javascript' src='http://example.org/parent.js?ver=1'></script>\n"; 
Last edited 8 years ago by Dima Stefantsov (previous) (diff)

@ocean90
8 years ago

#3 in reply to: ↑ 2 @ocean90
8 years ago

Replying to Dima Stefantsov:

I expect same behaviour as when two scripts queued: wordpress tries to render everything where it was registerd to be, moving footer children to header if parent requires header.

Makes sense, updated test in 35873.2.patch.

#4 @gitlost
8 years ago

See #25247 (which has been bypassed a bit by #35643 and changeset [36550]) where in comment https://core.trac.wordpress.org/ticket/25247#comment:11 it was suggested forcing any dependencies to the lowest common group would fix this issue. I'll upload updated patches there, for old times sake, though it may not now be the best place.

Last edited 8 years ago by gitlost (previous) (diff)

@ocean90
8 years ago

#5 @Dima Stefantsov
8 years ago

Wow, so this is going on for 2 years already!
Having trust that wp_enqueue_script will not silently forget about some of scripts seems pretty important to me. How soon do you think it will be fixed?

#6 @ocean90
8 years ago

@dima-stefantsov Can give 35873.3.patch a try please? Found it in a patch by @gitlost, see ticket:25247:25247_dependencies.diff.

@gitlost FYI: Your latest patch on #25247 seems not to fix this issue.

#7 @Dima Stefantsov
8 years ago

@ocean90 as far as I can see, it works as I expect it after I wrote

$this->group = $group = min($this->group, $group);

from patch 35873.3.patch

Everything works now. Will it appear in next minor WP release, if you confirm it doesn't break any tests?

#8 @ocean90
8 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

Will it appear in next minor WP release, if you confirm it doesn't break any tests?

Minor releases are only to fix regressions. It could land in 4.5 but therefore it needs a few more unit tests since there are currently none around group handling.

#9 @gitlost
8 years ago

@gitlost FYI: Your latest patch on #25247 seems not to fix this issue.

Oh, it seems functionally equivalent to 35873.3.patch, is there a particular test it fails?

Minor releases are only to fix regressions. It could land in 4.5 but therefore it needs a few more unit tests since there are currently none around group handling.

There's more group handling tests in 25247_tests6.diff.

@ocean90
8 years ago

Includes tests by @gitlost

#10 @ocean90
8 years ago

  • Keywords has-patch has-unit-tests needs-testing added; needs-patch needs-unit-tests removed
  • Milestone changed from Awaiting Review to 4.5

Attached 35873.4.patch which includes the tests from #25247, see also ticket:25247:11.

Last edited 8 years ago by ocean90 (previous) (diff)

#11 follow-up: @ocean90
8 years ago

WP_Dependencies::set_group() was introduced in [10357]. @azaozz Any objections to the proposed change in 35873.4.patch?

#12 in reply to: ↑ 11 @azaozz
8 years ago

Replying to ocean90:

35873.4.patch looks good. That code is meant to reduce the "group number" when there is a recursion and the dependency has higher "group number", so dependencies are always printed before the main script.

#13 @ocean90
8 years ago

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

In 36604:

Script Loader: Fix missing script output when the groups of dependencies are different.

Aka: Don't lose the grandchild.

Props gitlost, ocean90.
Fixes #35873.

Note: See TracTickets for help on using tickets.