When installing a fresh version of Drupal 8.2.0-beta2 (and beta3) the given site name is not showing. Instead it just says "Drupal" on the newly homepage.
MacOS Sierra, Apache 2.4.23, PHP 7.0.8. Safari 10.0 (build 12602). See attachment phpinfo. Site installation with language dutch (Nederlands).

Steps to reproduce it:

- Choose "Español" as site language.
- In the "Configure site" section, set the site name, mail, user and password as usual and in the regional options set "España" and "Europe/Madrid" and click in Save and Continue.
- When the install process finish the site name is "Drupal" and not the site name that you set during the install process.

I recorded a gif (drupal_site_name_issue.gif) with the process and the steps to reproduce it.

The system is a MacOS Sierra with php 7.0.12, sqlite, php built-in webserver and Drupal 8.2.1. I also tested with apache 2.4.18, mysql and OSX El Capitan and Ubuntu 16.04 as os with the same results.

CommentFileSizeAuthor
#103 2791405-2-103.patch4.91 KBalexpott
#103 100-103-interdiff.txt879 bytesalexpott
#100 2791405-2-99.patch4.91 KBalexpott
#100 98-99-interdiff.txt983 bytesalexpott
#98 92-98-interdiff.txt3.2 KBalexpott
#98 2791405-2-98.patch4.9 KBalexpott
#92 2791405-2-92.patch3.16 KBalexpott
#92 2791405-2-92.test-only.patch2.09 KBalexpott
#88 2791405-3-88.patch3.44 KBalexpott
#83 Choose_language___Drupal.png82.39 KBxjm
#58 Two-as-installed.png56.76 KBbsnodgrass
#58 as-modified.png47.35 KBbsnodgrass
#58 as-installed.png48.75 KBbsnodgrass
#57 Screenshot 2017-04-28 11.38.32.png63.3 KBmikeohara
#55 2791405-2-55.patch3.13 KBalexpott
#55 54-55-interdiff.txt865 bytesalexpott
#54 2791405-2-54.patch2.29 KBalexpott
#54 51-54-interdiff.txt2.08 KBalexpott
#51 2791405-51.patch2.26 KBalexpott
#51 50-51-interdiff.txt1.03 KBalexpott
#50 2791405-50.patch1.23 KBalexpott
#42 after.png34.43 KBlomasr
#30 drupal-site_name_not_saved-2791405-30.patch855 byteseyilmaz
#28 screenshot-2791405-28-2.png52.87 KBThew
#28 screenshot-2791405-28-1.png63.39 KBThew
#25 2791405-25.patch949 bytesThew
#23 2791405-23.patch949 bytesamit.drupal
#19 2791405-19.patch342 bytesflocondetoile
#9 drupal_site_name_issue.gif1.82 MBaralnoth
#5 Screen Shot 2016-10-17 at 11.42.55 AM.png140.64 KBcilefen
phpinfo().zip37.67 KBjaybee@baelemans.eu
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jaybee@baelemans.eu’s picture

Jaybee created an issue.
Edit 3th sept. If installing the default english everything is ok. When installing dutch (like I did) the name just changes to "Drupal"

aralnoth’s picture

I have the same issue. In my case, when I install a drupal site in spanish I see "Drupal" as a site name. When I install the site in english the site name is set properly.

cilefen’s picture

Have you got the configuration translation module installed?

aralnoth’s picture

No, I haven't installed it.

cilefen’s picture

Title: Site name » How to translate the site name
Version: 8.2.0-beta2 » 8.2.1
Category: Bug report » Support request
Status: Active » Fixed
Issue tags: -site
FileSize
140.64 KB

Site title is a configuration. Configuration Translation is required, and it works. From admin/config/system/site-information:

aralnoth’s picture

I don't want to translate the site name to any language. I want to install a site in spanish with the site name in spanish. So, when I install a new site, in the installation form I choose spanish as the site language, and then I set the site name that I want. When the install process is finished I found that the site name is always "Drupal". This happen only in drupal 8.2.x, when I make the same process in Drupal 8.0.x or 8.1.x I don't need content translation module to make this.

This happen when the install process is done by drush or by web.

cilefen’s picture

