Add file_exists() to _locale_parse_js_file() to prevent PHP warning

Issue exists in D7. Patch for D7 attached.

Test previously failed due to version incompatibility. Changing the version to 7 for patch testing purposes. Will switch back to 8 after patch has finished testing.

Testing complete - changing version back to D8.

I hadn't noticed it, or I wouldn't have posted it.

I tested this last #5 patch since I had the same error recently after installing a new module.
Just to confirm this patch effectively solves this displayed error.
Thanks

Yes, I've been using it and it fixes the problem for me too.

Thanks for the fix! However, we like to include an automated test each time we fix a bug, because that way we don't end up re-introducing the problem at a later date. Tagging as "Needs tests." (Also tagging with needs backport, since it seems this problem occurs in D7 as well)

The original post seems to do a good job of laying out how to reproduce the bug. We just need that in code. See http://drupal.org/node/1468170 for more details.

I don't think you read the thread through properly. Not only are their patches for both D7 and D8 in here, both of them have passed the simpletest. You'll notice I temporarily switched the version back to D7 in order to have the simpletest run on D7, then switched it back to D8 after that.

The patch needs new simple tests that confirms that the specific fix works.

Sorry, I misunderstood webchicks comments. I didn't realize tests had to be created for the patch. Makes sense.

Attaching patch with simpletest for D8.

Note that this is my first simpletest patch submission for Drupal, so it should probably be reviewed by someone in the know even if it passes the automated testing system.

The first patch includes the simpletest, without the patch applied (supposed to fail)
The second patch includes the simpletest with the patch applied (supposed to succeed)

Edit: I've re-tested the second patch, and it passed testing, so the comment below can be ignored.

I don't even know where to begin to fix that. The error is being reported in Views, something to do with a Views handler. But it's appearing in both tests, and the first test is simply the addition of the simpletest files (including a hidden module) which shouldn't have anything to do with Views at all, or even change anything for that matter.

Adding patch that should definitely work as it's only added one line of irrelevant code (1 == 1) to the index file. I'm trying to figure out whether the last failed patch is a problem in my code or in the core code.

This patch can be safely ignored by everyone.

#15: locale-D8_locale_check_file_exists-180330-15.patch queued for re-testing.

Hmm. Looks like it must have been a system bug. My test patch worked fine, so I re-tested the second patch in post #15, and it cleared this time. So the D8 patch appears to be good to go. I will work on the D7 patch next.

D7 patch attached.

Note: temporarily changing the version to D7 so as to ensure the patch is tested against D7. Version will be switched back to D8 after test has been run.

The first patch includes the simpletest without the patch applied (supposed to fail)
The second patch includes the simpletest with the patch applied (supposed to succeed)

Test passed. Setting thread back to D8.

Bump

I'd be wary of this patch because it would hide the fact that the module/theme actually has a problem that needs to be corrected.

Somewhat related: http://drupal.org/node/1715222

I'd be wary of this patch because it would hide the fact that the module/theme actually has a problem that needs to be corrected.

Let's trace this problem back a bit. First some background. The Drupal JavaScript framework provides the Drupal.t() and Drupal.formatPlural() functions. These functions works like the Drupal core functions t(), format_plural(). When the locale module is enabled, it supplies translations where these functions are called.

Any part of the system can add javascript files using drupal_add_js(). Any part of the system can also call drupal_get_js() to get the current list of javascript items. drupal_get_js() then invokes hook_js_alter() so that any module can alter the javascript object. The locale module implements this hook, and parses all javascript files to see if they have used the Drupal.t() or Drupal.formatPlural() functions. It gets the contents of these files by calling file_get_contents().

