Introduce a translations:// stream wrapper to access the .po file directory

Retitling. Note that this was suggested by @rfay and discussed with a few people on IRC as a better way to manage files in the translations directory instead of relying on conf_path() and variable_get() all the time. This is in line with public:// and private:// which are also stream wrappers for configured directories. The .po translations file directory is configured the same way and for DX it makes sense to have a stream wrapper for it as well.

Is it not possible to file scan directory with translations:// as the directory? That should get you those paths back, no?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -72,8 +72,8 @@ class LocaleFileImportStatus extends WebTestBase {
+    $dir = variable_set('locale_translate_file_directory', drupal_get_path('module', 'locale') . '/tests');

Do we need the assignment?

+++ b/core/modules/locale/lib/Drupal/locale/TranslationsStream.phpundefined
@@ -0,0 +1,37 @@
+  public function getDirectoryPath() {
...
+  function getExternalUrl() {

add/remove public

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -278,7 +278,14 @@ function locale_translate_batch_import_files($langcode = NULL, $finish_feedback
   $directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');

use StreamWrapper

Still interested in working on this?

Looks generally good to me. Would be good to have a stream wrappers expert look at this. Hm.

Don't store the real path, it might change if you deploy a site, I'm all in favor of storing either translations://one.hu.po or one.hu.po since the goal is to always use translations://

If we want to go for consistency we should store translations://one.hu.po, file_managed does the same (ex. public://Curve.jpg)

Agreed with attiks.

Why is getExternalUrl implemented? This is completely new code and I do not see a reason of its existence. Do we want to provide external , browser-accessible URLs to po files??

I completely agree with the realpath removal.

I don't think we wanted to provide public access to the .po files.

#20 I think the original idea was to be able to download .po files from an external location

Yeah, well, sites should not really make their .po files available for download. At least our htaccess used to have access deny patterns to avoid .po files being downloaded from sites, which might or might not have been removed. It is supposed to be internal data on sites. They might download it from remote sites, but they would not make it available for download.

@Gábor agreed!

locale_file is not present on Drupal 7, so unless we want to have an l10n_update => D8 core upgrade path, we don't need to touch the data, since the table will be new and empty with D8. I don't remember if we did the l10n_update => D8 upgrade path or not when introducing it.

Needs issue summary

Updating rationale.

Wrote issue summary

Removing "Issue summary initiative" tag

I'm doing some debugging:
'translations://po_u0Ip1X.po' maps to '/var/aegir/platforms/drupal8git/core/modules/locale/tests/po_u0Ip1X.po' which can not be right

patch for testbot, problem was that the tests were using /core/modules/locale/tests as directory, but that isn't writeable.

fixed importstatus test and added missing file

patch without colors

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -94,7 +94,8 @@ function locale_translate_import_form_submit($form, &$form_state) {
+  $destination = variable_get('locale_translate_file_directory', '');

can go away

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -94,7 +94,8 @@ function locale_translate_import_form_submit($form, &$form_state) {
+  $destination = variable_get('locale_translate_file_directory', '');

can go away

#47: i1658842-45.patch queued for re-testing.

new patches

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -35,8 +35,14 @@ class LocaleFileImportStatus extends WebTestBase {
-    // Set the translation file directory.
-    variable_set('locale_translate_file_directory', drupal_get_path('module', 'locale') . '/tests');
+    // Set the translation file directory to something writable.
+    $destination = conf_path() . '/files/translations';
+    file_prepare_directory($dir, FILE_CREATE_DIRECTORY);
+    variable_set('locale_translate_file_directory', $destination);

$dir is a undefined variable. Do you mean $destination? The default value for locale_translate_file_directory is conf_path() . '/files/translations' already. Why do you set it?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleFileImportStatus.phpundefined
@@ -191,11 +196,10 @@ class LocaleFileImportStatus extends WebTestBase {
     $dir = conf_path() . '/files/translations';
     file_prepare_directory($dir, FILE_CREATE_DIRECTORY);

Lets prepare the directory in the setUp method.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.phpundefined
@@ -36,8 +36,14 @@ class LocaleImportFunctionalTest extends WebTestBase {
-    // Set the translation file directory.
-    variable_set('locale_translate_file_directory', drupal_get_path('module', 'locale') . '/tests');
+    // Set the translation file directory to something writable.
+    $destination = conf_path() . '/files/translations';
+    file_prepare_directory($dir, FILE_CREATE_DIRECTORY);
+    variable_set('locale_translate_file_directory', $destination);

Same as the first comment.

comments fixed

go bot!

We detected some problems in #1751326: When locale import happens on module enable, many notices are thrown that are more related to this patch, so back to NW

So lets move this one forward and handle the other problems in #1751326: When locale import happens on module enable, many notices are thrown once this is committed

Ok, as of today we are once again under thresholds, so 8.x features are on the table again. Yay!

Unfortunately, this patch no longer applies. :( Can we get a quick re-roll? In looking through, I couldn't find anything to complain about, so feel free to mark RTBC again once this is done.

Rerolled the patch from penyaskito in comment 63.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.phpundefined
@@ -36,8 +36,13 @@ class LocaleImportFunctionalTest extends WebTestBase {
-    // Set the translation file directory.
-    variable_set('locale_translate_file_directory', drupal_get_path('module', 'locale') . '/tests');
+    // Set the translation file directory to something writable.
+    $destination = conf_path() . '/files/translations';
+    file_prepare_directory($destination, FILE_CREATE_DIRECTORY);

I think its better to use the 'translations://' stream wrapper here.

Go testbot!

Was RTBC 4 days ago before rerolls and minor improvements, the streamwrapper class is already committed, so better commit the rest :)

Webflo, thanks for following up

Yea, sorry about that. :P

I reviewed this already at one of the D8MI sprints, and it looks good to me. COmmitted and pushed to 8.x. Thanks!

I've updated the changelog at http://drupal.org/node/1352228 to refer to this issue as well, and updated the code example there to show usage example of the new stream wrapper. This should be fully done then :) Thanks all!

Shouldn't it be translation:// instead of translations:// ?

This exposed this pretty unfortunate side effect: #1787520: Translations not imported on installation - not necessarily a problem with this patch, rather how the module enable process works.

Follow-up: #1791920: Create new translations:// directory for tests

+++ b/core/lib/Drupal/Component/Gettext/PoStreamReader.php
@@ -165,7 +165,7 @@ class PoStreamReader implements PoStreamInterface, PoReaderInterface {
-      $this->_fd = fopen($this->_uri, 'rb');
+      $this->_fd = fopen(drupal_realpath($this->_uri), 'rb');

This is causing a problem with the installer. See #1864292: Installation in non-English language fails.

Update the summary, using the template summary template.