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
#15 drupal-remove-drupal_set_title-in-views-2102489-15.patch1.09 KBwebwarrior
PASSED: [[SimpleTest]]: [MySQL] 59,858 pass(es).
[ View ]
#12 2102489-remove_drupal_set_title_in_views.patch661 byteswebwarrior
FAILED: [[SimpleTest]]: [MySQL] 59,824 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#1 drupal-remove_drupal_set_title_in_views_module-2102489-1.patch1.58 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 58,797 pass(es).
[ View ]

Comments

InternetDevels’s picture

Status:Active» Needs review
StatusFileSize
new1.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,797 pass(es).
[ View ]
dawehner’s picture

  1. +++ b/core/modules/views/includes/ajax.inc
    @@ -35,7 +35,6 @@ function views_ajax_form_wrapper($form_id, &$form_state) {
       if (empty($form_state['ajax']) && !empty($form_state['title'])) {
    -    drupal_set_title($form_state['title']);
         drupal_add_css(drupal_get_path('module', 'views_ui') . '/css/views_ui.admin.css');
       }

    @@ -64,11 +63,6 @@ function views_ajax_form_wrapper($form_id, &$form_state) {

    @@ -64,11 +63,6 @@ function views_ajax_form_wrapper($form_id, &$form_state) {
         return $response;
       }

    -  // These forms have the title built in, so set the title here:
    -  if (empty($form_state['ajax']) && !empty($form_state['title'])) {
    -    drupal_set_title($form_state['title']);
    -  }
    -

    Have you manually tested that this actually works? I ran into issues with titles on dialogs today.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/Page.php
    @@ -91,7 +91,7 @@ public function execute() {
    +    $render['#title'] = filter_xss_admin($this->view->getTitle());

    I would love to get rid of this hunk as this is already heavily rewritten on #2068471: Normalize Controller/View-listener behavior with a Page object

dawehner’s picture

Issue tags:+Needs manual testing

.

dawehner’s picture

Status:Needs review» Needs work

.

amitsedaiz’s picture

I applied the patch, created node, edited the node and viewed admin content page. The title seems to appear fine in all the pages.

Steps:
1. Did a git pull to get the latest code for 8.x
2. patch -p1 < drupal-remove_drupal_set_title_in_views_module-2102489-1.patch , at the Drupal root folder
3. Output:
patching file core/modules/views/includes/ajax.inc
Hunk #2 succeeded at 65 (offset 2 lines).
patching file core/modules/views/lib/Drupal/views/Plugin/views/display/Page.php

Note: I am a newbie. Please do let me know any additional testing needs to be done.

amitsedaiz’s picture

Status:Needs work» Needs review
dawehner’s picture

Note: I am a newbie. Please do let me know any additional testing needs to be done.

Try to have a look at the different dialogs of views. So go to admin/structure/views and edit one of the existing ones.
Click on some of the edit options on there and have a look whether the expected title appears. You could test for example test with and without the patch.

j2r’s picture

Reviewed and Tested

The title for content page is same before and after the patch applied. Checked all other views and title is coming same before and after applying the patch.

j2r’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Reviewed and tested.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Sorry, that's not quite the manual testing that dawehner was asking for. Rather than testing titles on a view itself, he was referring to testing titles on the dialogs within the Views admin UI.

However, I just did that and it seems to work well. The titles of the all the Views admin dialogs are coming through in my testing. Yay for fewer calls to drupal_set_title()! :)

I couldn't quite parse from "I would love to get rid of this hunk as this is already heavily rewritten on #2068471: Normalize Controller/View-listener behavior with a Page object" if it was meant that Daniel didn't want that part of the patch committed, or whether he wanted that to happen because it would make that issue easier. But in case it was the former, #2068471: Normalize Controller/View-listener behavior with a Page object is currently set to needs work anyway, so hopefully it doesn't make it too annoying if I commit this one in the meantime. If so, let me know, and I can temporarily revert it, too.

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

webwarrior’s picture

Status:Closed (fixed)» Needs review
StatusFileSize
new661 bytes
FAILED: [[SimpleTest]]: [MySQL] 59,824 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Removed a drupal_set_title from Views UI. The title seems to be handled on line 368.

Status:Needs review» Needs work

The last submitted patch, 12: 2102489-remove_drupal_set_title_in_views.patch, failed testing.

dawehner’s picture

Ha, we have test coverage!

webwarrior’s picture

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,858 pass(es).
[ View ]

Could this possibly be a solution?

Status:Needs review» Needs work

The last submitted patch, 15: drupal-remove-drupal_set_title-in-views-2102489-15.patch, failed testing.

webwarrior’s picture

Status:Needs work» Needs review
Crell’s picture

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Seems OK, bot likes, and this is our last drupal_set_title(). Yay!

webchick’s picture

Title:Remove drupal_set_title in views module controllers» Remove drupal_set_title in views module controllers (last instance of drupal_set_title())
Assigned:Unassigned» dawehner
Priority:Normal» Major

Awesome!

Since Daniel caught a snafu with this last time, assigning to him for final review.

Also, since this is the last thing blocking #1830588: [META] remove drupal_set_title() and drupal_get_title() (which is critical + a beta blocker), also escalating this to major.

dawehner’s picture

Clicked a bit through the UI and all titles still appears as expected.

Sadly this will conflict with #2146473: Move views/includes/ajax.inc to a class in views_ui module which is RTBC since quite some time.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Cool, thanks for checking. And indeed, sorry about that. I've added that to my list of things to get in right after alpha8.

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title:Remove drupal_set_title in views module controllers (last instance of drupal_set_title())» Remove drupal_set_title in views module controllers (last instance of drupal_set_title() in modules)

Oh, boo. :( This was not the last instance of drupal_set_title() at all. :( There are tons of them in includes and whatnot. Left a comment at #1830588: [META] remove drupal_set_title() and drupal_get_title().

Status:Fixed» Closed (fixed)

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