Translations directory not autocreated

I've seen this lately. We had the module enabled all the way up to prod, then new developers (me!) joined the company. The module is enabled in config, and we were provided with a database dump (to get content and users), so the module was enabled in the database dump.

The locale module therefore never got installed on my local machine and the folder never got created. I've had to create a new update hook to ensure it got created on all environments, including local and remote, as we were moving hosts as well.

Perhaps there could be a requirements check for this module that makes sure the translations directory exists and is writeable?

@Sophie.SK, Can you please clarify "The locale module therefore never got installed on my local machine" How can you explain this? If you combine code and a database, any module enabled in the database should be enabled. Did you run config import after combining code and DB on your local environment?

The locale module does check for and create the directory at various points. For example when enabling the module and when writing po files.

I'm not sure if the module can be and should be made responsible for re-creating the environment. If your system relies on a translation directory within the public files directory, you can enforce this by placing a gitkeep file inside the translations directory and exclude that from gitignore. That way an empty directory is made available via the VCS.

@Sutharsan

Re-reading the comment, I think I mean "I didn't enable the module locally, therefore the install hooks never ran".

The module was enabled, but I didn't enable it. The directory creation happens on install.

This was further exacerbated when we switched hosts recently. We had the database and the config, so the module was enabled, but it wasn't being installed and the translations directory didn't get created when we deployed. I had to write an additional update hook to create the directory on each environment.

If your system relies on a translation directory within the public files directory, you can enforce this by placing a gitkeep file inside the translations directory and exclude that from gitignore.

That could work. Should it be documented that a directory will be created on install but if it's required otherwise then a gitkeep would be needed?

Restructured the issue summary with same content in an attempt to make the case easier to understand.

I can think of the following cases that should be considered:
- Update translation via user interface
- Upload a translation via the Translation Import page
- Adding a language
- Does configuration import trigger interface translation update?
- Update translation via drush
- Update translation via drupal console

Drush and Drupal Console are not in scope of this issue, but are important to consider when implementing a solution.

Some technical notes: Check translation status and Update translations are two independent processes. During 'check' the translation directory may be used for reading (only if the translation source is local or local+remote). During 'update' the translation directory is used for reading and writing (writing only in case of remote translation source).

Interesting idea, but it feels like cutting corners. When I look at https://www.php.net/manual/en/class.streamwrapper.php I think the streamwrapper must knows about the 'how' and not about the 'when'. It has an mkdir method, but looking at core implementations, it does not use it itself.

I have made a first patch that adds a batch task that creates the directory right before the download/import tasks. It also handles creating the directory when importing a translation via the interface.

I'm not sure about this approach, as it seems to rely on uploading a translation manually through the UI. That means we wouldn't get the benefits when doing things elsewhere (eg enabling a new module, which fetches new translations).

I think this is useful for one particular use case... maybe it's worth adding a requirements check (shown on the Status Report) for the directory existing?

When performing another action that tries to create translations (such as enabling a module), it'd be useful to check whether the directory exists before trying to create them, to avoid the error message.

@Sophie.SK, Mabe I was not clear about what the patch does. Right before the batch task that imports translations the translation directory is created (when needed). Additionally the translation directory is created before manual translation import.

