As part of #1830588: [META] remove drupal_set_title() and drupal_get_title(), all calls to drupal_set_title() in the installation / update system should be converted to use #title in a render array. For the purposes of this issue, that also includes the batch and error systems.

Affected files are:
core/includes/batch.inc
core/includes/errors.inc
core/includes/install.core.inc
core/includes/update.inc
core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
core/update.php
core/modules/system/system.api.php (hook_install_tasks docblock)

CommentFileSizeAuthor
#30 install.title_.30.patch32.03 KBsun
#26 install.title_.24.patch32.03 KBsun
#24 install-no-profiles.png15.74 KBsun
#24 interdiff.txt1.79 KBsun
#17 interdiff.txt5.48 KBsun
#17 install.title_.17.patch30.98 KBsun
#15 interdiff.txt4.56 KBsun
#13 interdiff.txt26.21 KBsun
#13 install.title_.13.patch30.45 KBsun
#7 remove-drupal_set_title-2192649-7.patch14.46 KBchakrapani
#2 2192649-2.patch14.37 KBsidharthap

Comments

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Issue summary: View changes
sidharthap’s picture

sidharthap
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2192649-2.patch14.37 KB

Here is the first patch file.

Status: Needs review » Needs work

The last submitted patch, 2: 2192649-2.patch, failed testing.

sidharthap’s picture

sidharthap
Primary language English
commented

All this drupal_set_title has been addressed is different issues. In my D8 fresh instance i did not encounter drupal_set_title() in any of the files as mentioned.
Please close this ticket marking duplicate.

Thanks

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

@sidharthap: I think you must have your patch applied or something, I still see drupal_set_title in the repository. For example http://drupalcode.org/project/drupal.git/blob/f5ba64c9c190d7836a48660d1f...

sidharthap’s picture

sidharthap
Primary language English
commented

ahh, i created the patch from fresh D8 copy. while applying the patch shows error.
error: patch failed: core/includes/batch.inc:107
error: core/includes/batch.inc: patch does not apply

is it related to my D8 instance or something else ?

chakrapani’s picture

chakrapani commented
Status: Needs work » Needs review
StatusFileSize
new remove-drupal_set_title-2192649-7.patch14.46 KB

Here is the re-roll from #2.

Status: Needs review » Needs work

The last submitted patch, 7: remove-drupal_set_title-2192649-7.patch, failed testing.

mfernea’s picture

mfernea commented

I'm trying to see what's the problem here.

mfernea’s picture

mfernea commented

Having a look at the install or update processes the current patch doesn't really work. It uses the standard method of setting the title from https://drupal.org/node/2067859 or other similllar tickets, but here that doesn't work.

On the other hand Drupal installs just fine either by web interface or using drush.

vishalgala’s picture

vishalgala commented
Assigned: Unassigned » vishalgala
vishalgala’s picture

vishalgala commented
Assigned: vishalgala » Unassigned
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: Unassigned » sun
Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new install.title_.13.patch30.45 KB
new interdiff.txt26.21 KB

This requires a lot more adjustments. As I'm apparently becoming an install system expert, I'm going to take this over.

  1. Fixed instalL_display_output(), Batch API, and update.inc to use proper render arrays.
  2. Incorporate prior art from #2194533: "Drupal is already installed" exception is printed like any other exception + enhance it for render pipeline.
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

13: install.title_.13.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new install.title_.15.patch30.9 KB
new interdiff.txt4.56 KB

@dawehner had a cursory look at this and mentioned that the usage of the string translation service in the installer exceptions is a bit weird.

So I've changed that to follow existing practices in core.

dawehner’s picture

dawehner
Primary language German
commented

First note: No visual change while running tests in seven.
There aren't visual artefacts in the installer. 

  1. +++ b/core/includes/batch.inc
