CKEditor should pass paths to locale not URLs

The errors clearly show that it's locale.module making these failed requests, not the CKEditor module.

This may be two different issues caused by the same problem. The 444 no response seems to be due to the file not being there while the PHP error being unable to read that file on the PHP side may also be due to that. Or the file is there but not set to be readable by the PHP / webserver processes.

I don't think this has anything to do with language per say. The file is attempted to be read but fail both via the browser/server and in PHP directly.

Duh, all right, I misread this. So locale module handles internal URLs and parses out the path only. I have no idea at this point how "external" (ie. http://..../) URls end up being created on your site.

What CKEditor module does is it calls https://api.drupal.org/api/drupal/core%21includes%21file.inc/function/fi... to create the URL to the file and then passes into locale. However, file_create_url() seems to now generate external URLs all the time(?)

If drupal_parse_url() encounters an external URL, it does return the full 'path' including the scheme and domain. So not sure why is this not happening more widely (on every D8 site)?

Strange things are going on here!

I cannot reproduce this: I'm running Drupal on port 8082 and when I run \Drupal\ckeditor\Tests\CKEditorTest (whose testJSTranslation() method tests precisely this), I get zero exceptions.

So there is ""nothing wrong"" (in double double quotes :D) with the code. CKEditor's getJSSettings() passes the library paths through array_map('file_create_url', ...) which makes each file have a full URL with a scheme. Then passes this into locale. Locale uses drupal_parse_url() (in _locale_parse_js()) which leaves the scheme/domain/port alone and returns the full URL up to the end of the path. That is practically leaving the URL as-is it was passed. This *always happens* with locale and ckeditor (on the first time the file is parsed).

So PHP ends up loading the files over HTTP. This is probably *very* slow, at least much slower on the initial load than loading from the local filesystem. This only happens on the first instance of the files being found, however it depends on the exact URL of the files passed, so if the site runs on different domains or ports, it will parse it for each again and again.

The solution is to pass in just the paths to the files, not full URLs. Like others do :)

Not sure how if at all we can write tests for this. It works either way if the webserver serves the file which worked before with the tests just as much as after :)

@WimLeers: and ideas as to how we could test this? It looks like a pretty important issue to fix for CKEditor performance at the least.

I think the only way to test this is by converting CKEditorTest::testJSTranslation() to a PHPUnit test. Which is impossible.

The next best thing is to make sure that locale_js_translate() does not ever download external files. In other words: if it would check the scheme on a given file URL, and if that is a non-local scheme, then it should fail. The CDN module also has a check for this:

$scheme = file_uri_scheme($original_uri);

// Only alter URLs for local stream wrappers. If a file is served
// remotely, it doesn't make sense to serve it from a CDN.
$local_schemes = array_keys(file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL));
if (!in_array($scheme, $local_schemes)) {
  return;
}

You could do something similar, but then throw an exception instead. Then the CKEditorTest::testJSTranslation() test would fail with an exception.

All right then. Here is a quick untested update. The test only patch is the interdiff :)

Great, thanks! :)

#17: please apply the patch, because this patch is not yet committed, therefor the problem cannot yet be solved. See https://drupal.org/patch/apply if you need help doing that.

@eule: also your new problem seems to be unrelated to the old one.

Looks like we should be able to add a quick assertion to make sure this logic works as expected.

#21: that's what Gábor did in #14, I asked for test coverage also. If you want to see why it's done this way rather than by an explicit assertion, please see #7 through #14.

We could theoretically do a test passing in a URL to the locale JS file parser, and expect an exception, is that explicit test useful? The introduction of the exception without the fix in CK failed already :)

Ok, I see. I still think it's a bit weird to add conditions without tests, but I guess you're saying the exception will catch the issue should it happen again.

Committed and pushed to 8.x. Thanks!

Yep, exactly :)