Title: How to translate the site name » When installing a site in a language besides English, the translated site name is
Category: Support request » Bug report
Status: Fixed » Active
Issue tags: +Needs steps to reproduce

Yes, forget about translation. I misunderstood the issue.

In terms of this being a bug, I cannot reproduce it. I just installed 8.2.1 via the web UI a site en Español with a site name that is different from "Drupal" and the name remains. If you can post the exact steps to reproduce in the issue summary, that will be great.

cilefen’s picture

Title: When installing a site in a language besides English, the translated site name is » When installing a site in a language besides English, the site name is not saved and reverts to "Drupal"
aralnoth’s picture

Issue summary: View changes
FileSize
1.82 MB
carstenG’s picture

I can confirm this behaviour...
I tried to install with german language only with drush site-install and via UI.
I tested on 8.2.1 and 8.2.4.

After installation the sitename is "Drupal". With english selected it works fine

eyilmaz’s picture

Reason: site name is translatable and gets overridden in the task install_finish_translations where the configuration gets updated via
Locale::config()->updateConfigTranslations($names, $langcodes).

eyilmaz’s picture

1 easy fix would be to make the site_name not translatable by adding translatable: false for name to system.schema.yml .
The result will be, that the site-name is not string translatable anymore, but via config_translation it should be possible to translate that anyway (?).

eyilmaz’s picture

alexpott’s picture

This is a bug - making the site name not translatable is not the way forward though. Not entirely sure of the best path here. The system.site created by the installer should be in the default language - ie. whatever is installed in. So it should just use what you've saved.

eyilmaz’s picture

