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()

Files: 
CommentFileSizeAuthor
#25 2102465-page-title-node-25.patch685 byteswebwarrior
PASSED: [[SimpleTest]]: [MySQL] 59,798 pass(es). View
#18 2102465-page-title-node-18.patch5.98 KBACF
PASSED: [[SimpleTest]]: [MySQL] 60,141 pass(es). View
#16 2102465-page-title-node-16.patch5.98 KBACF
PASSED: [[SimpleTest]]: [MySQL] 59,192 pass(es). View
#14 2102465-page-title-node-14.patch6.66 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,693 pass(es), 887 fail(s), and 45 exception(s). View
#14 2102465-diff-13-14.txt1.07 KBvijaycs85
#13 2102465-page-title-node-12.patch6.39 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,421 pass(es). View
#13 2102465-diff-10-12.txt676 bytesvijaycs85
#10 2102465-page-title-node-10.patch6.42 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,400 pass(es). View
#10 2102465-diff-8-10.txt3.65 KBvijaycs85
#8 2102465-page-title-node-8.patch6.81 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 59,275 pass(es), 3 fail(s), and 0 exception(s). View
#3 drupal-node_remove_drupal_set_title_2102465-3.patch6.77 KBsidharthap
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-node_remove_drupal_set_title_2102465-3.patch. Unable to apply patch. See the log in the details link for more information. View
#1 drupal-node_remove_drupal_set_title_2102465.patch6.78 KBInternetDevels
FAILED: [[SimpleTest]]: [MySQL] 58,746 pass(es), 3 fail(s), and 0 exception(s). View

Comments

InternetDevels’s picture

Status: Active » Needs review
FileSize
6.78 KB
FAILED: [[SimpleTest]]: [MySQL] 58,746 pass(es), 3 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

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

sidharthap’s picture

Status: Needs work » Needs review
FileSize
6.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-node_remove_drupal_set_title_2102465-3.patch. Unable to apply patch. See the log in the details link for more information. View

re-roll patch

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

looks good and should still apply

cosmicdreams’s picture

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

spoke too soon needs a reroll

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.81 KB
FAILED: [[SimpleTest]]: [MySQL] 59,275 pass(es), 3 fail(s), and 0 exception(s). View

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

Status: Needs work » Needs review
FileSize
3.65 KB
6.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,400 pass(es). View

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: Replace content revision table with a view

tim.plunkett’s picture

  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

  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

FileSize
676 bytes
6.39 KB
PASSED: [[SimpleTest]]: [MySQL] 59,421 pass(es). View

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

FileSize
1.07 KB
6.66 KB
FAILED: [[SimpleTest]]: [MySQL] 55,693 pass(es), 887 fail(s), and 45 exception(s). View

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

Status: Needs work » Needs review
FileSize
5.98 KB
PASSED: [[SimpleTest]]: [MySQL] 59,192 pass(es). View

Reroll, tested locally and passes tests.

dawehner’s picture

+++ 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

FileSize
5.98 KB
PASSED: [[SimpleTest]]: [MySQL] 60,141 pass(es). View

Oops, hopefully fixed now.

vijaycs85’s picture

Issue tags: +London_2013_October

Updating tag...

dawehner’s picture

Issue tags: -London_2013_October

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

ACF’s picture

Apologies for the lack of interdiff.

catch’s picture

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

Issue summary: View changes
Status: Closed (fixed) » Needs work
FileSize
685 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,798 pass(es). View

Shouldn't the other drupal_set_title also be removed ?

vijaycs85’s picture

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

Status: Needs review » Reviewed & tested by the community

+1

webchick’s picture

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.