Problem/Motivation

There's no translation:// stream wrapper for translation files, like there is for public:// and private://, even though it's configured the same way and in the same place, leading to inconsistencies and uncertainly about how to access these files correctly.

Proposed resolution

Write a stream wrapper like the one for public:// and private:// - make sure it's used consistently that all value saved to the locale_file table is the translate://local.po format.

As locale_file table isn't migrated from D7, not update patch needed.

Remaining tasks

The #37 patch needs a bit more work, but seems to be close, except for a few edge-case tests.

User interface changes

None

API changes

Locale directory reference should now use the wrapper

Basically all references like this should be updated to use the wrapper:

$directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');

Original report by penyaskito

It would be great having a translations:// stream wrapper, decoupling code that needs to know about these files from the variable name where the folder is being stored. This is line with existing public:// and private:// wrappers to improve developer experience and consistency. The translations file directory is even configurable on the same screen that the rest of the file system paths.

Files: 
CommentFileSizeAuthor
#71 translations-stream-wrapper-1658842-69-71.interdiff.txt2.14 KBwebflo
#71 translations-stream-wrapper-1658842-71.patch9.25 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 41,235 pass(es). View
#69 translations-stream-wrapper-1658842-69.patch9.31 KBwebflo
PASSED: [[SimpleTest]]: [MySQL] 40,734 pass(es). View
#67 i1658842-67.patch9.23 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 40,666 pass(es), 59 fail(s), and 81 exception(s). View
#63 i1658842-63.patch10.36 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 40,389 pass(es). View
#58 interdiff.txt3.21 KBattiks
#58 i1658842.patch10.61 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 40,655 pass(es). View
#56 i1658842.patch9.57 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,641 pass(es), 7 fail(s), and 4 exception(s). View
#56 interdiff.txt3.1 KBattiks
#53 interdiff.txt4.67 KBattiks
#53 i1658842.patch9.62 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,435 pass(es), 157 fail(s), and 46 exception(s). View
#47 i1658842-45.patch9.69 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,434 pass(es), 156 fail(s), and 46 exception(s). View
#46 i1658842-45.patch11.64 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 40,629 pass(es). View
#46 interdiff.txt4.75 KBattiks
#43 i1658842-43.patch7.66 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,301 pass(es), 146 fail(s), and 1,539 exception(s). View
#37 streamwrapper-translations-1658842-33.patch6.54 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 40,105 pass(es), 80 fail(s), and 108 exception(s). View
#37 interdiff-31-to-33.txt612 bytespenyaskito
#33 streamwrapper-translations-1658842-31.patch5.95 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 40,100 pass(es), 80 fail(s), and 109 exception(s). View
#33 interdiff-25-31.txt1.61 KBpenyaskito
#31 interdiff-13-to-25.txt548 bytespenyaskito
#31 streamwrapper-translations-1658842-25.patch4.87 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 40,255 pass(es), 7 fail(s), and 0 exception(s). View
#25 streamwrapper-translations-1658842-13.patch4.53 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 40,252 pass(es), 5 fail(s), and 9 exception(s). View
#25 interdiff-6-to-13.txt2.2 KBpenyaskito
#13 streamwrapper-translations-1658842-6.patch4.87 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 40,122 pass(es), 7 fail(s), and 0 exception(s). View
#13 interdiff-5-to-6.txt2.34 KBpenyaskito
#6 translations-stream-wrapper-1658842-5.patch4.85 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 36,980 pass(es), 7 fail(s), and 6 exception(s). View
#5 translations-stream-wrapper-1658842-3.patch0 bytespenyaskito
PASSED: [[SimpleTest]]: [MySQL] 36,992 pass(es). View
#3 translations-stream-wrapper-1658842-0.patch4.85 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch translations-stream-wrapper-1658842-0.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

penyaskito’s picture

Gábor Hojtsy’s picture

Title: Create a translations:// stream wrapper » 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.

penyaskito’s picture

FileSize
4.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch translations-stream-wrapper-1658842-0.patch. Unable to apply patch. See the log in the details link for more information. View

I'm not very happy with this. For keeping the tests green, I need to process the output of file_scan_directory in locale_translate_get_interface_translation_files.

