See Gábor's presentation from 17m35s.
Steps to reproduce:
- have language and locale modules enabled,
- create an empty drupal-something.pl.po in sites/default/files/translations directory,
- go to http://example.com/admin/config/regional/language/add and select Polish,
- 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.
Comment | File | Size | Author |
---|---|---|---|
#11 | 1750228-11.patch | 4.33 KB | corvus_ch |
#10 | 1750228-10.patch | 5.02 KB | corvus_ch |
#6 | drupal-core-1750228-6-interdiff.txt | 1.65 KB | Boobaa |
#6 | drupal-core-1750228-6-test_and_fix.patch | 3.82 KB | Boobaa |
#1 | drupal-core-1750228-1.patch | 3.27 KB | Boobaa |
Comments
Comment #1
BoobaaHere'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.
Comment #2
Gábor HojtsyComment #3
harrrrrrr CreditAttribution: harrrrrrr commentedTested and works
Comment #4
corvus_ch CreditAttribution: corvus_ch commentedConfirm, the patch does fix the problem. So lets add some tests for that.
Comment #5
corvus_ch CreditAttribution: corvus_ch commentedFor that is not directly related to this issue, this should be moved to its own issue.
Comment #6
BoobaaHere are patches with tests, both with and without the actual fix.
Comment #7
das-peter CreditAttribution: das-peter commentedLooks good from my perspective.
If I would want to be nit-picky I'd say we should fix the Drupal Code Sniffer Report:
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.
Comment #9
BoobaaLooks 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. ;)
Comment #10
corvus_ch CreditAttribution: corvus_ch commentedHere it is. Who want's to review?
Comment #11
corvus_ch CreditAttribution: corvus_ch commentedAnd here is the one without the changes in FileTransferTest.
Comment #12
das-peter CreditAttribution: das-peter commentedAs far as I see the only change in #11 is:
Thus same statement from my side - RTBC if tests pass.
Comment #13
das-peter CreditAttribution: das-peter commentedTests passed - let's RTBC this.
Comment #14
PrabhuG CreditAttribution: PrabhuG commentedManual Test passed. After applying patch "1750228-11.patch", AJAX error is not obtained.
Comment #15
webchickLooks 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!
Comment #16
Gábor HojtsyRemoving from sprint. I don't think this should get a changelog entry or change notice, it is such an edge case. Thanks all though!