WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#9798 closed enhancement (wontfix)

TinyMCE Spellchecker not working behind firewall (google spellcheck)

Reported by: bforchhammer Owned by: azaozz
Milestone: Priority: normal
Severity: normal Version: 2.8
Component: TinyMCE Keywords: has-patch
Focuses: Cc:

Description

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...

Attachments (7)

9798.wp_googlespell.patch (3.1 KB) - added by bforchhammer 6 years ago.
9798.wp_googlespell-2.patch (6.0 KB) - added by bforchhammer 6 years ago.
9798.wp_googlespell-3.patch (5.7 KB) - added by bforchhammer 6 years ago.
similiar_text() instead of list of miss-spellings and use of wordpress function "unhtmlentities"
9798.wp_googlespell-4.patch (5.8 KB) - added by bforchhammer 6 years ago.
removed port 443 from url (assuming that transport classes can figure this out themselves); also updated some documentation and some function names
wp-spellcheck.zip (7.4 KB) - added by bforchhammer 6 years ago.
Plugin version (wp-spellcheck)
spellchecker.patch (14.3 KB) - added by bforchhammer 6 years ago.
spellcheck.patch (14.1 KB) - added by bforchhammer 6 years ago.
Updated patch against current HEAD

Download all attachments as: .zip

Change History (47)

comment:1 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to Future Release

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

comment:2 @bforchhammer6 years ago

  • Keywords changed from needs-patch proxy, spellchecker to needs-patch proxy spellchecker

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.

comment:3 @Denis-de-Bernardy6 years ago

they could make their own function overridable.

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

comment:4 @bforchhammer6 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

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.

comment:5 @Denis-de-Bernardy6 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Future Release to 2.8

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? :-)

comment:6 @bforchhammer6 years ago

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.

comment:7 @Denis-de-Bernardy6 years ago

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.

comment:8 @Denis-de-Bernardy6 years ago

oh, and that one filter seems right, yes.

comment:9 @bforchhammer6 years ago

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".

comment:10 @Denis-de-Bernardy6 years ago

very true.

your default miss-spellings seem right.

@bforchhammer6 years ago

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

comment:11 @bforchhammer6 years ago

  • Keywords 2nd-opinion needs-testing added
  • 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.

comment:12 @Denis-de-Bernardy6 years ago

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

comment:13 @Denis-de-Bernardy6 years ago

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

@bforchhammer6 years ago

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

comment:14 @bforchhammer6 years ago

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.

comment:15 @Denis-de-Bernardy6 years ago

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.

comment:16 @Denis-de-Bernardy6 years ago

  • Keywords 2nd-opinion removed

comment:17 @pbearne6 years ago

  • Keywords tested added; needs-testing removed
  • Priority changed from normal to high

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

comment:18 follow-up: @Denis-de-Bernardy6 years ago

Super...

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

comment:19 in reply to: ↑ 18 @bforchhammer6 years ago

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!

comment:20 @ryan6 years ago

  • Priority changed from high to normal

comment:21 @azaozz6 years ago

  • Keywords needs-testing added; tested proxy spellchecker removed
  • Milestone changed from 2.8 to Future Release

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.

comment:22 @bforchhammer6 years ago

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!

comment:23 follow-up: @azaozz6 years ago

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

comment:24 in reply to: ↑ 23 ; follow-up: @Denis-de-Bernardy6 years ago

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.

comment:25 in reply to: ↑ 24 @azaozz6 years ago

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.

comment:26 @bforchhammer6 years ago

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...

comment:27 @bforchhammer6 years ago

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

comment:28 @Denis-de-Bernardy6 years ago

  • Keywords dev-feedback added; needs-testing removed

comment:29 @bforchhammer6 years ago

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.

@bforchhammer6 years ago

Plugin version (wp-spellcheck)

comment:30 @bforchhammer6 years ago

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

@bforchhammer6 years ago

comment:31 @bforchhammer6 years ago

  • Keywords needs-testing added

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.

comment:32 @bforchhammer6 years ago

  • Milestone changed from Future Release to 2.8.1

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

comment:33 @azaozz6 years ago

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.

comment:34 @Denis-de-Bernardy6 years ago

  • Milestone changed from 2.8.1 to 2.9

enhancement

comment:35 @Denis-de-Bernardy6 years ago

  • Keywords needs-review added; needs-testing dev-feedback removed

@bforchhammer6 years ago

Updated patch against current HEAD

comment:36 @bforchhammer6 years ago

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.

comment:37 @pbearne6 years ago

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.

comment:38 @azaozz5 years ago

  • Keywords needs-review removed
  • Milestone changed from 2.9 to Future Release

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?

comment:39 @bforchhammer5 years ago

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

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

comment:40 @Denis-de-Bernardy5 years ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.