Make WordPress Core

#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 13 months ago.
Patch for the issue.
wordpress-60331-02.patch (2.2 KB) - added by vladimiraus 13 months ago.
Additional comment change.
wordpress-60331-03.patch (3.7 KB) - added by vladimiraus 13 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 13 months ago.
Updated spacing.

Download all attachments as: .zip

Change History (11)

@vladimiraus
13 months ago

Patch for the issue.

#1 @vladimiraus
13 months ago

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

Needs review...

@vladimiraus
13 months ago

Additional comment change.

#2 @westonruter
13 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
13 months ago

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

@vladimiraus
13 months ago

Updated spacing.

#3 @westonruter
13 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
13 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
13 months ago

Ah, yes, sure!

#6 @vladimiraus
13 months ago

👍 Thank you!

#7 @westonruter
13 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.