@@ -278,7 +278,14 @@ function locale_translate_batch_import_files($langcode = NULL, $finish_feedback
  */
 function locale_translate_get_interface_translation_files($langcode = NULL) {
   $directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');
-  return file_scan_directory($directory, '!' . (!empty($langcode) ? '\.' . preg_quote($langcode, '!') : '') . '\.po$!', array('recurse' => FALSE));
+  $return = file_scan_directory($directory, '!' . (!empty($langcode) ? '\.' . preg_quote($langcode, '!') : '') . '\.po$!', array('recurse' => FALSE));
+
+  foreach ($return as $filepath => $file) {
+    $file->uri = 'translations://' . $file->filename;
+    $return[$file->uri] = $file;
+    unset($return[$filepath]);
+  }
+  return $return;
 }

Not sure if it's the path we should follow. If the answer is yes, we need to provide a hook_update_N implementation for updating the locale_file table.

penyaskito’s picture

Status: Active » Needs review
penyaskito’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,992 pass(es). View

Rerolled.

penyaskito’s picture

FileSize
4.85 KB
FAILED: [[SimpleTest]]: [MySQL] 36,980 pass(es), 7 fail(s), and 6 exception(s). View

Oops, it was empty.

Status: Needs review » Needs work

The last submitted patch, translations-stream-wrapper-1658842-5.patch, failed testing.

Gábor Hojtsy’s picture

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

attiks’s picture

+++ 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

Gábor Hojtsy’s picture

Still interested in working on this?

penyaskito’s picture

Yeah, I'll do a last attempt on the sprint :)

penyaskito’s picture

+++ 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

@attiks: Seems like it is not possible. Related: #1714654: file_scan_directory() does not work with a stream wrapper and empty (or root) path "foo://".

I'm taking care of your other comments, thanks for reviewing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
4.87 KB
FAILED: [[SimpleTest]]: [MySQL] 40,122 pass(es), 7 fail(s), and 0 exception(s). View

Translation tests work by now. Haven't launch others, let's see what testbot thinks.

Gábor Hojtsy’s picture

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

Status: Needs review » Needs work

The last submitted patch, streamwrapper-translations-1658842-6.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review

One of the things that make me doubt is if we should follow saving translation paths (/var/www/sites/default/files/translations/one.hu.po) or we should be saving the uri (translations://one.hu.po) into the locale_file table.

All failing tests are because of having mixed the two schemes, and I don't know if the right path should be using drupal_realpath around the uri when comparing to db in tests, or saving the files with the uri scheme prefix.

The second path looks more clean for me, and makes possible to change the location with only changing a variable (let's say, I decide to put my translations under source control in an existing project, but I don't want to put sites/default/files under SCM).

Marking as needs review for discussion.

attiks’s picture

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)

Gábor Hojtsy’s picture

Agreed with attiks.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
chx’s picture

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.

Gábor Hojtsy’s picture

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

attiks’s picture

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

Gábor Hojtsy’s picture

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.

attiks’s picture

@Gábor agreed!

penyaskito’s picture

Issue tags: +Needs upgrade path
FileSize
2.2 KB
4.53 KB
FAILED: [[SimpleTest]]: [MySQL] 40,252 pass(es), 5 fail(s), and 9 exception(s). View

Addressing comments at #20. Needs an upgrade path for existing PO files into the locale_file.

penyaskito’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

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.

Status: Needs review » Needs work

The last submitted patch, streamwrapper-translations-1658842-13.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path

I was looking at that right now with a D7 install.

Removing the tag then, because locale_update_8010 creates the table and no data is migrated from l10n_update, so no data needs to be updated.

penyaskito’s picture

Status: Needs review » Needs work

Back to needs work.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
4.87 KB
FAILED: [[SimpleTest]]: [MySQL] 40,255 pass(es), 7 fail(s), and 0 exception(s). View
548 bytes

Status: Needs review » Needs work

The last submitted patch, streamwrapper-translations-1658842-25.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.61 KB
5.95 KB
FAILED: [[SimpleTest]]: [MySQL] 40,100 pass(es), 80 fail(s), and 109 exception(s). View

Status: Needs review » Needs work

The last submitted patch, streamwrapper-translations-1658842-31.patch, failed testing.

penyaskito’s picture

Assigned: penyaskito » Unassigned

I have clues about how to fix this, but I won't be able of working on this before tomorrow evening. Leaving unassigned if anyone wants to work on it during morning sprint.

My branch is already at the D8MI sandbox, it's called streamwrapper-translations-1658842.

penyaskito’s picture

Assigned: Unassigned » penyaskito

Assigning back to me.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
612 bytes
6.54 KB
FAILED: [[SimpleTest]]: [MySQL] 40,105 pass(es), 80 fail(s), and 108 exception(s). View

When we do low-level operations to files, we must do it with the real filepath. I didn't expect this from streamwrappers, or we are doing anything wrong here.

Status: Needs review » Needs work

The last submitted patch, streamwrapper-translations-1658842-33.patch, failed testing.

BrockBoland’s picture

Needs issue summary

BrockBoland’s picture

Issue summary: View changes

Updating rationale.

twomasc’s picture

Wrote issue summary

twomasc’s picture

Removing "Issue summary initiative" tag

attiks’s picture

Assigned: penyaskito » attiks

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

attiks’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 40,301 pass(es), 146 fail(s), and 1,539 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, i1658842-43.patch, failed testing.

penyaskito’s picture

@attiks, that's right when testing importing files, but not when testing dumping files. We need to set the variable in those cases, but take care that setup does that for you and is not valid for both cases.

attiks’s picture

Status: Needs work » Needs review
FileSize
4.75 KB
11.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,629 pass(es). View

fixed importstatus test and added missing file

attiks’s picture

FileSize
9.69 KB
FAILED: [[SimpleTest]]: [MySQL] 40,434 pass(es), 156 fail(s), and 46 exception(s). View

patch without colors

attiks’s picture

+++ 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

attiks’s picture

+++ 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

Status: Needs review » Needs work
Issue tags: -Needs tests, -D8MI, -sprint, -language-ui

The last submitted patch, i1658842-45.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Needs tests, +D8MI, +sprint, +language-ui

The last submitted patch, i1658842-45.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
9.62 KB
FAILED: [[SimpleTest]]: [MySQL] 40,435 pass(es), 157 fail(s), and 46 exception(s). View
4.67 KB

new patches

Status: Needs review » Needs work

The last submitted patch, i1658842.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
+++ 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.

attiks’s picture

FileSize
3.1 KB
9.57 KB
FAILED: [[SimpleTest]]: [MySQL] 40,641 pass(es), 7 fail(s), and 4 exception(s). View

comments fixed

Status: Needs review » Needs work

The last submitted patch, i1658842.patch, failed testing.

attiks’s picture

FileSize
10.61 KB
PASSED: [[SimpleTest]]: [MySQL] 40,655 pass(es). View
3.21 KB

go bot!

attiks’s picture

Status: Needs work » Needs review
attiks’s picture

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community
<attiks> penyaskito: ping
<penyaskito> attiks, pong
<penyaskito> just looked at your patch, LGTM but haven't tested it
<penyaskito> have you did manual testing?
<attiks> penyaskito: would be great if you can have a look
<penyaskito> and is it drupal_realpath really needed?
<attiks> penyaskito: yes, import+export
<penyaskito> I was not sure about that
<attiks> penyaskito: it's the safest, php normally allows streamwrappers, but not always
<attiks> penyaskito: i probably it will depend on OS and version, so it think we would leave them in
<penyaskito> ok, so RTBCing it
<attiks> penyaskito: nice!

So for me it is RTBC.

attiks’s picture

Status: Reviewed & tested by the community » Needs work

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

penyaskito’s picture

Status: Needs work » Needs review
FileSize
10.36 KB
PASSED: [[SimpleTest]]: [MySQL] 40,389 pass(es). View

The only problem I've found is a notice like

Notice: Undefined index: langcode en locale_translate_batch_import() (línea 447 de /var/www/d8/core/modules/locale/locale.bulk.inc).

This is fixed in the attached patch.

Interdiff:

-  if ($options['langcode'] || preg_match('!(/|\.)([^\./]+)\.po$!', $filepath, $matches)) {
+  if (isset($options['langcode']) && $options['langcode'] ||
+      preg_match('!(/|\.)([^\./]+)\.po$!', $filepath, $matches)) {
attiks’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
9.23 KB
FAILED: [[SimpleTest]]: [MySQL] 40,666 pass(es), 59 fail(s), and 81 exception(s). View

Looks like the streamwrapper class itself has been commited before by accident.
If testbot pass is RTBC.

Status: Needs review » Needs work

The last submitted patch, i1658842-67.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
9.31 KB
PASSED: [[SimpleTest]]: [MySQL] 40,734 pass(es). View

Rerolled the patch from penyaskito in comment 63.

webflo’s picture

+++ 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.

webflo’s picture

Assigned: attiks » webflo
FileSize
9.25 KB
PASSED: [[SimpleTest]]: [MySQL] 41,235 pass(es). View
2.14 KB

Go testbot!

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

attiks’s picture

Webflo, thanks for following up

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!

Gábor Hojtsy’s picture

Issue tags: -sprint

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!

jbrown’s picture

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

Gábor Hojtsy’s picture

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.

c960657’s picture

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

+++ 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.

effulgentsia’s picture

Issue summary: View changes

Update the summary, using the template summary template.