As part of #1830588: [META] remove drupal_set_title() and drupal_get_title(), all calls to drupal_set_title() in core/authorize.php and core/includes/authorize.inc should be converted to use #title in a render array.

Suggested test process

There is a potential conflict with #2169765: Redesign update.php to be more consistent with the installation process, but I don't think this will affect the changes in #7.

CommentFileSizeAuthor
#7 drupal-2192653-set_title-authorize-7.patch4.33 KBianthomas_uk
#2 2192653-1.patch2.34 KBdivesh.kumar
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

divesh.kumar’s picture

divesh.kumar CreditAttribution: divesh.kumar commented
Assigned: Unassigned » divesh.kumar

I'll take on Authorization files.

divesh.kumar’s picture

divesh.kumar CreditAttribution: divesh.kumar commented
Status: Active » Needs review
FileSize
2192653-1.patch2.34 KB

Made required changes.

Status: Needs review » Needs work

The last submitted patch, 2: 2192653-1.patch, failed testing.

sidharthap’s picture

sidharthap
Primary language English
CreditAttribution: sidharthap commented

Here it fails simple test.$title = $_SESSION['authorize_operation']['page_title']; Title is being render before this $title rendered to page.

divesh.kumar’s picture

divesh.kumar CreditAttribution: divesh.kumar commented

Hi sidharthap,

All of the instance of drupal_set_title are getting change without failing any test. However when I remove this one it fails the test.

Any help or pointers on this?

ianthomas_uk’s picture

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

#2060401: Remove useless "early render" in authorize.php may be relevant

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Assigned: divesh.kumar » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
drupal-2192653-set_title-authorize-7.patch4.33 KB

I started the attached patch from scratch, but it ended up broadly similar to #2.

#2 didn't replace the title it removed from authorize_access_denied_page(). That was presumably because it was inside a function, so you couldn't just use a local variable like some of the others. That function didn't seem to add much value, so I've just copied its contents inline (wrapping the 'Access Denied' title inside t() in the process).

There was a bit of a weird dance going on with $_SESSION['authorize_operation']['page_title'], because all of $_SESSION['authorize_operation'] gets unset, which is why drupal_set_title needed to be called from authorize_run_operation() and also why the title has to be restored in update_authorize_update_batch_finished. By changing the title to $_SESSION['authorize_page_title'] it won't get unset, so none of that is needed (the title will just be picked up from $_SESSION in authorize.php).

awochna’s picture

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

Tested patch (drupal-2192653-set_title-authorize-7.patch) and everything works as expected.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

I don't have a local FTP server going, so I can't use the update manager completely. However, I do get to see the previous "Update manager" title, so AFAICT all is well.

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

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Status: Fixed » Reviewed & tested by the community

This doesn't seem to have been pushed.

webchick’s picture

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

Sorry, Drupal.org hates me. Pushed now.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented 
diff --git a/core/modules/update/update.authorize.inc b/core/modules/update/update.authorize.inc
index c4f3ad8..3bfdbcf 100644
--- a/core/modules/update/update.authorize.inc
+++ b/core/modules/update/update.authorize.inc
@@ -237,7 +237,6 @@ function update_authorize_update_batch_finished($success, $results) {
   $_SESSION['authorize_results']['page_message'] = $page_message;
   $_SESSION['authorize_results']['messages'] = $results['log'];
   $_SESSION['authorize_results']['tasks'] = $results['tasks'];
-  $_SESSION['authorize_operation']['page_title'] = t('Update manager');
 }
 
 /**
@@ -296,7 +295,6 @@ function update_authorize_install_batch_finished($success, $results) {
   $_SESSION['authorize_results']['page_message'] = $page_message;
   $_SESSION['authorize_results']['messages'] = $results['log'];
   $_SESSION['authorize_results']['tasks'] = $results['tasks'];
-  $_SESSION['authorize_operation']['page_title'] = t('Update manager');
 }

Removing those broke the page title in some cases - adding it back in #2547667: Fix broken page title Update Manager (authorize.php). It is admittedly confusing why it needs to be there since it's also set elsewhere, but I think the other places only set it under a limited set of conditions or something.