In LocaleManagerConfig line 607+ we can see the comment:

          elseif (locale_is_translatable($langcode)) {
          // If the language code is the active storage language, we should
          // update. If it is English, we should only update if English is also
          // translatable.

where locale_is_translatable checks for

$langcode != 'en' || \Drupal::config('locale.settings')->get('translate_english')

I think this is related to English as default language for default configuration shipped by modules (?). So this should remain as it is ?.

So the solution could be, that
in line 582

$processed = $this->processTranslatableData($name, $active, $translatable, $langcode);

$processed should be system.site from installation and not the default from the module?
so we have to check the language in $active and if it is the same as $langcode & don't translate?

eyilmaz’s picture

linking the issue, where the translation update was implemented, for further information how it works.

flocondetoile’s picture

Same bugs here. Seems that this issue is a regression of #2457703: Default translatable site name is "Drupal" (incorrectly). The default config shipped in system module has now as default value for site name 'Drupal', and so this value is get and replace the site name set during the install process if the site is installed in an another language than engliish.

Trying to find the commit between 8.1.10 and 8.2.0 which replace in system.site the empty default value for the site name by 'Drupal'.

The simplier solution could be to remove this default value from system.site. But would be nice to see why this default value has been added.

flocondetoile’s picture

flocondetoile’s picture

Status: Active » Needs review
FileSize
342 bytes

I don't succeed to find the issue which introduce this default value.

This patch fix the issue on my locale instance (on a french install). Let see what the bot think about it.

Status: Needs review » Needs work

The last submitted patch, 19: 2791405-19.patch, failed testing.

flocondetoile’s picture

flocondetoile’s picture

And of course test failed, because it was introduced too in #2633170: Drupal 8 Install Page: Page title is not complete

amit.drupal’s picture

Status: Needs work » Needs review
FileSize
949 bytes

i hope its work.

Status: Needs review » Needs work

The last submitted patch, 23: 2791405-23.patch, failed testing.

Thew’s picture

Version: 8.2.1 » 8.2.x-dev
Status: Needs work » Needs review
FileSize
949 bytes

This should work now.

Status: Needs review » Needs work

The last submitted patch, 25: 2791405-25.patch, failed testing.

claudiu.cristea’s picture

Issue tags: +Needs tests

We should start with a test that proves the bug.

Thew’s picture

Using Drupal 8.2.5-dev,

  1. Choose ไทย as site language.
  2. Choose standard installation.
  3. I use mariadb and php7.0
  4. My site name is ทดสอบชื่อเว็บภาษาไทย (See screenshot-2791405-28-1.png)
  5. After installation the site name is still ทดสอบชื่อเว็บภาษาไทย (See screenshot-2791405-28-2.png)

I think this has been fixed now.

However, if I test in drupal 8.2.1 from https://www.drupal.org/project/drupal/releases/8.2.1, the bug exists.

denutkarsh’s picture

@Thew We still need to make tests for this...

eyilmaz’s picture

cilefen’s picture

Status: Needs work » Needs review
eyilmaz’s picture

Version: 8.2.x-dev » 8.3.x-dev
flocondetoile’s picture

Version: 8.3.x-dev » 8.2.x-dev

@eyilmaz You need to override too the site name set by the intallParameters() method which is set to 'Drupal' in the WebTestBase Class.

<del>$parameters['install_configure_form']['site_name'] = 'My Custom Drupal site'; </del>

For example

Sorry, I just saw the SiteNameTest Test...

flocondetoile’s picture

Version: 8.2.x-dev » 8.3.x-dev
eyilmaz’s picture

@flocondetoile SiteNameDifferentLanguageTest extends the class SiteNameTest, which already overrides the site_name with a random value.

eyilmaz’s picture

I have no idea why the test for 8.2.x passes. It should fail, as for 8.3.x. here : https://dispatcher.drupalci.org/job/default/289292/consoleFull

flocondetoile’s picture

This test failed well on my local instance on 8.2.4.

flocondetoile’s picture

I relaunched the test.

Status: Needs review » Needs work

The last submitted patch, 30: drupal-site_name_not_saved-2791405-30.patch, failed testing.

flocondetoile’s picture

But on my local instance 8.2.x, the test pass. Seems that @Thew is right. Seems that this issue has been fixed on the latest dev 8.2.x.

cilefen’s picture

Look through the commits on each branch and if necessary, https://git-scm.com/docs/git-bisect, to find out what is going on.

lomasr’s picture

FileSize
34.43 KB

Applied the patch in #30 . At the time of installation I gave the site name "Test Test" , But I am still getting 'Drupal' . Please see the screen.

eyilmaz’s picture

There is something wrong with the test. It doesn't install the site with the given language. Hiding the patch for now.

flocondetoile’s picture

@Iomasr: patch #30 is a test only patch to demonstrate the issue. It doesn't contain any fix.

lomasr’s picture

Thanks, Please mention in the comment in #30.

Didier Misson’s picture

Same problem.
D8.2.5 in French, and no way to change this TITLE "DRUPAL" ...
Thanks

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marcvangend’s picture

In case anyone needs a temporary workaround...
You can put a line like this in your settings.php:

$config['system.site']['name'] = 'My Drupal Site';

Hope that helps.

alexpott’s picture

Priority: Normal » Major

This definitely still happens on 8.3.x - I could go to admin/config/system/site-information and fix it though. Still user input is being lost so this is a major bug to me.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.23 KB

Here is a test. Drupal\system\Tests\Installer\InstallerTranslationMultipleLanguageForeignTest will fail.

alexpott’s picture

Here's a bugfix. Not 100% sure it is the right approach - this is a super complex area.

The last submitted patch, 50: 2791405-50.patch, failed testing.

alexpott’s picture

This bug was introduced by #2633170: Drupal 8 Install Page: Page title is not complete - which makes sense since this change introduced a default translation to maintain for system.site:name.

alexpott’s picture

I think this is a better fix. Because there really isn't a default site name and this also means that if a distribution like commerce is being installed its name is used in the page title.

alexpott’s picture

Let's also test that distribution names are used my appropriate.

mikeohara’s picture

Issue tags: +Baltimore2017

I am testing this as part of DrupalCon Baltimore 2017

mikeohara’s picture

I have tested this and it appears to be working in 8.4.x with patch defined in #55 steps as defined in the issue details.

bsnodgrass’s picture

FileSize
48.75 KB
47.35 KB
56.76 KB

1. Tested and duplicated in 8.4.x according to the steps in the issue summary.

using sitename "Español Install re:#2791405"
The sitename appeared as Drupal at installation (see as-installed.png) completion
using /admin/config/system/site-information I was able to change to the desired sitename (see as-modified.png)

2. Tested Successful with 2791405-2-55.patch (see Two-as-installed.png

There was a warning message during the install process on both attempts
warning:
Fueron descartados 2 textos de traducción porque contienen HTML no permitido o mal formado.Consulte el registro para más detalles.
translated:
2 translation texts were discarded because they contain not permitted or poorly formed HTML. See registration for more details.

bsnodgrass’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 55: 2791405-2-55.patch, failed testing.

idebr’s picture

Status: Needs work » Needs review

Closed #2884917: Site title is not set when installation uses non-English language as a duplicate issue.

It appears the patch fail in #60 was unrelated, since it subsequently passed. Let's see what the testbot has to say.

szeidler’s picture

I manually tested the installation #55 with the current 8.4.x snapshot and changing the site name for new languages is working like charm.

On a production site I needed to manually update the system.site config translation item in the database, because changes in the UI didn't have any effect on the <title>. The the configuration forms including admin/config/system/site-information/translate showed the designated value, but it didn't had any effect.

ckaotik’s picture

The the configuration forms including admin/config/system/site-information/translate showed the designated value, but it didn't had any effect.

This seems to happen when there is a translated configuration for the configuration's default language. The edit form will display the values of the configuration's default language, the translate form will display the translated values. If a translation for the default language exists (which should not happen!), it will always be used for display/output in that language and you are unable to edit it, because it is the default language for which no translation link is offered.

I think we should investigate this issue thoroughly, because this may be a general issue that affects all of Core and Contrib. Maybe someone can write a test for the scenario I described above, to verify this is isolated to system.site?

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

hhigel’s picture

Version: 8.5.x-dev » 8.3.5

Problem still exists with Drupal core 8.3.5.
Possible work-around: see https://www.drupal.org/node/2844755

cilefen’s picture

Version: 8.3.5 » 8.5.x-dev

There is no need to change the version. It's understood to be in the development branch.

vaplas’s picture

Can we open up a follow up for #63?

Manual testing:

  1. Installing no-english site (8.4.0-alpha1) without the #55 patch.
  2. Site name was lost.
  3. But no problem with <title> after change site name via UI (/admin/config/system/site-information).
  4. Installing another language.
  5. Go to page with translate UI (/admin/config/system/site-information/translate/LANGCODE/add). See "Drupal" as default value.
  6. Changing it and again no problems with the <title> for new language.

So, +1 to add #55 as is. It solves the problem with name for new sites. What is especially nice before the minor update.

gagarine’s picture

In my opinion this feel wrong:

-name: 'Drupal'
+name: ''

That mean we can't have default for configuration? Or can we confirm this is really only a problem during the installation?

gagarine’s picture

Issue tags: +Usability
cilefen’s picture

Issue tags: -Usability

I removed the usability tag, because adding it alerts the usability team. Their input is not needed on this issue. Usability is about interface design, etc. This is just a bug.

chegor’s picture

I think this bug should be closed as quickly as possible.
Drupal positions itself as a tool for multilingual sites (https://www.drupal.org/8/multilingual) and at the same time we have a bug in the heart of drupal

cilefen’s picture

Any community member wanting this fixed as quickly as possible may review the current patch following the instructions: https://www.drupal.org/patch/review

alexpott’s picture

@gagarine the point is that there really is no default site name - this is set during an installation. By providing a default in the config file we attempt to provide a translation for it during when translations are downloaded. "Drupal" translates into "Drupal" and this ends up overriding what the user has entered. So yes this is just because the configuration is being set during install which is a pretty special situation. See #14 and #15 for the bits of code involved and why this is the case.

axel.rutz’s picture

alexpott’s picture

Priority: Major » Critical

I'm bumping this to critical. If this happens to you the first time you install Drupal you are off to a very bad impression - Drupal has just lost your data input of the site name during installation!

alexpott’s picture

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

In #67 @vaplas asks for a follow-up for the comment in #63, but I don't think this is a followup. Reading #63 and #62 shows that we have a problem for existing sites. So we need an upgrade path to fix this.

alexpott’s picture

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

Hmmm... actually I need a bit more information about #62/#63 if I install a site in French the user input is lost. There is no way to fix anything because there is only one system.site config object and the title is set to Drupal. It would be great if @szeidler or @ckaotik could provide steps to reproduce. Here is what I did:

  1. Choose "Francais" during site install
  2. Used the minimal profile
  3. Entered "A french site" in the site name field during install
  4. Saw "Drupal" as the site title after install and confirmed that this was the value in system.site. Note that the site is monolingual and all configuration is in French
alexpott’s picture

Note that if I do add another language to the French site installed in #77 I'm able to translate the site name regardless of whether this patch is applied or not. As far as I can see this issue only comes about because of the special situation of writing system.site during the install and trying to apply translations to it because you are installing in another language - so for as far as I can see #62 and #63 are not related to this issue.

szeidler’s picture

Hei @alexpott,

I just tested the issue again with and without having the patch applied.

The use-cases with 2 enabled languages led to the problem, I described in in #62. But it happened only on a production site, that was initially build on Drupal 8.0, so went through the whole lifecycle since then. For new installed projects, since I discovered the issue, it never has been an issue. Unfortunately I don't have a database dump of that site at that specific time anymore.

Without the patch applied.

Use-case: Only 1 enabled language

  1. Install the site with language "Français"
  2. Use the minimal profile
  3. After the installation the sitename is reverted back to "Drupal"
  4. Go to "/admin/config/system/site-information" and change the Sitename
  5. It is working

Use-case: 2 enabled languages

  1. Install the site with language "Français"
  2. Use the minimal profile
  3. After the installation the sitename is reverted back to "Drupal"
  4. Enable the `config_translation` core module.
  5. Go to "admin/config/regional/language/add" and add "German" as an additional language
  6. Go to "/admin/config/system/site-information" and change the Sitename for both languages
  7. It is working

With the patch applied

Use-case: Only 1 enabled language

  1. Install the site with language "Français"
  2. Use the minimal profile
  3. After the installation the sitename was NOT reverted back to "Drupal"
  4. So everything's fine

Use-case: 2 enabled languages

  1. Install the site with language "Français"
  2. Use the minimal profile
  3. After the installation the sitename is NOT reverted back to "Drupal"
  4. Enable the `config_translation` core module.
  5. Go to "admin/config/regional/language/add" and add "German" as an additional language
  6. Go to "/admin/config/system/site-information" and change the Sitename for both languages
  7. It is working
alexpott’s picture

@szeidler thanks for posting all the steps you've took to review the patch - nice work. Do you think given your testing you have the confidence to rtbc this? It'd be great to get this fixed because as noted in #75 this makes for a very bad first impression because we lose your data in the installer.

szeidler’s picture

Status: Needs review » Reviewed & tested by the community

I once again read through the feedback since you provided patch #55 and yes, I think we can move it confidently to RTBC.

If someone will be able to reproduce #62, #63 from and older living installation again, we should tackle it in a separate issue.

xjm’s picture

Priority: Critical » Major

I don't think this really meets the criteria for a critical issue as there's no data lost under normal site operation. I think it can be marked major for one piece of data being lost under certain circumstances in the installer.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record
FileSize
82.39 KB
+++ b/core/includes/theme.inc
@@ -1322,6 +1322,12 @@ function template_preprocess_html(&$variables) {
 
+  // Early in the installer the site name is unknown. In this case we need to
+  // fallback to the distribution's name.
+  if (empty($head_title['name']) && drupal_installation_attempted() && function_exists('drupal_install_profile_distribution_name')) {
+    $head_title['name'] = drupal_install_profile_distribution_name();
+  }

+++ b/core/modules/system/config/install/system.site.yml
@@ -1,5 +1,5 @@
-name: 'Drupal'
+name: ''

At first I thought this was changing it to display e.g. "Standard" for install profiles in the core distribution, but I checked and confirmed we have this in drupal_install_profile_distribution_name():

  return isset($info['distribution']['name']) ? $info['distribution']['name'] : 'Drupal';

I also double-checked that we have different test coverage asserting that English still sees "Drupal" (versus an empty string or something)

core/modules/system/src/Tests/Installer/InstallerTest.php:    $this->assertTitle('Choose language | Drupal');

And tested it manually to ensure there are no UI changes to the install process changed for English:

(Etc.)

So all that looks good, but I'm wondering if we should have a change record? I don't think anything should change for normal core installation workflows as at the end of the day it's just a default config value, but I could imagine some workflow might be pulling a default from this configuration (for a required field) and now just get an empty string.

xjm’s picture

It also feels really off that the fix for this bug is changing the value of the default config versus something actually in the installer; maybe that's why I'm hesitating about the CR and whatnot.

alexpott’s picture

Due to #84 I had another think about the fix and pondered when once the installer was done you can change the site name and it not get reverted when any new translations are processed by the locale config translation import. Following this thought process I found \Drupal\locale\LocaleConfigSubscriber::onConfigSave()

  /**
   * Updates the locale strings when a translated active configuration is saved.
   *
   * @param \Drupal\Core\Config\ConfigCrudEvent $event
   *   The configuration event.
   */
  public function onConfigSave(ConfigCrudEvent $event) {
    // Only attempt to feed back configuration translation changes to locale if
    // the update itself was not initiated by locale data changes.
    if (!drupal_installation_attempted() && !$this->localeConfigManager->isUpdatingTranslationsFromLocale()) {
      $config = $event->getConfig();
      $langcode = $config->get('langcode') ?: 'en';
      $this->updateLocaleStorage($config, $langcode);
    }
  }

Which is the nub of the problem. Under non install site operation we mark config has have a custom translation here but because of the if (!drupal_installation_attempted()... we're not doing this when we save system.site in \Drupal\Core\Installer\Form\SiteConfigureForm::submitForm(). This was added in #2468767: English config source strings are saved as custom translations in locale in foreign install so it's not as simple as just removing the !drupal_installation_attempted().

alexpott’s picture

What we need to happen is that when the installer is saving user entered config the LocaleConfigSubscriber needs to behave as though it is not during an install but all other times it needs to behave as it does now.

@xjm is correct to be suspicious of the earlier fix because any distribution could add another form to the installer that would have exactly the same problem.

alexpott’s picture

I've spent some time in the debugger today trying to work out if there is a better fix. I'm not sure that there is. Here's why:

  1. The SiteConfigureForm configures system.site and when you are installing in French you'll enter a French title.
  2. The SiteConfigureForm also will install the Update module and File module depending on what the user ticks.

Both of these actions cause configuration saves. For system.site save you want the LocaleConfigSubscriber to fire as though the user is doing this a regular runtime. For the config saves caused by the module install you want the LocaleConfigSubscriber to behave as though we're in the installer because config translations have not been downloaded yet - this happens after the SiteConfigureForm has been submitted in the install_finish_translations step.

Really tricky.

alexpott’s picture

Given #87 and the concerns about the previous approach all I can think of is to special case system.site. This means distributions that allow users to configure translatable configuration during installation where that config are still going to have an issue but there's not much we can do about that.

No interdiff because it is a different approach - tests are the same though.

Gábor Hojtsy’s picture

Discussing with @alexpott the idea to split the installer steps came up. I think that would be a "workaround" at a once used place while baking this into an onConfigSave() to run all the time looks like less ideal to me. I agree we need to work around this in some way, I don't have very strong feelings against this patch either, just noting.

Status: Needs review » Needs work

The last submitted patch, 88: 2791405-3-88.patch, failed testing. View results

Lennard Westerveld’s picture

I have this issue also not related to the installation of Drupal.
Also when installing a new language and save the site config page again and the problem is there again.
See also https://www.drupal.org/forum/support/post-installation/2017-01-18/site-n...

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
2.09 KB
3.16 KB

I think looking at the two patches #88 and #55 I prefer the one in #55 because there really is not a default for site name. It is something that is set up during installation. It happens that removing the default fixes the specific problem with \Drupal\locale\LocaleConfigSubscriber and the difference between something being configured during an install and something being installed during an install and we should open a follow up to discuss potential solutions that are generic and not based on special casing like the patch in #88. I don't think a CR is necessary here because nothing is really changing - other installers like Drush already provide a default for the site name because even in a non-interactive install you have to provide one because non-interactive install completes the site configure form too.

Re-uploading the patch in #55 and a test-only run.

Regarding the criticality of this issue: the installer is many people's first impression of Drupal if you are installing in a language other than English we lose your site name. In my mind that sets a very bad impression as why would you trust Drupal to look after your data after that?

alexpott’s picture

Created #2925203: LocaleConfigSubscriber can result in data loss during install as a followup to address the issue with LocaleConfigSubscriber

The last submitted patch, 92: 2791405-2-92.test-only.patch, failed testing. View results

vaplas’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott, thank you very much!

We already have at least 3 RTBC to #55 in #59, #67, #81. So, back RTBC to #92.

I fully support that the problem is serious. Possible examples of disappointment (duplicate previous posts):

  1. When I type a long site name, I do not want to lose my work.
  2. If I did not immediately notice the loss of the title, I do not want to discover this loss suddenly.
  3. I do not want to waste time figuring out the causes of discrepancies site name.
  4. If my data disappears already in the first step of working with CMS, then I begin to doubt that I made the right choice.
catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/theme.inc
@@ -1322,6 +1322,12 @@ function template_preprocess_html(&$variables) {
+  if (empty($head_title['name']) && drupal_installation_attempted() && function_exists('drupal_install_profile_distribution_name')) {

Not really keen on the installer logic added in template_preprocess_html().

If #2925203: LocaleConfigSubscriber can result in data loss during install goes in wouldn't this be unnecessary?

alexpott’s picture

+++ b/core/includes/theme.inc
@@ -1322,6 +1322,12 @@ function template_preprocess_html(&$variables) {
+  // Early in the installer the site name is unknown. In this case we need to
+  // fallback to the distribution's name.
+  if (empty($head_title['name']) && drupal_installation_attempted() && function_exists('drupal_install_profile_distribution_name')) {
+    $head_title['name'] = drupal_install_profile_distribution_name();
+  }

Atm we fallback to the default declared in system.site.yml - with this change we get round the fact there is no default and we also improve the situation for distributions because they are no longer saying "Drupal" when ideally they'd want their name in the title. I can't really think of a way around this because we're directly reading system.site in at this point. See https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/t... - it is not pretty.

alexpott’s picture

@catch suggest trying to use configuration overrides - seems to work - neat idea :)

catch’s picture

Status: Needs review » Reviewed & tested by the community

That looks better, installer is horrible still but at least it's contained.

alexpott’s picture

Some coding standards things I noticed :(

Fingers-crossed we're green. Testing locally showed the one testing the distribution name passed.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Installer/ConfigOverride.php
@@ -0,0 +1,62 @@
+      // Early in the installer the site name is unknown. In this case we need to
+      // fallback to the distribution's name.

One more coding standards issue - comment longer than 80 chars. Will roll a new patch once we know if the approach is green on all core tests.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 100: 2791405-2-99.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
879 bytes
4.91 KB
Search.Drupal\search\Tests\SearchConfigSettingsFormTest
✗	
findFile
exception: [Warning] Line 128 of vendor/symfony/class-loader/ApcClassLoader.php:
apcu_store(): Unable to allocate memory for pool

Is an unrelated test fail.

Here's the coding standards fix patch...

Gábor Hojtsy’s picture

  • Gábor Hojtsy committed dad7955 on 8.5.x
    Issue #2791405 by alexpott, Thew, flocondetoile, eyilmaz, bsnodgrass,...
Gábor Hojtsy’s picture

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

Yay, looks fantastic. I agree this approach looks even better than before. Well contained in the override service. Will check with other committers about backportability but I see no reason not to, this only involves new installs so for existing sites this should not be an issue. I was pondering a bit about what would it cause to existing sites if we change the default name here, but I could not come up with any reasonable real life scenario where this could be an issue.

  • Gábor Hojtsy committed bc9d792 on 8.4.x
    Issue #2791405 by alexpott, Thew, flocondetoile, eyilmaz, bsnodgrass,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Merged to 8.4.x after discussing with @catch.

vaplas’s picture

Yay! 🎉🎉🎉

Cool patch and commit to 8.4! Great thanks!

Status: Fixed » Closed (fixed)

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