@@ -47,42 +47,38 @@ function _batch_page(Request $request) {
 
+  $build = array();
+
   // Add batch-specific CSS.
-  $attached = array('#attached' => array('css' => array()));
   foreach ($batch['sets'] as $batch_set) {
     if (isset($batch_set['css'])) {
       foreach ($batch_set['css'] as $css) {
-        $attached['#attached']['css'][$css] = array();
+        $build['#attached']['css'][$css] = array();
       }
     }
   }
-  drupal_render($attached);

    OT: This old strategy of calling drupal_render on anything just opens up new issues for all of the instances. this is pretty sad to be honest. 

  2. +++ b/core/includes/install.core.inc
@@ -89,12 +92,23 @@ function install_drupal($settings = array()) {
+    if (!$install_state['interactive']) {
+      throw $e;
+    }

    It would be kinda helpful to explain why we rethrow it in this case. This isn't obvious for me. 

  3. +++ b/core/includes/install.core.inc
@@ -1303,13 +1327,10 @@ function install_select_profile(&$install_state) {
+        return drupal_get_form('install_select_profile_form', $install_state);

@@ -1549,10 +1571,7 @@ function install_select_language(&$install_state) {
+      return drupal_get_form('install_select_language_form', count($files) > 1 ? $files : array());

    Do we have the form builder service available at that point of the installer? 

  4. +++ b/core/includes/install.core.inc
@@ -2184,7 +2185,11 @@ function install_finished(&$install_state) {
+    '#title' => t('@drupal installation complete', array('@drupal' => drupal_install_profile_distribution_name())),

    Out of scope: This placeholder is quite odd. 

  5. +++ b/core/includes/install.core.inc
@@ -2512,7 +2517,7 @@ function install_check_requirements($install_state) {
+ * @thows \Drupal\Core\Installer\Exception\InstallerException

    We should fix @thows and use @throws directly. 

  6. +++ b/core/modules/system/templates/install-page.html.twig
@@ -12,14 +12,14 @@
 #}
 <!DOCTYPE html>
-<html lang="{{ language.langcode }}" dir="{{ language.dir }}">
+<html{{ html_attributes }}>
...
 </head>
-<body class="install-page">
+<body class="{{ attributes.class }}">
 

+++ b/core/themes/seven/templates/install-page.html.twig
@@ -13,14 +13,14 @@
 <!DOCTYPE html>
-<html lang="{{ language.langcode }}" dir="{{ language.dir }}" class="install-background">
+<html{{ html_attributes }} class="install-background">
 <head>
   {{ head }}
   <title>{{ head_title }}</title>
   {{ styles }}
   {{ scripts }}
 </head>
-<body class="install-page">
+<body class="{{ attributes.class }}">
 
 <div class="l-container">

    Do we really need such changes here? 

  7. +++ b/core/update.php
@@ -225,14 +231,19 @@ function update_access_denied_page() {
-  drupal_set_title('Access denied');
...
+    '#title' => 'Access denied',

    Should/Can we translate that bit? 

  8. +++ b/core/update.php
@@ -452,9 +463,16 @@ function update_task_list($active = NULL) {
+      // $output has to be rendered here, because the maintenance page template
+      // is not wrapped into the html template, which means that any #attached
+      // libraries in $output will not be loaded, because the wrapping HTML has
+      // been printed already.
+      '#content' => drupal_render($output),

    It is great to have an explanation here.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new install.title_.17.patch30.98 KB
new interdiff.txt5.48 KB

Fixed the actionable issues from @dawehner's review.

re 3) Yes, FormBuilder is available at install time. The installer environment is not changed by this patch.

re 6) It took me an entire hour to just find Seven's install-page.html.twig template override, so at minimum, I want to move the template into its ./templates directory here. — The fixes to the template itself are technically not really required here and only caused by further debugging, but they are certainly worth to do.

re 7) Previous patches in here added t() around the strings in update.php, but I intentionally reverted those, because the special update.php environment is similarly complex as the installer environment, and I'm not very familiar with the update.php environment. This patch solely focuses on using #title instead of drupal_set_title(). Wrapping those strings into t() should be considered in a dedicated separate issue.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

The fixes to the template itself are technically not really required here and only caused by further debugging, but they are certainly worth to do.

You probably can't argue against that, though at least from a theoretical standpoint the intersection between people knowing how the installer works and know how to proper write templates so that themes can leverage
them might be pretty small.

re 7) Previous patches in here added t() around the strings in update.php, but I intentionally reverted those, because the special update.php environment is similarly complex as the installer environment, and I'm not very familiar with the update.php environment. This patch solely focuses on using #title instead of drupal_set_title(). Wrapping those strings into t() should be considered in a dedicated separate issue.

That is totally fine!

+++ b/core/includes/install.core.inc
@@ -101,6 +101,7 @@ function install_drupal($settings = array()) {
+    // In the non-interactive installer, exceptions are always thrown directly.

Thank you! This explains stuff.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks for reviewing this, dawehner — I started reviewing it also, but my knowledge of the installer is limited to say the least, so I'm glad you caught some of the "you shouldn't do X" things.

The patch itself looks entirely sane and clean to me. RTBC +1

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

This patch is at least 5-6 fixes in one. :( I don't see how I can commit this. Can we not have a fix just for the issue at hand?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

It's simply not that simple to convert legacy code in the installer, because the installer is still some massive plate of spaghetti.

This patch performs the desired change at hand. The necessary changes for this issue will coincidentally also allow us to proceed with modernizing the installer.

Aside from that, I simply don't have the energy to split every single installer spaghetti clean-up into a separate issue. Especially because there's hardly anyone who's able to and who will review those issues (in 2017, mayhaps). ~5% of the current code will remain once we're done. If we want to move forward, let's move forward. If we don't want to move forward, then well, fine, then state that clearly.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Maybe catch or alexpott can take this one, then. I simply can't, I have no idea what's going on in this patch unless/until it's split up into digestible chunks.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Looking through the changes I can see this would be hard to split up - there's lots of places where $form['#title'] has to be used, but the form is being rendered to a string directly in the callback (which would ignore #title). Then once you do that you have to change some other things as well.

One question though:

+++ b/core/includes/install.core.inc
@@ -1314,13 +1339,10 @@ function install_select_profile(&$install_state) {
+        throw new NoProfilesException(\Drupal::service('string_translation'));

@@ -1871,24 +1893,6 @@ function _install_get_version_info($version) {
- * Indicates that Drupal has already been installed.

There's a lot of effort gone into making these exception messages translatable, in fact that's a decent chunk of the patch, but:

1. We usually don't bother translating exceptions - in these cases they're equivalent to PHP error messages, just nicer versions of them.

2. Does that even work in practice?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new install.title_.24.patch32.03 KB
new interdiff.txt1.79 KB
new install-no-profiles.png15.74 KB

Yes, this works, as proven by attached screenshot. Testing it was easy by temporarily hacking ExtensionDiscovery and choosing a different language than English in the installer:

  public function scan($type, $include_tests = NULL) {
    if ($type == 'profile') {
      return array();
    }

However, the validation/error handling logic was not correct. Fixing that feels a little out of scope, but since you asked for a proof/test for this code, I guess we can fix it as well here.

dawehner’s picture

dawehner
Primary language German
commented

1. We usually don't bother translating exceptions - in these cases they're equivalent to PHP error messages, just nicer versions of them.

2. Does that even work in practice?

There are a couple of instances which won't happen for normal users, one example is the following message:
We were unable to find any installation profiles. Installation profiles tell us what modules to enable and what schema to install in the database. A profile is necessary to continue with the installation process.
Other examples though can easily happen for normal people using drupal, for which we should not require to learn the english language.
One example is the following message:
li>To start over, you must empty your existing database, delete your active configuration, and copy <em>default.settings.php</em> over <em>settings.php</em>.</li><li>To install to a different database, edit the appropriate <em>settings.php</em> file in the <em>sites</em> folder

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new install.title_.24.patch32.03 KB

d.o issue comment conflict resolution--

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

OK cool that the translation works there. Still looks inconsistent with some exceptions getting passed the translation service and some a string run through t(), but that's even more out of scope for this issue. Moving back to RTBC.

catch’s picture

catch
he/him
Primary language English
commented

26: install.title_.24.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: install.title_.24.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new install.title_.30.patch32.03 KB

Merged 8.x (cleanly)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Hurray! :)

Just one more now: #2209595: Remove drupal_set_title(), drupal_get_title() and associated tests.

Status: Fixed » Closed (fixed)

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