TinyMCE Spellchecker not working behind firewall (google spellcheck)

[bforchhammer]

Hi,

the TinyMCE spellchecker (that is the default google spellchecker) is not working for wordpress setups behind a firewall.

Wordpress is going to have Proxy support in core in 2.8 (see ​#4011).

Is it possible to hack the tinymce spellchecker so it uses wordpress' new http handling functions? Then the spellchecker could use the same config and connect via proxy as well...

[Denis-de-Bernardy]

shouldn't this be dealt with upstream, over at Moxiecode?

[bforchhammer]

Possibly, but they would not use wordpress' WP_Http class, which means you would probably end up with another set of proxy configuration options in the tinymce subfolder!? (That is if the current way tinymce is set up stays the same).

If people start using proxy support in 2.8 there's a high probability that they will run into this issue.

[Denis-de-Bernardy]

they could make their own function overridable.

either way, the keyword ultimately is needs-patch, not debate. :-)

[bforchhammer]

Here's a first patch with a possible solution.
I've created a new class WP_GoogleSpell which extends TinyMCE's GoogleSpell class and overrides the http request bits with code using the WP_Http class.

[Denis-de-Bernardy]

Big +1 to the approach.

One small tweak I'd suggest: dirname(dirname(...dirname(FILE)...)) rather than the ../../.., as some servers don't behave well with relative include paths.

Also, I took a quick glance at the original class and:

​http://wiki.moxiecode.com/index.php/TinyMCE:Plugins/spellchecker#Spellchecker_PHP_options

On the one hand side, I opened #9805, which presumably works since nobody is reporting spell checker issues. But the patch would need to be rewritten in such a way that these regex (and the one in the Spellchecker class) no longer use the /e modifier (see #8689).

On the other side, the jargon option caught my attention. Could you look into bundling a fix to #7810 while we're at it? :-)

[bforchhammer]

Second patch

• dirname instead of '../../' on require
• overloaded function _unhtmlentities and removed 'e' option from preg_replace calls
• added feature to allow "spelling exceptions"

I am not sure if this is the correct way of handling "suggestions". Do we have to maintain a list of possible "miss-spellings" and map it to the correctly spelled words, or is there a better way of doing this?

Also is one filter for the exception list appropriate? How should it be named (any wp convention that I should follow)?

Documentation isn't complete either.

[Denis-de-Bernardy]

mm... this won't work:

preg_replace('~&#x([0-9a-f]+);~i', 'chr(hexdec("\\1"))', $string);

the preg_replace with /e and the replacement chunk actually needs to be replaced by a preg_replace_callback that does a similar thing (e.g. r11098)

I've no idea of how the spellcheck exceptions work with GoogleSpell, but what you did "feels" right at first glance.

[Denis-de-Bernardy]

oh, and that one filter seems right, yes.

[bforchhammer]

Hm, I should've read #8689 more carefully... I think ​wp_kses_decode_entities() does the same as the first two lines of the _unhtmlentities() function so we can possible just call that function. (same for ​#9805; if #9805 gets implemented we should probably call that function instead).

We also need a list of default "miss-spellings" of "WordPress".

[Denis-de-Bernardy]

very true.

your default miss-spellings seem right.

[bforchhammer]

similiar_text() instead of list of miss-spellings and use of wordpress function "unhtmlentities"

[bforchhammer]

• I disabled sslverify on the http connection becasue it tells me that the verification fails on my test server. Should this be active?
• Changed it to use similiar_text to compare a given word with the list of possible suggestions; this way we don't have to maintain a list of miss-spellings. (which seemed kind of ugly in the first place).
• The similiarity treshold is set to 80% at the moment.

[Denis-de-Bernardy]

With the current patch, I get: "Error response: Protocol https not supported or disabled in libcurl{"id":null,"result":[],"error":null}"

[Denis-de-Bernardy]

without it, things work. so we're probably missing a few options in the wp_remote_request() calls.

[bforchhammer]

removed port 443 from url (assuming that transport classes can figure this out themselves); also updated some documentation and some function names

[bforchhammer]

I've removed the port number from the url, maybe it works now? Or could it be a server setting thing? The Wp_Http_Curl class sets a whole bunch more options than the TinyMCE version... I don't really have any idea why it does not work for you - it works fine on my test server.

[Denis-de-Bernardy]

Mm... sorry about that. It was actually related to my laptop. Curl is not configured correctly. I added a quick note in the IRC channel to get some extra testers on this ticket.

[pbearne]

Hi I have given this a quick test on our 2.8 checkout and I can now miss spell worppress and the Google spell worded through fire wall