The problem here is that a module developer may attempt to add a non-existent js file using drupal_add_js(). A site builder then uses that 3rd party module, enables the locale module, and gets this error. So if the site builder wants to fix this error, they have to either hack core, or hack the 3rd party module, both of which are recommend against. [The other option would be turning off error reporting, but that's overkill to deal with this one error.]

This is what happened in my case. The module developer had some code that attempted to add a non-existent file. I reported the error, but in the meantime I was left with this error unless/until the developer chose to fix it.

Many Drupal site builders do not know how to apply patches, and/or are unable/unwilling to alter core. If we want Drupal to be accessible to the community at large, we at the very least need to be suppressing this message by calling file_exists() before attempting to add a file, and possibly implementing an API to handle this situation (writing to the dblog() and/or writing to the javascript console for example). Right now we have left a hole in the system, and are relying on PHP error reporting to catch it.

Silently catching errors undermines the purpose of PHP warnings, making it very difficult for the module maintainer to discover the bug while introducing a performance hit. Suppressing the warning and adding a new way to log it would be reinventing the wheel, as this is exactly what the error display options are for. The correct course of action is for the site builder to disable the display of error messages on their production sites, patch their copy of the module (even if only to include a stub file) and report the bug to the maintainer.

I'm sympathetic to the idea of non-technical administrators encountering this problem but, for the sake of security, production sites should not have error reporting enabled. A better argument would be to turn off error reporting by default: http://drupal.org/node/688638

And this is why people find Drupal unfriendly

Sorry, but I can't agree with you. This just puts Drupal out of the league of your average developer and is an elitist stance.

Another use case is using BASIC AUTH on the webserver. I got this error
file_get_contents(http://example.com/sites/example.com/files/js/wysiwyg/wysiwyg_tinymce_Vx...): failed to open stream: HTTP request failed! HTTP/1.1 401 Authorization Required in _locale_parse_js_file() (regel 1488 van /var/www/drupal7/includes/locale.inc).

The test for file_exists on the parsed $path will probably and thus suppress the error which is not good.

See also #1814980: file_get_contents is used on remote URIs in locale.inc for another use case.

Thanks @Jaypan for trying to solve this. Unfortunately is needs works figuring out the other path.

Is this still a bug in Drupal 8?

to elaborate on @clemens.tolboom's comment: Why should we do an http request for this? I would expect these files to be accessible via public://, so a direct file operation without a trip through the http stack, should be possible.

Hi, can someone explain further how to use this patch? I am relatively new with drupal and want to make sure I apply this in the correct way. Thanks!

@elianarunner: The best explanation is on https://drupal.org/patch/apply

The 7 patch needs a re-roll.

patching file includes/locale.inc
patching file modules/locale/locale.test
Hunk #2 FAILED at 3090.
1 out of 2 hunks FAILED -- saving rejects to file modules/locale/locale.test.rej
patching file modules/locale/tests/locale_test_add_js.info
patching file modules/locale/tests/locale_test_add_js.module

reroll attempt. Please review closely since it is 3AM for me.

Sorry, didn't work. Feel free to take this. Otherwise, I'll try again in a few days.

Changing to 7.x for the testbot.

33: 1803330-33-D7.patch queued for re-testing.

@ clemens.tolboom

#28
Yes, I am able to reproduce this. We’re running a test site using Basic Authentication. Occasionally we get the error message, too:
file_get_contents(http://example.com/sites/example.com/files/js/wysiwyg/wysiwyg_tinymce_Vx...): failed to open stream: HTTP request failed! HTTP/1.1 401 Authorization Required in _locale_parse_js_file() (regel 1488 van /var/www/drupal7/includes/locale.inc).

Changing it back so we can get this into _locale_parse_js_file() in D8. #15.

I get this problem when a url it tries to parse begins with a "/" (a relative url for an angular app on my site).

I fixed it by adding:

    $filepath = preg_replace('#$/#', '', $filepath);

between lines 1486 and 1487,

this fixed certain things but the patch worked better...oops, sry.

Well, the patch in #15 needs to be retested.

Re-rolled patch against latest D8

Patch has a hook_menu, drupal_add_js(), and isn't actually in a functioning state (test doesn't appear in admin/config/development/testing). Reworking this and will submit updated patch soon.

Moved LocaleAddJSTest.php to core/modules/locale/src/Tests to reflect PSR-4 standards. Also converted .info file to .info.yml. This fixed the test appearing in admin/config/development/testing. Removed the hook_menu as well as the drupal_add_js and replaced with hook_page_attachments per the former functions being deprecated. Test is now functioning and can be reviewed by simply commenting out lines 1048-1050 of the locale.module and running LocaleAddJSTest from the Testing UI.

Can we also get a version of the patch with only the test coverage uploaded? That way we can make sure the tests fail without the bugfix and will catch regressions. Thanks @Tommy Cox!

The tests fail sans bugfix. Ready for review!

Converted getInfo() to PHPDoc. Removed setUp(). Cleaned up comments.

I realized I commented in #48 but since this got a bump I'm adding performance related tags since in other areas we have actually been removing calls to file_exists().

Is there another approach we can take?

We're about to parse those JS files anyway, so a file_exists() doesn't matter for performance. See the file_get_contents() on the next line.

file_exists() is problematic when it is about php files that we could serve from opcache and won't require IO at all in some configurations. But here, it really doesn't matter.

Fair enough, thanks @Berdir :)

Edit Remaining Tasks in Summary

+++ b/core/modules/locale/tests/modules/locale_test_add_js/locale_test_add_js.module
@@ -7,24 +7,10 @@
+  $page['#attached']['js'][] =

Maybe the 'js' needs to be 'library' ?

Will just try it.

Was the test aborted on purpose or should this be re-queued?

Likely accidental, I added another test.

Note that error reporting is disabled by default in D8 since https://www.drupal.org/node/2226761

Should we add a watchdog message as this shouldn't exist as a problem but it does?

+++ b/core/modules/locale/src/Tests/LocaleAddJSTest.php
@@ -0,0 +1,36 @@
+ * Tests adding JavaScript when locale module is enabled

+++ b/core/modules/locale/tests/modules/locale_test_add_js/locale_test_add_js.module
@@ -0,0 +1,16 @@
+ * test for non-existence of error message
...
+ * Adds a non existent js file to the page

I think these should end with periods.

The actual interdiff for #75. Attached a start to the watchdog in the last post, but not sure exactly what to do about the watchdog.

The current test doesn't cause an error anymore without the patch, rendering it useless.

+++ b/core/modules/locale/tests/modules/locale_test_add_js/locale_test_add_js.module
@@ -0,0 +1,16 @@
+function locale_test_add_js_page_attachments(&$page) {
+  $page['#attached']['library'][] =
+    drupal_get_path('module', 'locale_test_add_js') . '/non_existent_js_file.js';
+}

This likely needs to be tested and updated. I'm guessing this is no longer the best practice for attaching JavaScript.

Made some updates. Not sure why, but the test module hook doesn't seem to get called.

This should do the trick

Whoops, included local testing module in last commit. Re-rollll

Thanks for figuring out the library attachment! What do you think about the watchdog?

Makes sense to me.

One issue: I can't get the test patch (drupal-add-file_exists-to-1803330-85-tests.patch) to fail, which it should. If I install the test module as a regular module the issue is reproduced:

Some of this is not needed, but I didn't have time to clean it. Shows the bug though, I think.

uncomment the fix.

Great! I just removed locale from the test setup as it's already a dependency of locale_test_add_js.module. This finally looks RTBC to me.

Comment Adjustments.

Path #33 still working with Drupal 7.54. Thanks @lokapujya :)

This is in direct conflict w/ #2735717: Stream wrapper reference in JS library causes error in _locale_parse_js_file(). Should it be closed duplicate and our forces combined over there?

please ignore this patch

Rerolled patch against 9.5x. please review

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed failed commands on #114
Attached patch against Drupal 10.1.x

Path #33 still working with Drupal 7.97. Thanks @lokapujya :)