WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 years ago

Last modified 6 years ago

#19906 closed enhancement (wontfix)

Quicktags docs recommend code causing JS error: Uncaught ReferenceError: QTags is not defined

Reported by: jeremyclarke Owned by:
Milestone: Priority: normal
Severity: trivial Version: 3.3.1
Component: Inline Docs Keywords: has-patch needs-codex
Focuses: Cc:
PR Number:

Description

Right now the docs for qt.addbutton in quicktags.dev.js have the following text:

	 * If you are echoing JS directly from PHP,
	 * use add_action( 'admin_print_footer_scripts', 'output_my_js', 100 ) or add_action( 'wp_footer', 'output_my_js', 100 )

This works, but if you add the QTags.addButton() calls on those hooks then they will cause JS errors on any screen without an editor:

Uncaught ReferenceError: QTags is not defined

The issue is avoided if you use the other option, enqueuing a script dependent on 'quicktags', but that is a lot more work and forces all pages in the admin to load the quicktags script unnecessarily.

Maybe there is some way to magically make calling QTags.addbutton() safe no matter what, but I think at minimum we need to add a note to the PHPdoc about checking QTags before using it, as that solves the problem pretty simply:

 if ( typeof QTags != 'undefined' ) {
	QTags.addButton( 'gv_translation', 'translation', '<div class="translation">', '</div>' );
}

So the phodoc could say :

	 * If you are echoing JS directly from PHP use
	 *	add_action( 'admin_print_footer_scripts', 'output_my_js', 100 ) 
	 * or 
	 *	add_action( 'wp_footer', 'output_my_js', 100 )
	 *	
	 * If echoing the addButton calls directly instead of enqueing with the 'quicktags' dependency
	 * make sure to check that the QTags object is defined first, otherwise your code will throw errors 
	 * when no editor is present:
	 *	if ( typeof QTags != 'undefined' ) { QTags.addButton(...) } 
	 *

The attached patch just adds that to the docs. If a committer has a preferred wording then just use that instead, obviously the patch didn't take me long :)

BTW: I created a Codex article with the docblock after not finding a reference to the new API anywhere other than in the JS file itself. Nacin's posts mentioned that they had changed but not what was new ;)

http://codex.wordpress.org/Quicktags_API

Clearly it needs work, I didn't want to put too much time into it at first because I didn't understand the system yet (as I'm learning now, by finding JS errors).

Attachments (1)

19762-quicktags-docblock.diff (1.3 KB) - added by jeremyclarke 8 years ago.
update qt.addButton() dockblock in quicktags.dev.js

Download all attachments as: .zip

Change History (6)

@jeremyclarke
8 years ago

update qt.addButton() dockblock in quicktags.dev.js

#1 in reply to: ↑ description ; follow-up: @TobiasBg
8 years ago

  • Resolution set to invalid
  • Status changed from new to closed

Hi Jeremy,

the solution is much easier: You'll just need to enqueue the "quicktags" script before you add your footer print action. There's no need to enqueue a script that *depends*, you just enqueue "quicktags" directly.

wp_enqueue_script( 'quicktags' );

This works because "output_my_js" is hooked with a priority of 100, so this will be done after all the footer scripts (including "quicktags" then) will have been included.

And about this:

The issue is avoided if you use the other option, enqueuing a script dependent on 'quicktags', but that is a lot more work and forces all pages in the admin to load the quicktags script unnecessarily.

The script would only be loaded on all pages if you don't use the correct hooks. Script enqueuing should only be done when you know that you are on your plugin's admin screen, e.g. in "load-{$plugin_hook}".

#2 in reply to: ↑ 1 ; follow-up: @jeremyclarke
8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Hi Tobias, thanks for replying, but I don't think we should close this ticket. Whether your solution works or not, I think there is still a need for clarification in the inline docs because right now it encourages something that throws a javascript error which causes all kinds of havoc.

We should decide on the clearest solution and refer to it in the docs and on the wiki.

the solution is much easier: You'll just need to enqueue the "quicktags" script before you add your footer print action. There's no need to enqueue a script that *depends*, you just enqueue "quicktags" directly.

wp_enqueue_script( 'quicktags' );

This solution involves using two hooked functions instead of just one right? That's how I got it working but not really how you made it sound. Two hooked functions on the same hook seems unnecessarily complicated to me, though I can see how it is simpler for people who don't know JS at all and just want to use standard WP-PHP. Notably though, this method doesn't seem to work if you use the admin_print_footer_scripts instead of wp_footer, so if this is the official solution we need to be clear in the docs that you must use wp_footer.

The script would only be loaded on all pages if you don't use the correct hooks. Script enqueuing should only be done when you know that you are on your plugin's admin screen, e.g. in "load-{$plugin_hook}".

The quicktags are intended for anything with wp_editor(), not for a specific plugin screen, so we can't assume that people will know what screen they want to add it to. In theory we could tell people all the different screens/conditionals that would imply the presence of an editor, but with the increasing proliferation of wp_editor() the buttons might be needed just about anywhere. If we go the route of telling people to enqueue quicktags we will pretty much have to tell them to enqueue it everywhere just in case, which is a waste IMHO.

It seems like a good solution would be to just define the buttons all the time as long as it doesn't break anything when there's no editor, they don't take up much space after all. Checking if QTags is defined is a simple way to neutralize the javascript without having to worry about every possible scenario where it would be needed.

Is there some way to know if wp_editor() was called during the current pageload? If we could know that with PHP then that would be the ideal solution, as it would truly be just one line of code in the admin_print_footer_scripts action to determine whether to output the button definitions or not. Alternately a footer hook that only fires if the editor was used during the current page, though that seems more convoluted than necessary.

#3 in reply to: ↑ 2 @TobiasBg
8 years ago

clarification in the inline docs because right now it encourages something that throws a javascript error which causes all kinds of havoc.

You could interpret it this way, but if you do, this basically is valid for all documentation in WP. You would actually have to add a similar warning to the docs of many other JS objects or even PHP functions that are in core. IMHO, that's "over-documenting".
(And as you mentioned the wiki: This would probably be the more appropriate place for this hint/warning.)

So, to correct way simply is to load your JS only on pages where you know that the prerequisites (here for quicktags) are fulfilled: For core, this would be post.php and post-new.php (for post types that have support for the editor), and on your own plugin's screens, this would be all screens where you actually load a wp_editor() yourself.

Now, as your intention seems to be to add a button to *all* wp_editors() that there are (which I'm not sure is good thing to do), a typeof undefined check would indeed be in order, and I fully support and encourage it. I just doubt that it has to be recommended in the docs, as most plugins won't need it, if they load their scripts in the proper way, and only for certain wp_editor() instances.

#4 @c3mdigital
6 years ago

  • Keywords close added; has-patch dev-feedback removed
  • Resolution set to wontfix
  • Status changed from reopened to closed

The proper way to enqueue or add inline javascript via 'admin_print_footer_scripts' is to do check early in the function so that your scrip is only output on the pages intended.

function output_my_js() {
    global $pagenow;
   if ( ! in_array( $pagenow, array( 'post.php', 'post-new.php' ) )
       return;

    // your js

}

I don't think we need the recommendation inline but it would be a nice codex example.

#5 @SergeyBiryukov
6 years ago

  • Keywords has-patch added; close removed
  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.