Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
For Umami to work with multilingual content (initially with Spanish content), we need to be able to import multilingual content.
Few steps need to be completed in order to achieve that:
- The existing CSV content files in English should move to a structure that can support future languages (3028627
- Enable multilingual functionality as part of Umami install profile3029839
- Translated content in Spanish need to finalized in order to be part of OOTB 3028769
While this process might take time, there are other multilingual configurations and settings that need to be addressed.
Proposed resolution
- Creating a patch [WIP] of "good enough" patches (instead of waiting for them until they are finalized)
- Creating dummy Spanish content
- Updating
InstallHelper.phpto support multilingual import
That last step of updating InstallHelper.php would be the only real patch that would eventually be committed to core.
The [WIP] patches are there to allow other people to easily test multilingual capabilities with Umami.
| Comment | File | Size | Author |
|---|---|---|---|
| #64 | interdiff_62-64.txt | 544 bytes | shaal |
| #64 | umami-import-multilingual-content-64.patch | 73.3 KB | shaal |
| #63 | Screen Shot 2019-03-22 at 20.31.26.png | 117.75 KB | kjay |
| #62 | interdiff_61-62.txt | 569 bytes | shaal |
| #62 | umami-import-multilingual-content-62.patch | 73.36 KB | shaal |
Comments
Comment #2
shaalInitial patch for testing, by applying it Umami install profile will import English content + dummy Spanish content.
Comment #3
shaal(checking if it will pass tests)
Comment #4
Gábor HojtsyShould be rerolled on top of #3028627: Restructure Umami's default content files so more than one language can be included.
Comment #5
shaalInterdiff was not possible with the previous patch #2.
Changes in this patch:
Todo:
Move blocks' content from
InstallHelper.phpinto external English CSV files, and then get block content get translated as well (ie. text on hero image, footer, etc.)There's a decision that needs to be made / issue that needs to be resolved.
When the user chooses a 3rd language (not English and not Spanish), how should we handle imported multilingual content?
When installing Umami in English/Spanish, content will always be available in English and Spanish, ie:
drupalsite.local/en/awesome-recipeand
drupalsite.local/es/awesome-recipeBut when installing Umami in 3rd language how should we import the English content?
Here's what will happen with each option (only relevant when Umami is installed in a 3rd language, I'll use
heas an example):1) as default (3rd language) content:
drupalsite.local/awesome-recipe- worksdrupalsite.local/he/awesome-recipe- worksdrupalsite.local/es/awesome-recipe- worksdrupalsite.local/en/awesome-recipe- doesn't work2) as English content
drupalsite.local/en/awesome-recipe- worksdrupalsite.local/es/awesome-recipe- worksdrupalsite.local/awesome-recipe- doesn't workdrupalsite.local/he/awesome-recipe- doesn't work3) as both default and English
drupalsite.local/en/awesome-recipe- worksdrupalsite.local/es/awesome-recipe- worksdrupalsite.local/awesome-recipe- worksdrupalsite.local/he/awesome-recipe- works=====
But the best solution would actually be importing English as English and Spanish as Spanish, yet somehow still allow no-language-prefix to work as well -
drupalsite.local/awesome-recipe- would display content in English(I don't know what settings are needed to make that best solution to work)
Comment #6
Gábor HojtsyYes this would be the best. You can set the English prefix to be empty if the site was not installed in Spanish by default. So the
/whateverURLs would always be English even if you install in Italian or Uyghur. This is a bit different from default Drupal behaviour but it can be configured this way and it makes sense for Umami since we don't have translation to all the languages.Comment #7
Gábor HojtsyHere is a drawing form :)
In other words if you install in a language that has default content in that language, then make that language path prefix empty (as is Drupal's default), otherwise make English path prefix empty and the language you installed in not empty.
Comment #8
shaalHere are a few scenarios that users expect.
Installing Umami in English, expecting:
/English interface, English content/enEnglish interface, English content/esSpanish interface, Spanish contentInstalling Umami in Spanish, expecting:
/Spanish interface, Spanish content/enEnglish interface, English content/esSpanish interface, Spanish contentInstalling Umami in Hebrew, expecting:
/Hebrew interface, English content/enEnglish interface, English content/esSpanish interface, Spanish content/heHebrew interface, Hebrew content(the last
/heshould displayPage not foundmessages since there is no content in that language, until the user will add some)Comment #9
shaalI think the simplest (and best?) solution,
is importing English content as English. Leaving all rest of Drupal settings as default.
Then installing Umami in a 3rd language - would work as expected.
It works like the scenarios detailed in #8
One thing that is left to figure out is a View's setting that is missing
(we need that setting for a multilingual website regardless of this specific patch)
Can we create in the View a condition "If no content in the current language - display English content"?
Comment #10
shaal(bad patch, please ignore)
Comment #12
shaalSomething went wrong in the last patch I submitted... I think merging latest 8.7.x made a little mess.
Because 8.7.x includes Umami Tour - generating interdiff was not possible.
Patch now supports importing multilingual blocks!!
I added the CSV files needed for English block, as well as dummy CSV content in Spanish.
Looking forward to the completion of #3029839: Multilingual functionality - enable English and Spanish as part of Umami installation
Once that other issue is fixed - this patch can become much smaller (currently it's providing the missing functionality that will be included in that other issue)
Comment #14
shaalFixed coding standard issues
Fixed typo in block import that prevented footer block to get the image.
Comment #15
shaalI just realized... that if we won't have ALL content translated from English to Spanish, we should change the structure of CSV files to include an internal ID (like what @smaz did for taxonomy files), and update
installHelper.phpto support importing content and translation by ID instead of (currently) by row of CSV file.Remaining tasks
installHelper.phpto import content and translation by ID instead of by rowAlso, need to change the structure of dummy content CSV files in Spanish to be the same as the new structure of CSV files in English.
Comment #16
Gábor Hojtsy@shaal good thinking about #15.
Also #3029839: Multilingual functionality - enable English and Spanish as part of Umami installation landed, so that patch of the patch will not apply anymore.
Comment #17
shaalThis patch is kind of a complete rework...
Including:
installHelper.phpcan now import content in any language, the minimum requirements for that to happen:/heComment #19
shaalFixed coding standard issues, and fixed the test to work with the new CSV files locations.
Comment #20
shaalThis patch is based on #19 and includes the following changes:
I don't know why that last item, with the config files translations, is still not taking effect during installation.
Comment #21
shaalI applied patch #135 in https://www.drupal.org/project/drupal/issues/2599228#comment-13015153
The good - Not getting an error when allowing content types to be translatable
The bad - the config entities that have field label translations - are not shown in Spanish when the site is installed
Comment #22
shaalPatch #20 with coding standard fixes
Comment #23
Gábor HojtsyNone of these should be shipped in umami, they would be downloaded from drupal.org for all the config that is shipped as yml files within umami once drupal.org translates the fields. They should not be shipped in umami.
Let's make sure there are newlines at end of files :)
Comment #24
shaalThis patch includes all suggestions from #23 + updated contnet for articles CSV files in Spanish.
Comment #25
smazI haven't fully reviewed the code but:
1) There's a PHP 7.0 incompatibility:
[$all_content, $translated_languages] = $this->readMultilingualContent($filename);That shorthand for list was introduced in PHP 7.1: https://wiki.php.net/rfc/short_list_syntax
If core is still supporting 7.0, we'd need to change that to:
list($all_content, $translated_languages) = $this->readMultilingualContent($filename);2) I get the following during install (installing via drush)
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] Invalid argument supplied for foreach() InstallHelper.php:678
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] array_combine() expects parameter 1 to be array, null given InstallHelper.php:181
[warning] Invalid argument supplied for foreach() InstallHelper.php:678
I'm guessing this is the blank lines at the end of CSVs? If so either a) we need to be allowed to have no newlines at the end of CSV files or b) our import code needs to check it's not a blank line.
3) 3 field configs now have:
Is the configuration for all fields in another patch? If so, those shouldn't be in this patch or we'll need to re-roll it when the other one is added.
Comment #26
Gábor Hojtsy#2599228: Programmatically created translatable content type returns SQL error on content creation landed, so this can continue working without outside dependencies now.
Comment #27
shaalBased on patch #24 with these modifications:
Enable Recipe, Article, and Page content types + taxonomies to be translatable (You will see a "Translate" tab when viewing content)
The config files that have
Translatable: truesetting the fields that can be translatable, for an example in Recipe:Ingredients, Recipe Instruction, Summary.
While the config files with
Translatable: falsesetting the fields that should not be translatable, such as Tags (The tag translation happens in the tag content itself, the Tag ID stays the same in both languages).Comment #28
shaalApplying #25 in order to support PHP 7.0
Comment #31
shaalImport Spanish block content, connect block content with nodes through internal id (from CSV).
The 2 hero blocks (homepage and /recipes page) - are always in English, and wouldn't switch to Spanish.
Comment #33
Gábor HojtsyThe hero block type is not configured to be translatable for most of its fields:
The other block types seem to be fine, but its worth double-checking them :)
Comment #34
shaal@Gabor, thank you for these finding #33!
I looked into it and found 5 block fields that were not set as translatable.
Updated Spanish recipes of
Vegan chocolate and nut browniesandFixed code standards.
oops, I see now that I left a commented line by mistake, it's marked with
////1 remaining task:
Update the Test to be aligned with Umami's multilingual changes.
Comment #36
smazThis should fix the failing tests due to config mis-matching & the coding standards issue (just removed the commented out line).
Comment #37
smazSetting to needs review to trigger testbot.
Comment #39
smazFixing the new failures hopefully!
CSV headers now don't just try to get it from the english file.
Removed unused variable.
Comment #40
kjay CreditAttribution: kjay commentedI'm having a stab at recreating the patch on behalf of @smaz based on the interdiff he posted in #39.
Comment #42
shaal@smaz re: #39, Why was this bracket removed?
Comment #43
shaalThis patch includes #40 and the following updates:
Comment #44
shaalFixed partial translation of Spanish recipe content.
Comment #45
kjay CreditAttribution: kjay commentedThis is absolutely amazing work and it's so good to see this coming together.
This patch installed no problem for me and I was able to navigate around the translated content. The views provided the expected behaviour of offering less content when in Spanish (only offering the content actually translated).
A few minor issues that I noticed:
Comment #46
shaalThank you @kjay !
I fixed the content and HTML typos as described in #45 + couple more I have found.
Re: 2)
Must be an old installation.
That directory is no longer in core and the files that used to be there were moved into
..../default_content/languages/en/article_bodyComment #47
shaalRemoved a space in the Spanish about page URL which made the link invalid.
Comment #48
Gábor HojtsyNeeds a reroll now that the content patches got committed.
Comment #49
shaalReroll for patch #47.
Removed all Spanish CSV files from the patch.
Comment #50
rteijeiro CreditAttribution: rteijeiro at 1xINTERNET commentedThings I noticed:
Anything else I should test?
Comment #51
shaalThank you @rteijeiro !
You were testing the lean version of the import patch, which no longer contains CSV Spanish translations in it and depends on CSV's available in core.
When you tested this Blocks and Recipes were not in core yet - so you would only see the English version of the missing Spanish content.
In meantime - Block CSV patch got into core and very soon Recipes CSV patch should get into core as well.
Let's review this again as soon as #3038309: Spanish translations of Umami demo recipes got into core.
Comment #52
Gábor HojtsyBoth of those are in now, @rteijeiro please retest. I think there will be somewhat different results, but some of the issues you found should still apply.
Comment #53
smazI'm looking at the code, I haven't checked the resulting content:
importContentFromFile is called 8 times, and each time it will read/process the directories for what languages are there. Can this be refactored to only read the filesystem once?
Perhaps we should use https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Language!Language... to get languages instead.
In readMultilingualContent, we have two foreach loops. We might be able to just use one:
Can we do the $keyed_content part here?
A few other minor things:
getNodePath: $langcode, $content_type missing from docblock above
saveNodePath: $langcode, $content_type missing from docblock
processPage: unused $module_path argument
processRecipe: $langcode missing from docblock
processArticle: $langcode missing from docblock
processBannerBlock: $langcode missing from docblock
processDisclaimerBlock: No docblock, unused $module_path argument, function shouldn't have a blank line before $values =... I think
processFooterPromoBlock: No docblock
processContent: Docblock says 'Imports content', but this doesn't - it processes the content to be a structured array?
importContentFromFile: $ct_machine_name is not explained in the docblock
Comment #54
kjay CreditAttribution: kjay commentedThis patch installs well and I focused on reviewing the content installed and the behaviour of the site, which worked really well :) This is great work!
Unfortunately I did experience an issue with installation when using the interface. Installing with Drush seemed to work fine but with the interface there is a warning generated at the Configure site stage and it persists through the installation phase - see screenshot attached. Is this just happening for me?
Once installed, the front page fails to load, I got a 404 unfortunately. This was cleared with by clicking on home. See the issue in the attached screenshot:
The only code issue I spotted were the spaces between the opening and closing p tags in core/profiles/demo_umami/modules/demo_umami_content/default_content/languages/es/node/page.csv line 2.
Comment #55
shaalThank you @smaz and @kjay !
#53
Now using the API instead of figuring out languages to import according to existing directories.
readMultilingualContent function - is now adding the header in 1 loop instead of running a loop again.
Added missing entire docblocks and missing docblock pieces.
#54
That warning message during installation is a core issue, @gabor created a separate issue to solve that.
I couldn't reproduce getting a 404 page error after installation. Can you please provide steps to reproduce?
+ I added some extra performance improvements.
Comment #56
shaalThanks @kjay
Fixed 2 trailing whitespaces.
Comment #57
kjay CreditAttribution: kjay commentedThe patch on #56 installs without warnings or errors for me. I installed using the standard profile selection form and with Drush - both worked great with only the issue of the warning about 2 translation strings that @shaal has confirmed as being a core issue being addressed elsewhere.
I installed 3 times and didn't have a repeat of the 404 for the front page and this was probably just an error of how I was doing a previous test.
I visited all the pages in both English and Spanish and all worked as expected.
This is a fantastic feature for Umami - marking RTBC.
Comment #58
Gábor HojtsyOverall great stuff! Sorry, but found a bunch of code style / phpdoc issues :)
First line should wrap to 80 chars (before "created").
Should not be title cased.
An array with two items. The first item is an array of ..., the second item is an array of ....
Don't capitalize Node.
Should not have empty lines between these.
If this node ID is an ID from the CSV file indeed, can we call it "node CSV ID" or somesuch to distinguish it clearly from node id as understood by Drupal? This would likely need to change elsewhere.
alias url => URL alias
Extra empty line that does not even have a *
Here and elsewhere in sentences, don't capitalize Field.
Remove empty line between @params
Should not have empty line.
Entity type...
Should not have extra newline.
Missing explanation for bundle machine name
Comment #59
smazA couple more to add:
1) Now that we have gone from the two for...each loops in readMultilingualContent, we don't need this else statement anymore:
2) We should be using Dependency Injection for the language manager service. We can also improve this to only get the languages once, rather than every time we call readMultilingualContent().
This would need:
In create:
In __construct, add the language manager argument that's now being passed in:
Then when we need to use the languages, we change from:
to:
Comment #60
smazActually, my first point above may not be correct as `importContentFromFile` uses the returned $translated_languages.
Comment #61
shaalThank you @gabor and @smaz !
#58
I cleaned and organized all the comments.
I changed
$node_idto$node_csv_id#59
getLanguages() is now called only once at the beginning and then saved into a variable.
Comment #62
shaalMinor capitalization fix.
Comment #63
kjay CreditAttribution: kjay commentedI've reviewed the patch in #62 and the patch installs Umami no problem, whether by drush or profile selection screen.
I've checked every page in both English and Spanish and all look to be working as expected.
A review of the code (for formatting) all looks good with the exception of one possible issue, see the following screenshot of warning that PHPStorm flags for me - that a parameter description is offered for a missing parameter.
Comment #64
shaalThank you @kjay
#63
Yes, that
$langcodewas no longer needed in that function. I removed the extra comment.Comment #65
kjay CreditAttribution: kjay commentedThe patch in #64 takes care of the redundant comment and installs great. Great work! Going for another RTBC :)
Comment #67
Gábor HojtsySuperb, thanks all! Committed to 8.8 and 8.7!