Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Track assets those registered or enqueued in block's render callback. #6579

Merged
merged 11 commits into from
Sep 22, 2021
Merged

Track assets those registered or enqueued in block's render callback. #6579

merged 11 commits into from
Sep 22, 2021

Conversation

dhaval-parekh
Copy link
Collaborator

@dhaval-parekh dhaval-parekh commented Aug 30, 2021 •
edited by westonruter
Loading

Summary

Add wrapper callback to detect/track specific function which registered or enqueued asset in block's render callback. instead of showing do_blocks() as a source of an enqueueing asset.

Fixes #5411
Fixes #6231

Plugin Before After
Without Gutenberg image image
With Gutenberg image image

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@dhaval-parekh dhaval-parekh requested a review from pierlon August 30, 2021 13:49
@dhaval-parekh dhaval-parekh changed the title Track asset registered/enqueued in block render callback. Track assets that registered or enqueued in block's render callback. Aug 30, 2021
@dhaval-parekh dhaval-parekh changed the title Track assets that registered or enqueued in block's render callback. Track assets those registered or enqueued in block's render callback. Aug 30, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2021 •
edited
Loading

Plugin builds for 9e3a620 are ready 🛎️!

@westonruter
Copy link
Member

In regards to #6231, I'll be preventing the comment-reply.js script from showing up on the page in the first place as part of #6577.

@westonruter westonruter added this to the v2.2 milestone Sep 10, 2021
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into bug/5411-…
fdcbc87 
…incorrect-error-source

* 'develop' of github.com:ampproject/amp-wp: (613 commits)
  Bump svgo from 2.6.0 to 2.6.1
  Advise to check loopback request test
  Simplify language for condition for what headers are looked for
  Bump eslint-plugin-jest from 24.4.0 to 24.4.2
  Improve test coverage for get_persistent_object_cache_learn_more_action
  Remove absolete wp_doing_ajax filter
  Eliminate use of deprecated assertion
  Store results of page caching to reduce severity of missing a persistent object cache
  Explain how page cache detection is performed
  Improve tests for page caching
  fixup! Eliminate attempting to challenge page caching in favor of response header detection
  Revert "Increase timeout for selector"
  Eliminate attempting to challenge page caching in favor of response header detection
  Only verify save button is disabled after being clicked
  Require future expires header for client-side caching
  Require Cache-Control to have max-age to satisfy client-caching
  Add optional SSL-verification, Basic auth forwarding, and request failure handling
  Remove extra margin from top of Site Health tests
  Fix test failure in older versions of WP
  Refactor to improve test coverage
  ...
@westonruter
Copy link
Member

Just a note to self when testing: there are two scenarios in which a block may enqueue a script or a style.

  1. In the block's render_callback.
  2. When a block type is registered with a style or script handle arg.

It seems this PR addressing the first case, but we should make sure the second case is also addressed.

@westonruter
Copy link
Member

When using the Bad Block Plugin, I'm seeing:

Before After
image image

The only validation error that isn't properly attributed to Bad Block is this inline script:

image

That's the inline script that is being returned in the render_callback. Interestingly, if I deactivate Gutenberg then it shows as coming from the Bad Block block (but not specifically the Bad Block plugin):

image

@westonruter
Copy link
Member

The reason why the block is getting identified as coming from Gutenberg is due to this compat code in gutenberg_inject_default_block_context() which wraps the render_callback in a closure.

@westonruter
Copy link
Member

There we go. Now it's working as expected.

Plugin Before After
Without Gutenberg image image
With Gutenberg image image

@westonruter
Copy link
Member

There is one area for future improvement and that is to try to link validation errors for scripts/styles in the head/footer to blocks that caused them to be added to the page. For example, with the Bad Block plugin, only the one validation error coming from the block content is getting reported as being from the block:

image

When in reality, all 5 are being added to the page because that one block was added:

image

But this can be done in a subsequent PR.

@westonruter
Copy link
Member

@pierlon Please add your review when you get a chance.

The changes here are key for Site Scanning to produce accurate results.

pierlon
pierlon approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@westonruter westonruter merged commit a4b34a4 into develop Sep 22, 2021
@westonruter westonruter deleted the bug/5411-incorrect-error-source branch September 22, 2021 05:31
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 14, 2021
@westonruter westonruter mentioned this pull request Dec 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierlon pierlon pierlon approved these changes

@westonruter westonruter westonruter approved these changes

Assignees
No one assigned
Labels
Changelogged Whether the issue/PR has been added to release notes.
Projects
None yet
Milestone
v2.2
3 participants
@dhaval-parekh @westonruter @pierlon