(updated the #11 text accordingly)

This patch add a test for the new function that creates the directory. It updates an existing test for the added cron task.

I re-rolled the patch against 9.1.5.

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

1. +++ b/core/modules/locale/locale.module
   @@ -764,6 +764,15 @@ function locale_system_file_system_settings_submit(&$form, FormStateInterface $f
   +function locale_prepare_translations_directory() {
   

   I wish locale code was a bit more like the rest of core. Adding more functions to the global namespace is a shame.

2. +++ b/core/modules/locale/locale.module
   @@ -764,6 +764,15 @@ function locale_system_file_system_settings_submit(&$form, FormStateInterface $f
   +  $directory = \Drupal::config('locale.settings')->get('translation.path');
   ...
   +  \Drupal::service('file_system')->prepareDirectory($directory, $options);
   

   Code in \Drupal\locale\Form\LocaleSettingsForm::buildForm

       if ($directory = $config->get('translation.path')) {
         $description = $this->t('Translation files are stored locally in the  %path directory. You can change this directory on the <a href=":url">File system</a> configuration page.', ['%path' => $directory, ':url' => Url::fromRoute('system.file_system_settings')->toString()]);
       }
       else {
         $description = $this->t('Translation files will not be stored locally. Change the Interface translation directory on the <a href=":url">File system configuration</a> page.', [':url' => Url::fromRoute('system.file_system_settings')->toString()]);
       }
   

   implies that is acceptable for this setting to be empty.

   Also I'm not sure that the extra complexity around the permissions is really necessary. We don't do this in system_check_directory() so if you go to the file system form and press save the permissions will change.

3. +++ b/core/modules/locale/tests/src/Kernel/LocalePrepareDirectoryTest.php
   @@ -0,0 +1,36 @@
   +    $directory = Settings::get('file_public_path') . '/' . strtolower($this->randomMachineName(8));
   

   There's no need for the name to be random - a consistent name would be better here - something like 'test_translations'...

4. One thing that odd is that if you go to the file system form and press save you'll end up with an htaccess file that'll deny access to all files in the folder via your web server - which feels correct. I think if we are going to introduce locale_prepare_translations_directory() then we should call it from locale.install and also call FileSecurity::writeHtaccess($directory); in it - and then things are more consistent.
5. The test is testing locale_prepare_translations_directory() - it's not testing the import form or the batch - ie. the bug that has caused the need to make this change.

Re-roll patch #21

Still needs work, see #25

Fixed failed tests of patch #28.

If you go to the admin/config/media/file-system page, the "translations" directory will be created and will contain a .htaccess but I don't know what code does this.

Just ran into this issue once again. It would be great to fix this, since it can be pretty annoying such as breaking CI pipelines when installing modules.

Could we move this issue forward? I guess the comments in #25 are still valid?

Re rolling patch 30 for latest Drupal 10.2.x

I made a mistake re-rolling 35, so I've re-rolled a new patch to resolve the issue which should apply cleanly against latest 10.2.x

The MR !8320 should address the feedback from #25.3 and #25.5, but I wonder what action (if any) should be taken for other comments on #25, so marking this "Needs review".

I feel the priority should be raised, because this issue is typical for causing frustration, since the the UI doesn't give a proper indication of what is wrong with settings.

MR works fine. Tested drush locale-update and translations update via UI

I have mixed feelings about this issue.

I think the missing translation directory in your dev situation for example is a lack of syncing files in your development workflow. Other critical files may also be missing. Eg. the JS translation files may get regenerated locally and that may end up leading to a mismatch in your live site when the updates get pushed (depending on what you push).

At the same time I understand the developer experience improvement that instead of an error, you get an empty directory at least. I am not sure though if that does not lead to unwanted side effects.

How are other files missing handled by Drupal? Eg. if your site logo is missing or other key files that you don't have on the dev environment?

One MR specific comment: How is the locale/src/Plugin/QueueWorker/LocaleTranslation.php change related?

Should this be in PNMI ?

To address some of the questions in #42

How are other files missing handled by Drupal? Eg. if your site logo is missing or other key files that you don't have on the dev environment?

I don't think its recommended to include files from sites/default/files to version control. If the theme build assets are not included in version control and the build process fails, at least there's visual cue that the files are missing. When translation directory missing, the error message on UI/watchdog doesn't give guidance how to resolve the issue. I added the screenshot of the error message on UI to issue summary.

For code specific discussion I think the comments on merge request would be preferred, but locale/src/Plugin/QueueWorker/LocaleTranslation.php change is needed because LocaleTranslation::processItem() provides zero $args. The tests where failing without this change.

Indeed that error message is not helpful. Could the error be more specific and tell the user the directory does not exist? What can the user then do to get the directory created? (Save the file settings config form?) At least if we let the user know this, they have a chance to sync their translation files if they wish so without silently getting them into a different situation.

Thanks so much for taking a look @gábor hojtsy moving to NW to address the feedback.

re: #46
Asking users to go to separate form to resolve this issue does not help the command-line use cases (CI pipelines, syncing database from different environment, etc.).

IMHO automatically creating the missing translation directory gives the best DX/UX. We could add some message to inform about autocreation.

I have run into this a few times with CI pipelines and it is a bit of an annoyance. I agree with sokru that we should handle it automatically if it doesn't exist. I can't see a downside to having it there, even if it is empty.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

MR updated + missing typehints.

We could add some message to inform about autocreation.

Think would be a good compromise

Whoops - tried to update the MR to work with 11.2 but it seems to have gone wrong somewhere.. Sorry!

Development happens on 11.x, not 11.2.x.

Fixed MR by rebasing back with 11.x. Didn't really review, just git magic.

Patch file of the MR for those that want to use it in composer.patches.json. Applies on 11.2.1.