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
StatusFileSize
new6.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
StatusFileSize
new6.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
StatusFileSize
new6.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
StatusFileSize
new3.65 KB
new6.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

StatusFileSize
new676 bytes
new6.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

StatusFileSize
new1.07 KB
new6.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
StatusFileSize
new5.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

StatusFileSize
new5.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
StatusFileSize
new685 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.