Make WordPress Core

Opened 3 months ago

Closed 3 months ago

#60331 closed defect (bug) (fixed)

Clarify in phpdoc that wp_print_inline_script_tag() and wp_get_inline_script_tag() are not only for JavaScript

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Script Loader Keywords: good-first-bug has-patch needs-testing
Focuses: Cc:

Description

In #56313 the wp_print_inline_script_tag() function is used to output the importmap script for script modules. The content of an importmap script is JSON, and not JavaScript. This is indicated by passing a type argument to wp_print_inline_script_tag():

<?php
wp_print_inline_script_tag(
        wp_json_encode( $import_map, JSON_HEX_TAG | JSON_HEX_AMP ),
        array(
                'type' => 'importmap',
                'id'   => 'wp-importmap',
        )
);

Nevertheless, in the phpdoc for wp_print_inline_script_tag() and wp_get_inline_script_tag() it is specifically mentioning "JavaScript" and $javascript. These should be made less-specific to JavaScript since the function can be used to print any script. For example, the Speculation Rules plugin uses it to print the speculationrules script.

This is split out from a PR discussion in #60320.

Attachments (4)

wordpress-60331.patch (1.9 KB) - added by vladimiraus 3 months ago.
Patch for the issue.
wordpress-60331-02.patch (2.2 KB) - added by vladimiraus 3 months ago.
Additional comment change.
wordpress-60331-03.patch (3.7 KB) - added by vladimiraus 3 months ago.
Thanks for the feedback @westonruter. All addressed plus found some extra variables that were not replaced.
wordpress-60331-04.patch (3.6 KB) - added by vladimiraus 3 months ago.
Updated spacing.

Download all attachments as: .zip

Change History (11)

@vladimiraus
3 months ago

Patch for the issue.

#1 @vladimiraus
3 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Needs review...

@vladimiraus
3 months ago

Additional comment change.

#2 @westonruter
3 months ago

@vladimiraus Thanks for the patch!

For wp_get_inline_script_tag(), instead of "Wraps inline script in <script> tag" what about:

Constructs an inline script tag.

And for wp_print_inline_script_tag():

Prints an inline script tag.

These would seem less redundant and simpler.

Also, for the first argument, instead of $script what about $text or $data? This would map to script.text (spec) or script.firstChild.data in the DOM.

@vladimiraus
3 months ago

Thanks for the feedback @westonruter. All addressed plus found some extra variables that were not replaced.

@vladimiraus
3 months ago

Updated spacing.

#3 @westonruter
3 months ago

@vladimiraus Thanks! By the way, did you know you can contribute patches via GitHub pull requests instead of attaching patch files? All you have to do is fork the wordpress-develop repo, make your changes, and then open a pull request with a link to this Trac ticket in the PR description. This makes it much easier to review and collaborate, and it will run all of the automated tests. Would you be willing to do that?

Here's the full instructions: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/#in-github

#4 @vladimiraus
3 months ago

@westonruter
Yep. Wanted to do it for the next patch. 😃
I presented "First time contribution" at the latest meetup and wanted to show one issue with patches and another with merge requests.
Are you happy to commit this one via patches?

#5 @westonruter
3 months ago

Ah, yes, sure!

#6 @vladimiraus
3 months ago

👍 Thank you!

#7 @westonruter
3 months ago

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

In 57367:

Script Loader: Clarify in docs that wp_get_inline_script_tag() and wp_print_inline_script_tag() can take non-JS data.

Props vladimiraus.
Fixes #60331.

Note: See TracTickets for help on using tickets.