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.

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

Fix tinymce not defined #4731

Closed
wants to merge 2 commits into from

Conversation

joemcgill
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/58648


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@joemcgill joemcgill changed the title [DRAFT] Fix tinymce not defined Fix tinymce not defined Jun 27, 2023
@joemcgill joemcgill marked this pull request as ready for review June 27, 2023 23:33
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @joemcgill, LGTM!

return '';
}

// If the intended strategy is 'defer', limit the initial list of eligibles.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If the intended strategy is 'defer', limit the initial list of eligibles.
// If the intended strategy is 'defer', limit the initial list of eligibles since 'async' can fallback to 'defer' but not vice-versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved in edd9347.


$actual = get_echo( 'wp_print_scripts', array( array( 'wp-tinymce' ) ) );

$this->assertStringNotContainsString( 'async', $actual );
Copy link
Member

Choose a reason for hiding this comment

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

For good measure perhaps:

Suggested change
$this->assertStringNotContainsString( 'async', $actual );
$this->assertStringNotContainsString( 'async', $actual );
$this->assertStringNotContainsString( 'defer', $actual );

Copy link
Member

Choose a reason for hiding this comment

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

If we add the the suggested, reminder to add messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Handled in 3d1a245.

Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

LGTM, a couple of non-blocking nits in there.

src/wp-includes/class-wp-scripts.php Show resolved Hide resolved
*
* @ticket 58648
*/
public function test_printing_tinymce_scripts() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's covered elsewhere but a similar test for scripts registered with a loading strategy but never have wp_enqueue_script() called might be helpful.

might being the keyword. This isn't a passive agressive change request -- could be added later so this can be committed pre beta.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion. I've added one in d3835ba.

@ockham
Copy link
Contributor

ockham commented Jun 28, 2023

Bit of a long shot, but I think @tyxla recently worked on TinyMCE. Marin, is this code something you're familiar with? If so, could you weigh in here? 😊

@tyxla
Copy link
Member

tyxla commented Jun 28, 2023

Bit of a long shot, but I think @tyxla recently worked on TinyMCE. Marin, is this code something you're familiar with? If so, could you weigh in here? 😊

Thanks for the ping!

From what I can see this isn't related to my experimentation on the Gutenberg side; it's rather related to the recent introduction of the async and defer loading strategy in https://core.trac.wordpress.org/ticket/12009.

The proposed fix appears to be spot on 👍

@azaozz
Copy link
Contributor

azaozz commented Jun 28, 2023

Right, the bug was that async was added to scripts that were "printed" directly without being enqueued first. The fix works well and was committed in https://core.trac.wordpress.org/changeset/56092.

Not closing this PR as there are few suggestions that can be added later.

@joemcgill joemcgill mentioned this pull request Jul 14, 2023
@joemcgill
Copy link
Member Author

Moved followup tasks to a different PR to clarify what would be committed separately.

@joemcgill joemcgill closed this Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants