See Gábor's presentation from 17m35s.

Steps to reproduce:

  1. have language and locale modules enabled,
  2. create an empty drupal-something.pl.po in sites/default/files/translations directory,
  3. go to http://example.com/admin/config/regional/language/add and select Polish,
  4. click "Add language".

All you get is a fatal error:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=13&op=do_nojs&op=do StatusText: OK ResponseText: ( ! ) Fatal error: Call to a member function getTranslation() on a non-object in /home/boobaa/drupal/d8/core/lib/Drupal/Component/Gettext/PoStreamReader.php on line 234 Call Stack #TimeMemoryFunctionLocation 10.0003642464{main}( )../index.php:0 20.148920601992Symfony\Component\HttpKernel\Kernel->handle( )../index.php:35 30.168021720200Drupal\Core\HttpKernel->handle( )../Kernel.php:193 40.168121721160Symfony\Component\HttpKernel\HttpKernel->handle( )../HttpKernel.php:51 50.168121721160Symfony\Component\HttpKernel\HttpKernel->handleRaw( )../HttpKernel.php:73 60.201023946768call_user_func_array ( )../HttpKernel.php:129 70.201023947016Drupal\Core\EventSubscriber\{closure}( )../HttpKernel.php:129 80.201023947064call_user_func_array ( )../LegacyControllerSubscriber.php:45 90.201023947312system_batch_page( )../LegacyControllerSubscriber.php:45 100.201023947312_batch_page( )../system.admin.inc:2337 110.201123948240_batch_do( )../batch.inc:84 120.201123948240_batch_process( )../batch.inc:108 130.204224235080call_user_func_array ( )../batch.inc:245 140.204224235120locale_translate_batch_import( )../batch.inc:245 150.233425967800Drupal\locale\Gettext::fileToDatabase( )../locale.bulk.inc:452 160.235626179360Drupal\Component\Gettext\PoStreamReader->open( )../Gettext.php:89 170.235726180088Drupal\Component\Gettext\PoStreamReader->readHeader( )../PoStreamReader.php:170

Patch is on the way.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Boobaa’s picture

Assigned: Boobaa » Unassigned
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.27 KB

Here's the patch that handles this 0-bytes file case correctly; without tests for this time, apparently.

PS. It looks like we will scan all the files that have both explicit namespace declaration and "new Exception()" in them, but this should be another issue, I think.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint
harrrrrrr’s picture

Tested and works

corvus_ch’s picture

Status: Needs review » Needs work

Confirm, the patch does fix the problem. So lets add some tests for that.

corvus_ch’s picture

+++ b/core/modules/locale/lib/Drupal/locale/Gettext.phpundefined
@@ -94,7 +94,7 @@ class Gettext {
     $header = $reader->getHeader();
     if (!$header) {
-      throw new Exception('Missing or malformed header.');
+      throw new \Exception('Missing or malformed header.');
     }

For that is not directly related to this issue, this should be moved to its own issue.

Boobaa’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.82 KB
1.65 KB

Here are patches with tests, both with and without the actual fix.

das-peter’s picture

Looks good from my perspective.
If I would want to be nit-picky I'd say we should fix the Drupal Code Sniffer Report:

...
49 | ERROR | No scope modifier specified for function "testStandalonePoFile"
59 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
...
351 | ERROR | No scope modifier specified for function "getEmptyPoFile"
...

But since the whole file contains violations I think a clean-up can/should be a follow-up.
Thus I'd say RTBC as soon as test are green.

Status: Needs review » Needs work

The last submitted patch, drupal-core-1750228-6-test_and_fix.patch, failed testing.

Boobaa’s picture

Looks like this fails for the testbot (and some others) because the code wants to throw a new Exception in its own namespace, without using any Exception classes. Let's fix that by introducing the proper use line; assigning this turn to @corvus_ch. ;)

corvus_ch’s picture

Status: Needs work » Needs review
FileSize
5.02 KB

Here it is. Who want's to review?

corvus_ch’s picture

FileSize
4.33 KB

And here is the one without the changes in FileTransferTest.

das-peter’s picture

As far as I see the only change in #11 is:

diff --git a/core/modules/locale/lib/Drupal/locale/Gettext.php b/core/modules/locale/lib/Drupal/locale/Gettext.php
index 9d3bfb8..1f42289 100644
--- a/core/modules/locale/lib/Drupal/locale/Gettext.php
+++ b/core/modules/locale/lib/Drupal/locale/Gettext.php
@@ -10,6 +10,7 @@ namespace Drupal\locale;
 use Drupal\Component\Gettext\PoStreamReader;
 use Drupal\Component\Gettext\PoMemoryWriter;
 use Drupal\locale\PoDatabaseWriter;
+use Exception;
 
 /**
  * Static class providing Drupal specific Gettext functionality.

Thus same statement from my side - RTBC if tests pass.

das-peter’s picture

Status: Needs review » Reviewed & tested by the community

Tests passed - let's RTBC this.

PrabhuG’s picture

Manual Test passed. After applying patch "1750228-11.patch", AJAX error is not obtained.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks good, although I'm confused why empty files are considered valid data not worth raising an exception over, Gábor informed me that this is by design.

Just removed the t() around assertion message in:

+ $this->assertRaw(t('The translation was successfully imported. There are %number newly created translated strings, %update strings were updated and %delete strings were removed.', array('%number' => 0, '%update' => 0, '%delete' => 0)), t('The empty translation file was successfully imported.'));

...and committed/pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from sprint. I don't think this should get a changelog entry or change notice, it is such an edge case. Thanks all though!

Automatically closed -- issue fixed for 2 weeks with no activity.