[Denis-de-Bernardy]

Super...

so, we should change the WP check to something like. if strtolower(word) = wordpress then ignore. and then we're good to go.

[bforchhammer]

Replying to Denis-de-Bernardy:

so, we should change the WP check to something like. if strtolower(word) = wordpress then ignore. and then we're good to go.

Not sure what you mean... are you saying we should add the lowercase version of "WordPress" to the list of good words? If so I disagree; WordPress is a "name" and as such should be spelled in the right case!

[azaozz]

IMHO this is on the border of being a plugin material. If we are adding it, the class should probably go in wp-admin/includes. We can change the location in the TinyMCE config to load it from there.

[bforchhammer]

I think at least the "proxy support" bit should definitely go into 2.8 in some form! I repeat my argument from above:

If people start using proxy support in 2.8 there's a high probability that they will run into this issue.

Also, as far as I can see the TinyMCE config file only allows to set a "general.engine" parameter which is used for both including the class-file as well as instantiating the class.

​general.php, l. 18-19: this is where class file gets included.

if (isset($config['general.engine']))
	require_once(dirname(__FILE__) . "/../classes/" . $config["general.engine"] . ".php");

​rpc.php l. 96-99: this is where the SpellChecker object is created.

if (isset($config['general.engine'])) {
	$spellchecker = new $config['general.engine']($config);
	$result = call_user_func_array(array($spellchecker, $input['method']), $input['params']);
} 

I am not sure there's a way to use a class from a different location - without changing any TinyMCE files. Please correct me if I'm wrong. I am open for suggestions!

[azaozz]

I meant the main js config for TinyMCE where the spellchecker plugin has some options: ​http://wiki.moxiecode.com/index.php/TinyMCE:Plugins/spellchecker

[Denis-de-Bernardy]

Replying to azaozz:

I meant the main js config for TinyMCE where the spellchecker plugin has some options: ​http://wiki.moxiecode.com/index.php/TinyMCE:Plugins/spellchecker

Indeed, but the main point he's raising is that it should be using WP_HTTP rather than TinyMCE's built-in transport. And he's definitely right on this front.

[azaozz]

Replying to Denis-de-Bernardy:
Exactly, we can point the js part of the spellchecker to any back-end that may or may not use the rest of the files there. TinyMCE includes only the js part by default, the rest is separate.

[bforchhammer]

azaozz: Okay, I see what you mean now. We could change the js config and point [spellchecker_rpc_url] to a custom version of ​rpc.php; I guess that makes sense but it means that we're going to have replicate a fair bit of the code in that file...

If this is the way to go I'm happy to implement a solution.

Hmm, done right this could allow plugins to provide custom Spellchecker modules and implement things like the word exceptions...

[bforchhammer]

Hm, so I guess this could completely be implemented in a plugin... question is: should it be in the core?

[bforchhammer]

I've started to work on a ​plugin implementation..

Once the plugin is stable parts of it can possibly be merged into wordpress core. Until then I think this is a good way to gather more ideas, get some feedback and have it tested by a few more people.

[bforchhammer]

Plugin version (wp-spellcheck)

[bforchhammer]

I've released the first version of the plugin. Check out the ​source code here.

[bforchhammer]

I've attached a huge patch with the important bits from the plugin. This needs some testing and comments.

Not sure if I put everything into the right places and it is a fairly big patch; If people prefer we could split it into two bits: the spellchecking api and the proxy-enabled googlespell replacement.

[bforchhammer]

Let's see if we can get this into 2.8.1... :-)

[azaozz]

Few thoughts about the core patch:

• we need to include the classes only when doing spell checking, no point including them on each page load,

• better to use admin_url() and includes_url() as they get the right protocol (http/https),

• can probably add another case in admin-ajax.php to include all needed files and instantiate the classes, etc.

[Denis-de-Bernardy]

enhancement

[bforchhammer]

Updated patch against current HEAD

[bforchhammer]

We have been using ​the plugin for about two months now on our wpmu platform without discovering any problems... Is there anything that could prevent this from being commited to the core? If not I'd like to suggest that this gets added and we fix any bugs and other issues on separate tickets.

[pbearne]

when this is fixed can we make sure that the spellchacker can be called when not logged in so we can use TinyMCE in themes etc.

[azaozz]

This has been available as a plugin for few months but doesn't seem that many people need it. Perhaps it's better as a plugin?

[bforchhammer]

Agreed. I guess we can mark this as wontfix and just upgrade the plug-in if something comes up and changes become necessary...