Include admin-bar script deps in dev mode (e.g. hoverintent-js)#3928
Include admin-bar script deps in dev mode (e.g. hoverintent-js)#3928westonruter merged 5 commits intodevelopfrom
Conversation
|
@schlessera I've revisited the logic for the method used to determine if one dependency is exclusively the dependency of another dependency. The logic is making my brain hurt and my jet lag is not helping. I've added unit tests to help bring some assurance that the logic is correct, but it may need further tweaking. Once this PR is merged we'll cut the 1.4.2-RC1 release. |
|
Note for merger: squash merging cannot be used because 4f0983a will conflict with 1.4 branch. So each other commit should be cherry-picked onto the 1.4 branch individually, with the reference to this PR added to the commit message. |
| } | ||
|
|
||
| /** | ||
| * Check if a handle is exclusively a dependency of another handle. |
There was a problem hiding this comment.
Before reviewing the actual logic, let me try to rephrase here to make sure I understand the intent and we're on the same page here.
The way I understand it is that this method checks whether a dependency is only being pulled in as a child dependency of a given parent dependency and nowhere else (neither as a child dependency of another parent dependency, nor as a main dependency itself).
This is meant to be used to detect dependencies that are either safe to remove when their parent was removed or safe to add to devmode if their parent was added to devmode.
Is the above correct?
There was a problem hiding this comment.
(neither as a child dependency of another parent dependency
I don't think this part is quite right. It should be allowed to a (recursive) dependency of something else, if that thing is also ultimately a dependency of the dependency in question.
This is meant to be used to detect dependencies that are either safe to remove when their parent was removed or safe to add to devmode if their parent was added to devmode.
The latter, yes, whether we can add dev mode to the dependency. We only want to add dev mode to dependences which are ultimately and exclusively connected with the admin bar. So this is what this method is trying to check for.
| */ | ||
| protected static function is_exclusively_dependent( WP_Dependencies $dependencies, $dependency_handle, $dependent_handle ) { | ||
|
|
||
| // If a dependency handle is the same as the dependent handle, then this self-referential relationship is exclusive. |
There was a problem hiding this comment.
I don't think that's the case. If jquery all of a sudden has jquery as a dependency, it wouldn't mean that no-one else can have it as a dependency anymore because of that...
I don't think this allows an early bail here, I think you still need to check whether this is used in other places as well.
There was a problem hiding this comment.
I included this condition as somewhat of an edge case test since if someone has jquery as a dependency of jquery this actually causes WordPress to self-destruct:
PHP Fatal error: Allowed memory size of 1073741824 bytes exhausted (tried to allocate 20480 bytes) in /wp-includes/class.wp-dependencies.php on line 151
This is due to WP_Dependencies::all_deps() not catching infinite loops: https://github.com/WordPress/wordpress-develop/blob/3b25bc43628a3b0c033248aef260581277880a32/src/wp-includes/class.wp-dependencies.php#L132-L133
The code path won't ever be traversed in how this code is used because admin-bar will never a dependency of admin-bar.
So we might as well remove this condition.
| if ( | ||
| self::has_dependency( $dependencies, $queued_handle, $dependency_handle ) | ||
| && | ||
| ! self::has_dependency( $dependencies, $queued_handle, $dependent_handle ) |
There was a problem hiding this comment.
I don't think this second condition is entirely correct, it is merely an assumption and cannot be asserted.
As an example, if something used the admin bar dependency to integrate with the admin bar, and uses dashicons at the same time, there's no direct reason to assume that the dashicons will only be used in the context of this admin bar integration. This could be totally unrelated, couldn't it?
There was a problem hiding this comment.
If this check is not here then the admin_bar_scripts_have_dev_mode dataset test fails with:
Expected dev mode to be enabled on element for query:
//script[ contains( @src, "/hoverintent-js" ) ]
The hoverintent-js asset should NOT get dev mode if:
- There is a queued handle that depends on it, and
- This queued handle does not ultimately depend on admin bar.
The second point corresponds to this second condition above.
| wp_enqueue_style( 'special-icons', 'https://example.com/special-icons.css', [ 'dashicons', 'admin-bar' ], '0.1' ); | ||
| }, | ||
| function ( DOMXPath $xpath ) { | ||
| $element = $xpath->query( '//link[ @id = "dashicons-css" ]' )->item( 0 ); |
There was a problem hiding this comment.
Should probably use $this->assert_dev_mode_is_on_queried_element as well here.
| }, | ||
| ], | ||
|
|
||
| 'dashicons_in_dev_mode_because_all_dependents_depend_on_admin_bar' => [ |
There was a problem hiding this comment.
This is the case that I think is debatable. I don't think that having both 'dashicons' and 'admin-bar' as dependencies warrants the conclusion that dashicons is not used outside the admin bar.
There was a problem hiding this comment.
I'm not sure. The special-icons style here depends on admin-bar so if you enqueue it then you'd also get the admin-bar. In other words, it's true that perhaps some of the icons are being used outside the admin bar area, but perhaps this is because of some inline editing features. This is good because those special icons are needed by that authenticated user in dev mode. So if the user is logged-out, and the admin bar is not being included, then these should not be getting enqueued.
|
@pierlon There is a test failure in WordPress 4.9 due to the amp-paired-browsing-client not being printed: https://travis-ci.org/ampproject/amp-wp/jobs/625330248#L1062-L1068 I believe this is due to the fact that Lines 261 to 263 in 80e70e3 In other words, there is no polyfill for the polyfills. Three options I see here:
I've gone ahead and removed the test case in 6c19d91, but this will need to be fixed for v1.5 in a separate PR. That PR can revert this commit to re-add those tests. Issue opened: #3932 |
…not pass in AMP 1.4
|
|
de90b0d to
64a8051
Compare
|
Build for testing: amp.zip (1.5.0-alpha-20191216T000003Z-64a80518) |
…not pass in AMP 1.4 (#3928)
* tag '1.4.2': (25 commits) Bump 1.4.2 Bump 'tested up to' to 5.3.2 Catch unfiltered requests when testing Crowsignal embed (#3956) Bump version 1.4.2-RC1 Bump 'Tested up to' to 5.3.1 Remove test case for paired browsing since broken in WP4.9 and would not pass in AMP 1.4 (#3928) Further refine is_exclusively_dependent and add tests (#3928) Only include hoverintent-js in dev mode if exclusive dependency of admin-bar (#3928) Include admin-bar script deps in dev mode (e.g. hoverintent-js) (#3928) Fix tests after security fix in WP 5.3.1 (r46896) (#3926) Add E2E tests to allow_failures Convert `theme_features` variable into `get_theme_config` function Only apply smooth scroll fix on 'Twenty Twenty' <= 1.0.0 Re-add smooth scrolling fix on WP < 5.3.1 No need to apply smooth scrolling fix for Twenty Twenty theme Bump stylesheet cache group after #3866 (#3880) Return allowed KSES tags if not an array (#3879) Bundle common font files for externalizing data: URLs (#3866) Pull the built `block-libray` package from Gutenberg SVN if it does not exists (#3847) Optimize is_amp_allowed_attribute checking for reference points (#3815) ...
Summary
Fixes #3927.
Before:
After:
Checklist