Subtask of #1830588: [META] remove drupal_set_title() and drupal_get_title()

Problem/Motivation

Using procedural drupal_set_title() inside controller class is not encouraged.

Proposed resolution

Replace drupal_set_title() with #title in page return array.

Remaining tasks

Issue patch

User interface changes

Refer parent issue at #1830588: [META] remove drupal_set_title() and drupal_get_title()

API changes

Refer parent issue at #1830588: [META] remove drupal_set_title() and drupal_get_title()

CommentFileSizeAuthor
#25 2102465-page-title-node-25.patch685 byteswebwarrior
#18 2102465-page-title-node-18.patch5.98 KBACF
#16 2102465-page-title-node-16.patch5.98 KBACF
#14 2102465-page-title-node-14.patch6.66 KBvijaycs85
#14 2102465-diff-13-14.txt1.07 KBvijaycs85
#13 2102465-page-title-node-12.patch6.39 KBvijaycs85
#13 2102465-diff-10-12.txt676 bytesvijaycs85
#10 2102465-page-title-node-10.patch6.42 KBvijaycs85
#10 2102465-diff-8-10.txt3.65 KBvijaycs85
#8 2102465-page-title-node-8.patch6.81 KBvijaycs85
#3 drupal-node_remove_drupal_set_title_2102465-3.patch6.77 KBsidharthap
#1 drupal-node_remove_drupal_set_title_2102465.patch6.78 KBInternetDevels
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Status: Active » Needs review
FileSize
drupal-node_remove_drupal_set_title_2102465.patch6.78 KB

Status: Needs review » Needs work

The last submitted patch, drupal-node_remove_drupal_set_title_2102465.patch, failed testing.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented
Status: Needs work » Needs review
FileSize
drupal-node_remove_drupal_set_title_2102465-3.patch6.77 KB

re-roll patch

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
Status: Needs review » Reviewed & tested by the community

looks good and should still apply

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
Issue tags: -WSCCI Conversions

#3: drupal-node_remove_drupal_set_title_2102465-3.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +WSCCI Conversions

The last submitted patch, drupal-node_remove_drupal_set_title_2102465-3.patch, failed testing.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented

spoke too soon needs a reroll

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2102465-page-title-node-8.patch6.81 KB

Another re-roll from #1

Status: Needs review » Needs work

The last submitted patch, 2102465-page-title-node-8.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review
FileSize
2102465-diff-8-10.txt3.65 KB
2102465-page-title-node-10.patch6.42 KB

Seems valid fails

D7:
drupal_set_title('My title');

D8
$page['#title'] = String::checkPlain('My title');

Whereas

D7
drupal_set_title('My title', PASS_THROUGH);

D8:
$page['#title'] = 'My title';

Updating patch to do this (seems it was doing the opposite).

Also reverting changes in node_revision_overview() as the whole function will be removed as part of #1863906: [PP-1] Replace content revision table with a view

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
  1. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -142,4 +142,18 @@ protected function buildPage(NodeInterface $node) {
+    $name = String::checkPlain($node_type->type);
+    return $this->t('Create @name', array('@name' => $name));

    This is going to double escape, @ runs checkplain 

  2. +++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.php
@@ -23,10 +24,10 @@ public function form(array $form, array &$form_state) {
+      $form['#title'] = String::checkPlain($this->t('Add content type'));

    This is not needed, since we KNOW that "Add content type" is clean

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -142,4 +142,18 @@ protected function buildPage(NodeInterface $node) {
+   * @param EntityInterface $node_type

    This should be the full namespace. 

  2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -142,4 +142,18 @@ protected function buildPage(NodeInterface $node) {
+  public function addPageTitle(EntityInterface $node_type) {

    We do have a NodeTypeInterface 

  3. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -142,4 +142,18 @@ protected function buildPage(NodeInterface $node) {
+    $name = String::checkPlain($node_type->type);
+    return $this->t('Create @name', array('@name' => $name));

    This basically check plains twice. 

  4. +++ b/core/modules/node/lib/Drupal/node/Form/NodeTypeDeleteConfirm.php
@@ -71,8 +72,8 @@ public function getConfirmText() {
-      drupal_set_title($this->getQuestion(), PASS_THROUGH);
...
+      $form['#title'] = $this->getQuestion();

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -66,7 +67,7 @@ public function form(array $form, array &$form_state) {
-      drupal_set_title(t('<em>Edit @type</em> @title', array('@type' => node_get_type_label($node), '@title' => $node->label())), PASS_THROUGH);
+      $form['#title'] = $this->t('<em>Edit @type</em> @title', array('@type' => node_get_type_label($node), '@title' => $node->label()));

@@ -74,6 +75,7 @@ public function form(array $form, array &$form_state) {
+      $form['#title'] = $this->t('Preview');

@@ -400,7 +402,6 @@ public function preview(array $form, array &$form_state) {
-    drupal_set_title(t('Preview'), PASS_THROUGH);

+++ b/core/modules/node/lib/Drupal/node/NodeTypeFormController.php
@@ -23,10 +24,10 @@ public function form(array $form, array &$form_state) {
-      drupal_set_title(t('Add content type'));
+      $form['#title'] = String::checkPlain($this->t('Add content type'));
...
-      drupal_set_title(t('Edit %label content type', array('%label' => $type->label())), PASS_THROUGH);
+      $form['#title'] = $this->t('Edit %label content type', array('%label' => $type->label()));

    Looks perfect

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
2102465-diff-10-12.txt676 bytes
2102465-page-title-node-12.patch6.39 KB

Thanks for the quick review. here is update.

#11.1 - fixed
#11.2 a) it is conversion of what we have in D7 b) t() can bring anything in.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
FileSize
2102465-diff-13-14.txt1.07 KB
2102465-page-title-node-14.patch6.66 KB

Thanks for the review @dawehner. Here is the updates:
#12.1 fixed
#12.2 fixed
#12.3 fixed - duplicate of #11.1
#12.4 thanks :)

Status: Needs review » Needs work

The last submitted patch, 2102465-page-title-node-14.patch, failed testing.

ACF’s picture

ACF CreditAttribution: ACF commented
Status: Needs work » Needs review
FileSize
2102465-page-title-node-16.patch5.98 KB

Reroll, tested locally and passes tests.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -142,4 +143,17 @@ protected function buildPage(NodeInterface $node) {
+      return $this->t('Create @name', array('@name' => $node_type->type));

The amount of spaces is just too damn high.

ACF’s picture

ACF CreditAttribution: ACF commented
FileSize
2102465-page-title-node-18.patch5.98 KB

Oops, hopefully fixed now.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue tags: +London_2013_October

Updating tag...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: -London_2013_October

Please try to post interdiff before: https://drupal.org/documentation/git/interdiff

dawehner’s picture

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

... But actually my goal was to set this to RTBC.

ACF’s picture

ACF CreditAttribution: ACF commented

Apologies for the lack of interdiff.

catch’s picture

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

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

webwarrior’s picture

webwarrior CreditAttribution: webwarrior commented
Issue summary: View changes
Status: Closed (fixed) » Needs work
FileSize
2102465-page-title-node-25.patch685 bytes

Shouldn't the other drupal_set_title also be removed ?

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs work » Needs review

Good catch @webwarrior. Manually tested this patch and it works. Inside form() method the title for preview page is handled. So it is safe to remove.

dawehner’s picture

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

+1

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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