Problem/Motivation

The translations directory locations is currently locked to $site_path . '/files/translation' during site installation (in install_check_translations()).

Proposed resolution

This changes makes it configurable like it is already done in install_begin_request() (same file line 429).

I have marked the issue as a feature request but one could argue it's almost a bug because it was already made configurable in other parts of the install process :-)

Remaining tasks

- create MRs and functional test (or extend some)
- decide about option for quichinstall of core, related to #3089277: Provide core CLI commands for the most common features of Drush

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#38 2950407-nr-bot.txt3.04 KBneeds-review-queue-bot
#37 2950407-37.patch10.42 KBcasey
#36 2950407-nr-bot.txt2.36 KBneeds-review-queue-bot
#35 2950407-35.patch11.17 KBtimohuisman
#25 drupal-translations_directory_configurable-2950407-24.patch10.59 KBarnested
#18 2950407_17.patch22.16 KBtimohuisman
#14 2950407-8.9.x-14.patch10.64 KBtstoeckler
#2 drupal-make-translation-directory-configurable-during-install-2950407-2-D8.patch1.53 KBarnested

Issue fork drupal-2950407

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

arnested created an issue. See original summary.

arnested’s picture

arnested commented
Status: Active » Needs work
StatusFileSize
new drupal-make-translation-directory-configurable-during-install-2950407-2-D8.patch1.53 KB
arnested’s picture

arnested commented
Status: Needs work » Needs review

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

casey’s picture

casey commented
Version: 8.6.x-dev » 8.9.x-dev

The patch still seems to be working just fine.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

tstoeckler made their first commit to this issue’s fork.

tstoeckler opened merge request !37

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Thanks @arnested and @casey for opening and reviewing this issue!

This is still a problem. I am coming from #2625820: install_check_translations() sometimes incorrectly returns NULL instead of array and found this issue now. The patch in #2 works fine, I just think we can do a bit better:

  1. Instead of duplicating the logic from install_begin_request() I think it makes more sense to "store" it in the install state and just re-use it from there. Instead of just adding a third argument to install_check_translations() for this purpose I think it is better to just pass the $install_state entirely. That will also allow further fixes in the future; I am for example interested in making install_check_translations() aware of the configured translation filename pattern.
  2. Creating the public files directory only makes sense if the translation directory is subdirectory of it, so we should check that.
  3. We should try to add some test coverage for this.

I created a merge request that does exactly that. I added a draft change notice for the parameter change in install_check_translations() and added a basic test. I was not able to add a legacy test for the parameter deprecation because install_check_translations() performs HTTP requests, so I hit #2571475: Outbound HTTP requests fail with KernelTestBase. The only way to avoid the HTTP request is the early return that will be removed by #2625820: install_check_translations() sometimes incorrectly returns NULL instead of array so I don't think it makes much sense to exploit that here.

I am also not providing a "tests-only" patch because that triggers #3158071: Redirect loop during installation with non-English so it could possibly crash the test boot due to an infinite loop. I will comment there about my findings in regards to that.

One further note: Instead of adding yet another entry to $install_state I considered something like a general translation_config key in anticipation of further related keys, such as the filename pattern noted above. However, we already have the 'server_pattern' key which should then go under that, as well, but we currently have no way of deprecating a key there. Also it might also make sense to provide some more generic integration between $install_state and $config from settings.php, but I wouldn't want to hold this up on that. So for now I think yet another key is the least bad option.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Oh and I didn't provide an interdiff to #2 because it's almost identical to the patch itself, so it wouldn't be of much use.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Opened #3182660: [PP-1] Make translation filename pattern configurable during install for the follow-up issue regarding the filenames. I will base that one on this.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

See #3182697: ComposerIntegrationTest fails under Merge request testing for the remaining test failure. This should be good for reviews, as far as I can tell.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new 2950407-8.9.x-14.patch10.64 KB

Here's a 8.9.x patch. Not sure if this has a change of being backported, so not running the tests as of now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

This doesn't apply to 9.1.x or 9.2.x so needs to be re-rolled. The solution might need a tweak since #3181644: PCRE library version 10.35 with pcre.jit=1 makes \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() regex misbehave has landed.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

timohuisman’s picture

timohuisman
Primary language Dutch
Location Leiden, Netherlands
commented
StatusFileSize
new 2950407_17.patch22.16 KB

We use composer patches to apply patches. Merge request patch urls are updated for every commit on the branch, which makes it a moving target. Attached is a patch containing the current state of the merge request (https://git.drupalcode.org/project/drupal/-/merge_requests/37.patch).

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

@alexpott there are some open threads if you could take a look.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For open threads in MR

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

arnested’s picture

arnested commented
StatusFileSize
new drupal-translations_directory_configurable-2950407-24.patch10.59 KB

Reroll.

andypost’s picture

andypost
he/him
Primary language Russian
commented

patches are no longer used to run tests, please update MR or create new one

arnested opened merge request !8742

arnested’s picture

arnested commented

I was about o, but got distracted. Pushed now.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Issue tags: +Needs issue summary update
Related issues: +#3089277: Provide core CLI commands for the most common features of Drush

Polished IS a bit but it needs more clean-up especially for CLI

andypost’s picture

andypost
he/him
Primary language Russian
commented
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

proper state as test added

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs change record

as the function argument changed issue needs change record

arnested’s picture

arnested commented

as the function argument changed issue needs change record

@tstoeckler already added a change record: https://www.drupal.org/node/3182651

smustgrave’s picture

smustgrave commented
Issue tags: -Needs issue summary update, -Needs change record

CR appears to be written and IS updated so removing those tags.

timohuisman’s picture

timohuisman
Primary language Dutch
Location Leiden, Netherlands
commented
StatusFileSize
new 2950407-35.patch11.17 KB

The https://git.drupalcode.org/project/drupal/-/merge_requests/8742 is still applicable, but because it's a moving target we prefer a patch as snapshot of the MR.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2950407-nr-bot.txt2.36 KB

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.

casey’s picture

casey commented
Status: Needs work » Needs review
StatusFileSize
new 2950407-37.patch10.42 KB

Rebased MR.

Attached snapshot of latest state of MR for safe usage with composer patches.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 2950407-nr-bot.txt3.04 KB

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.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.