theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig

More general title. Updated summary.

I've got a couple questions about your patch. Sorry these questions are so newb-level, just trying to help.

• Which branch are we using? I tried twig_engine but the patch wouldn't apply.
• Your patch removes the function theme_task_list from theme.maintenance.inc and replaces it with template_preprocess_task_list, but it looks like template_preprocess_task_list already exists (it's the next function in the file). This might be part of why the function is failing.
• Also, your patch does a loop with "$items as $k => $item", but the already existing template_preprocess_task_list uses "$items as $key => $item". That's a minor difference but would help with code consistency.

I've created a diagram that describes the function changes in this patch: https://docs.google.com/drawings/d/1YSwbiZr_zNN8NzU8xrci4UfMXRZve_bpM_sZ...

@jenlampton, here is your problem I think, well at least part of it :).

+++ b/core/includes/theme.maintenance.inc
@@ -173,11 +172,15 @@ function theme_update_page($variables) {
   if (!empty($messages)) {
-    $output .= '<div id="authorize-results">';
+    $item_lists = array();

$item_lists should be $item_list.

Plus, have you declared $lists as an empty array before populating it, I do not see it.

@@ -186,31 +189,8 @@ function theme_authorize_report($variables) {
         }
         $items[] = theme('authorize_message', array('message' => $log_message['message'], 'success' => $log_message['success']));
       }
-      $output .= theme('item_list',  array('items' => $items, 'title' => $heading));
+      $lists[] = theme('item_list',  array('items' => $items, 'title' => $heading));
     }
-    $output .= '</div>';
-  }
-  return $output;

Made some progress, but in my working copy template_preprocess_update_page() and template_preprocess_install_page() are not being called. Tomorrow I'll post a patch that should at least install.

I used base_hook to make update.php and install.php work on HEAD, but as mentioned in #9, template_preprocess_update_page() and template_preprocess_install_page() are not being called. I couldn't figure out a way to get the preprocess to fire. I tried adding them to 'preprocess functions' in drupal_common_theme() (along with the base_hook), but that seems unnecessary and didn't change anything anyway.

See also:
#1885714: Remove theme_install_page()
#1885850: Remove theme_update_page()

The 'theme_hook_original' approach taken in the sandbox (#1821006-3: Create preprocess function for theme_install_page) doesn't seem to work on HEAD (the patch in #3 here results in a blank screen for both install.php and update.php), and I'm not even sure how it works on the sandbox, grepping through core I can only see theme_hook_original being stored, not used in any way.

[ 8.x ] $ grep -nr -C1 "theme_hook_original" core
core/includes/theme.inc-1041-  $variables += array(
core/includes/theme.inc:1042:    'theme_hook_original' => $original_hook,
core/includes/theme.inc-1043-  );
--
core/includes/theme.inc-2318-          // Lastly, inherit the original theme variables of the current list.
core/includes/theme.inc:2319:          $child['#theme'] = $variables['theme_hook_original'];
core/includes/theme.inc-2320-          $child['#type'] = $variables['type'];

Also, 'show_messages' => NULL was added to install_page in drupal_common_theme() to prevent these errors after installation:

Notice: Undefined index: show_messages in template_preprocess_maintenance_page() (line 3048 of core/includes/theme.inc).

From #10:

I can only see theme_hook_original being stored, not used in any way.

Not strictly true, but the only place I saw it being used was in template_preprocess_item_list(), which I don't think applies here.

Turns out installation was only working locally because I already had a settings.php file.

After talking to @Fabianx, it sounds like theme.maintenance.inc will actually need to be converted in stages.

1. Add preprocess functions to theme.maintenance.inc, add Twig templates.
2. Convert Seven (and Bartik) to Twig, engine = twig in .info file or make Twig the default theme engine.
3. Remove theme_ functions from theme.maintenance.inc and add 'template' declarations to drupal_common_theme() in theme.inc.

So here's a more modest patch that just does #1 in preparation for the next steps.

#2 and #3 from my comment in #14 would probably need to be in the same patch.

Based on #1905584: Move base theme system templates into /core/templates it seems this issue would be accommodated by #1898454: system.module - Convert PHPTemplate templates to Twig, though that may result in a large patch.

Ignore what I said about system.module in #16, we'll proceed to convert this separately of system.module and put in /core/templates as described in #1905584: Move base theme system templates into /core/templates

Re-posting patch from #10 (freshly rerolled to remove the offset) now that #1942490: Make Twig engine available during install is in!

Docs will need some love (#1913208: [policy] Standardize template preprocess function documentation for instance), will take a look over the next few days.

After looking at this again, I think #1885850: Remove theme_update_page() and #1885714: Remove theme_install_page() should just be incorporated into this issue, unless someone can figure out how to get those preprocess functions (template_preprocess_install_page, template_preprocess_update_page) to fire. I know we're trying to just convert and not consolidate yet, but I think this might be a good case for an exception to be made.

Edited for clarification.

Doing #20 would introduce its own problems, see #1885714-2: Remove theme_install_page(). So I'm instead proposing that for this initial conversion issue, we actually don't touch theme_update_page() or theme_install_page().

Attached patch does this, cleans up docs, and also moves the parens around "active" and "done" from template_preprocess_task_list() to task-list.html.twig.

Nope, that's not the right patch. Try this one, interdiff is from #18.

Sorry for the patch onslaught. Revised the patch with more docs touchups and converted the remaining theme() calls to render arrays. Interdiff is from #22.

Improve summary

Looks like maintenance-page.tpl.php conversion should be part of this patch.

Note status of theme_install_page() and theme_update_page().

Add conversion summary table

Added maintenance-page.html.twig from the sandbox and made a few minor tweaks. See #1189822: Convert maintenance-page.html.twig to HTML5 to convert maintenance-page to HTML5, this is just a straight conversion so yes it's XHTML and that's just fine for now :) Compared markup with Stark as the default theme and everything seems to match up.

This also fixes task-list. I'm not sure why, but the preprocess function was not firing without adding 'preprocess functions' => array('template_preprocess_task_list'), - copped from the sandbox.

Add maintenance-page.tpl.php HTML5 conversion issue to related

Tagging. If anyone knows how we can manually test authorize.php please add testing steps in the summary.

Update conversion table, add testing steps

Unassigning for now, if I can figure out how to test the authorize.php report I'll post my findings and update the issue summary. Otherwise, this is ready for review.

remove sandbox

Update conversion table

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1987426: Convert maintenance-page.tpl.php to Twig for template conversion.

Another tweak

Updated issue summary.

#29: twig-maintainence-page-theme-only-1885564-29.patch queued for re-testing.

#29: twig-maintainence-page-theme-only-1885564-29.patch queued for re-testing.

#29: twig-maintainence-page-theme-only-1885564-29.patch queued for re-testing.

Reroll!

#36: twig-maintainence-page-theme-only-1885564-36.patch queued for re-testing.

Manually testing this now...

+++ b/core/includes/theme.incundefined
@@ -3311,12 +3311,16 @@ function drupal_common_theme() {
+      'preprocess functions' => array('template_preprocess_task_list'),
+      'template' => 'task-list',
     ),
     'authorize_message' => array(
       'variables' => array('message' => NULL, 'success' => TRUE),
+      'template' => 'authorize-message',
     ),
     'authorize_report' => array(
       'variables' => array('messages' => array()),
+      'template' => 'authorize-report',

Maybe newbies question but why do you need to set the preprocess function for task_list but not for authorize_report?

I'm not 100% sure I found all use-cases that get routed through authorize.php, but for everything I tested, the markup using Twig and the patch in #36 was identical to the markup from PHPTemplate.

Re #40, I don't see why we would need to specify the preprocess functions, so I think that can be removed.

I removed the preprocess function line.

Getting "Fatal error: Class 'Attribute' not found in /Applications/MAMP/htdocs/drupal8/core/includes/theme.maintenance.inc on line 131" from template_preprocess_task_list(). I need to figure out how to load the Attribute class.

Ok I found the issue, I added "use Drupal\Core\Template\Attribute;" to theme.maintenance.inc and that fixed the /install.php problem

#47: twig-maintainence-page-theme-only-1885564-47.patch queued for re-testing.

Thanks everyone for working on this one! Tagging for profiling.

manual testing is a novice task.

Needs a reroll before it can be tested or profiled.

Cleaned up a bit and re-rolled.

#54: 1885564-54-twig-theme-maintenance.patch queued for re-testing.

#54: 1885564-54-twig-theme-maintenance.patch queued for re-testing.

Seems there was a bunch of things not so right so here is an attempt to correct them.

Needs a rebase just based on the patch file size. Thanks for working on this :)

Whoops

Unassigning for now, but will try to take another look when possible. Couldn't figure out how to manually test authorize.php (same as #27). Any ideas? Please post thoughts...

After installing this patch, I reviewed the update routine where these messages would appear (task list) and found everything in order. I also tested the same code on a new install (fresh DB) and reviewed task list on install. Worked. I 'm still not entirely sure how to conclusively test the authorize message template, but reviewed the code. I don't think there's any issue here -- looks clean. so... marking as reviewed.

This really needs testing of authorize.php as well before it can be committed.

In addition it'd be good to test what happens when the database is down etc. since presumably these will be called then too.

Yes, this needs more testing as well as profiling. To start we could manually invoke authorize_report with some data and check before/after output (and perhaps profile that way as well), until we can figure out how to test authorize.php properly.

I'm not worried about profiling - performance of the maintenance page isn't a big deal, but definitely additional testing.

Okay, great. Thanks @catch!

Any hints as to how we can test authorize.php in a realistic scenario? So far we've had a few people try (myself included) and come up short, and I asked in #drupal-contribute a few times but to no avail.

Wow, a question from August, and I think I know the answer.

From my experience with manually testing Drupal 7, I remember I came across errors with the authorize.php (remember when this always blew up early in D7?) whenever you used the system's "install a new module" feature from the modules (now, extend) page.

I am having trouble testing that 'Install a new module' functionality, possibly related to PHP 5.4 and this issue:

#842620: Update manager can't install modules using FTP due broken FileTransferAuthorizeForm

Updated issue summary.

62: 1885564-62-twig-theme-maintenance.patch queued for re-testing.

Two things are bothering me a bit with authorize-message.html.twig:

• theme_authorize_message returned an array and not a string... this is weird.
• Why is a fail bold but a success message not? Can't that just be dealt with in CSS if it needs to be like that?

If anybody knows how to test those pages. please let us know.

Killed the theme_authorize_message because it wasn't a theme function, it was just a helper to build array items for an item_list.
Tested the output of the 'authorize_report' manually with a direct call to the theme function because I couldn't find a way to get at it otherwise.

Tagging for reroll.

Rerolled.

    $variables['attributes']['id'] = 'authorize-results';

This ID doesn't seem to be referenced anywhere else.

          $message = '<strong>' . $log_message['message'] . '</strong>';
          $classes = array('failure');

I agree that this should probably be done with CSS instead.

+++ b/core/includes/theme.maintenance.inc
@@ -119,86 +119,66 @@ function theme_task_list($variables) {
+    $variables['attributes']['id'] = 'authorize-results';

+++ b/core/modules/system/templates/authorize-report.html.twig
@@ -0,0 +1,23 @@
+  <div{{ attributes }}>

This is where authorize-results should show up during markup review.

We may have to do the .failure CSS change in a follow-up issue but maybe Cottser can weigh in on that?

If it's a markup change I'd lean towards follow-up, probably better if these conversions don't touch CSS.

Here's a quick dreammarkup followup for that:
#2215543: Replace strong tag with CSS in template_preprocess_authorize_report and remove id..

This is where authorize-results should show up during markup review.

I meant that #authorize-results doesn't seem to be referenced from CSS or JavaScript, so if it's unused maybe we should consider dropping it from the output markup entirely (and if that's the case we can perhaps remove the wrapper <div> as well?)

Oh ok now I understand, feel free to add that to the dreammarkup issue in #82 as well.

The patch in #79 did not apply.
Here is a re-roll of it.

85: 1885564-twig-theme_maintenance-85.patch queued for re-testing.

Tagging for reroll.

Assigning to myself. Assigned during my first Core Mentoring session.

Please be kind!

Changed use Drupal\Component\Utility\Settings; to use Drupal\Core\Site\Settings;.

Settings moved in this issue: #2208475: Move Settings into Drupal\Core\Site\Settings

90: 1885564-twig-theme_maintenance-90.patch queued for re-testing.

Guys, we can't proceed work on https://drupal.org/node/2215543 because of this issue. Thanks!

@vollepeer that issue just makes a small CSS issue that's why it was broken out. This issue can proceed it needs manual review to make sure the markup is the same before and after.

The only thing that may be holding this back is we aren't sure how to view those autorized pages, and if you know that or could find that out we can get some people to test the markup hasn't changed.

+++ b/core/modules/system/templates/task-list.html.twig
@@ -0,0 +1,25 @@
+<h2 class="element-invisible">Installation tasks</h2>

Wrong class change here from "visually-hidden" to old "element-invisible"

+++ b/core/modules/system/templates/task-list.html.twig
@@ -0,0 +1,25 @@
+    {% if task.status %} <span class="element-invisible">({{ task.status }})</span>{% endif %}

Same thing here

Note that authorized_result was unusable before this due to the use of theme_authorize_message(). The markup is OK now that it is removed

Patch attached

Just resetting the status to get the patch tested

.twig files were missing

Are the needs tags here still relevant?

Yes they are we, still need a markup comparison (needs manual testing tag) of the authorize report and we aren't sure how to get that template for the "steps to test".

97: 1885564-twig-theme_maintenance-98.patch queued for re-testing.

I could take this for a spin but I'm missing the steps of how and where to test.

Yes, we apparently have authorize.php in core but nobody knows how to manually test it or they're not telling :( See around #66 #69 comments.

I wonder if this helps with manual testing, the following is from settings.php

* Access control for update.php script.
*
* If you are updating your Drupal installation using the update.php script but
* are not logged in using either an account with the "Administer software
* updates" permission or the site maintenance account (the account that was
* created during installation), you will need to modify the access check
* statement below. Change the FALSE to a TRUE to disable the access check.
* After finishing the upgrade, be sure to open this file again and change the
* TRUE back to a FALSE!

So to test, login to an account that is not the maintenance account, but has permission "Administer software
* updates" then access update page that should give positive result, and without permission should give negative.

Then try with site maintenance account, should give positive result.

Then try login with account with no "Administer software
* updates" permission, should give negative result, and finally edit settings.php
$settings['update_free_access'] = FALSE;

Change to "TRUE" and try again, should give positive result.

Hope that makes some kind of sense.

PS the page to try is "yoursite/update.php" plus, also try last step as anonymous user, should give same results with settings.php change from FALSE to TRUE

@mbrett5062 sure but that's update.php, we need to test authorize.php.

But I thought that is all authorize.php does, is authorize the use of update.php for various scenarios. So if the tests I outlined pass or fail that is an indication the authorize.php is working correctly. Or do you need more direct testing. Maybe I misunderstand the use of authorize.php, if so please forgive my noise.

@mbrett5062 - I honestly don't know because I've never used authorize.php and from the sounds of it a lot of folks haven't. Thanks for your efforts :)

The way I understand it, and have only once had to make use of authorize.php is in the following scenario.

A module is updated, but contains broken code, that kills your site, not allowing you to reach even the login page or the front end.
A new release of the module is created that will fix the bug and revert any bad database changes, but you can not access the site as admin to run update.php.
So you edit settings.php and then are able to run update.php as anonymous user. Fixing the bug and database so resurrecting your site.

Tested manually on new download of D8-dev. With and Without patch works as expected, output looks exactly the same. Attached file is a diff of the full page HTML output.
(Not that it shows much as there is no difference)

Oppss did not realize .diff would be sent for test, sorry.

@mbrett5062 No worries about the .diff patch fail. *.txt works well or something-do-not-test.patch I think also works.

A diff for manual testing would be if you could turn on twig_debug config setting in your settings.php and the before and after the patch you should see a reference to authorize-report.html.twig in the twig debug message.

twig_debug is in the settings.local.php so you can just copy and uncomment that into your settings.php. After a cache rebuild (drush cr) you will see all the twig templates rendered on the page in html comments.

Re: #108:

So you edit settings.php and then are able to run update.php as anonymous user. Fixing the bug and database so resurrecting your site.

It's my understanding that this is not the same thing as authorize.php. The docblock in authorize.php starts with "Administrative script for running authorized file operations."

authorize.php is called when manipulating files from the interface.

Right now there is a bug preventing that #2042447: Install a module user interface does not install modules (or themes)

OK so after solving #2042447: Install a module user interface does not install modules (or themes) I've encountered the fact that preprocess functions (i.e. template_preprocess_*) were not automatically discovered.

So I attached a patch and the whole thing is testable with simplytest.me

To test:

• Apply the 2 patches or go to simplytest.me
• Install with minimal (or whatever)
• Enable the HTTP basic auth module
• Click "Install a module"
• Install devel (http://ftp.drupal.org/files/projects/devel-8.x-1.x-dev.tar.gz)
• You should see an error or a success (but themed properly...)

Ok so I investigated why the preprocess functions would not be discovered from time to time. It's because they reside in theme.maintenance.inc and this file is included on demand by drupal_maintenance_theme()(whereas pager.inc is always included for example). So it depends when the Theme Registry is built.
As a solution: either we state the preprocess functions, either we add the path (relative to the system module) in drupal_common_theme().

This patch is testable with simplytest.me too with the same process as before

D8 maintainer will have to chose either #114 or this one

Thanks @SebCorbin! Overall I like the 'file' a lot better (#115) but not the ../../../ part. Can we use DRUPAL_ROOT or a PHP built-in to handle that?

Well the path is relative to the system module, so we can do

  'file' => 'includes/theme.maintenance.inc',
  'path' => DRUPAL_ROOT . 'core',
  'template' => 'modules/system/templates/task-list'

Re-rolled.

__DIR__ could work too, it's built into pHP and you are already in the includes folder. So the file could literally be the file.

'path' => __DIR__,
'file' => 'theme.maintenance.inc',
'template' => 'modules/system/templates/task-list'

Though a bit confused why this is needed... shouldn't it just pickup in the current directory by default?

And, I'm considering moving the authorize theme functions into their own issue so we can get the task list one in. And deal with those two on their own with a fresh start(of not knowing how to test). :P

Split out task_list because that is ready to go in and we need to get the steps to reproduce for the authorize templates.
#2329505: Convert theme_task_list to Twig template

Tried the thing above with __DIR__ but not sure if the 'path' relates to the 'file' or the 'template' or both...

That __DIR__ doesn't work after I looked at how it works... and 'path' get's applied to the template and the file...

includes works best I think, I tested it out on our testable version for the task-list.html.twig and it worked for the preprocess firing and the template loading.

@SebCorbin you rock!!! That patch of yours in #2042447: Install a module user interface does not install modules (or themes) totally makes this testable.

I totally missed the testing steps somehow, thanks @SebCorbin I've added those steps to the issue summary! You do indeed rock.

Thanks again @SebCorbin, you have no idea how good it feels to finally see authorize.php after waiting for well over a year :)

I tested the latest patch here along with #2042447-27: Install a module user interface does not install modules (or themes) and I tested installing http://ftp.drupal.org/files/projects/page_manager-8.x-1.x-dev.tar.gz. This patch isn't 100% but it's a heck of a lot better than what's happening on HEAD.

Also not sure about the page_manager section in the before (if it's even supposed to be there…), we are losing that with this patch.

Before patching

After patching

Oops, I forgot to change the issue # for the suggested commit message on #2329505: Convert theme_task_list to Twig template. Don't worry, nothing from here got committed :)

Did small reroll for this

Here's few screenshots from the page

Wrong issue, laurii?

Right issue, see the last handful of comments.

Did a reroll, but can't test this; attempting to install new modules (even with the latest from #2042447: Install a module user interface does not install modules (or themes)) doesn't work.

Thank you for the re-roll @akalata

I was reviewing this and noticed this:

I used the latest patch from #2042447: Install a module user interface does not install modules (or themes) to review.

I'm a bit baffled why that would escape, they are renderable arrays with #markup... I may need to take another stab at this to move more markup to the template.

Reroll of #133.

Re-rolling for the markup/class changes.

+++ b/core/includes/theme.maintenance.inc
@@ -105,64 +105,37 @@ function _drupal_maintenance_theme() {
   $messages = $variables['messages'];
...
+          '#markup' => $message,

I think this is an array sometimes and possibly just need to cast it as always being that and just add '#wrapper_attributes' to it, but not sure yet... need to debug and step through this.

The escaping is because after the batch process all the SafeMarkup set's are gone. Need to somehow peel them out of batch, or set them as safe after. The later being easier.

Solved 2 issues:

• preprocess was not picked up (again) because includes are not loaded on registry rebuild -> now they are
• Unsafe links markup is now converted to render arrays with #markup, but the link were pointing to core/admin/modules, used $GLOBALS['base_url'] to solve that

Patch from https://www.drupal.org/node/2042447#comment-9623965 still required

Here's the current result and what we aim to obtain

@SebCorbin thanks, could you post an interdiff so we can see exactly your changes please? https://drupal.org/documentation/git/interdiff

#markup is a bit cheating but I'm not sure how else we can deal with it through the batch process it seems to lose being safe markup. A bit worried about the need for $GLOBALS['base_url'] now, there should be a way to avoid the need for that, no?

Sorry, here's the interdiff (and the twig file was missing too).

Either we use #markup, or we change the $content['next_steps'] to be integrated with $content['authorize_report'].

About the $GLOBALS['base_url'], I know it's a bit dirty, but I think it's cleanier than using fromUri('base:../admin/modules') to get out of the parent core/ directory.

Install script is using it too, as it's also in the core/ directory http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Installer/Ex...

Thanks for the interdiff, let's sort that base_url stuff out in a different issue as it's unrelated to this and seems like a more fundamental problem. You have some include stuff added to the registry snuck into that patch as well, that should be removed.

Fixed @joelpittet suggestions in #146

When testing, should I apply the latest patch here, or the latest patch from #2042447: Install a module user interface does not install modules (or themes) first? Both apply cleanly on their own, but not when taken together.

Per the manual testing instructions, this needs to be rerolled b/c the two patches no longer apply together. Postponing until #2042447: Install a module user interface does not install modules (or themes) is in (it is currently RTBC). Also triggered a retest to see if the current patch still works.

Needs both for manual testing properly. Rerolled`

There was some unnecessary changes in the re-roll and moved report variable that the template expected to the definition and made use of the attributes object that was added.

Rerolled

Wrong patch rerolled, there shouldn't be:

+++ b/core/includes/theme.maintenance.inc
@@ -100,60 +100,40 @@ function _drupal_maintenance_theme() {
         $items[] = array(
           '#markup' => drupal_render($authorize_message),
           '#wrapper_attributes' => array('class' => $log_message['success'] ? 'authorize-results__success' : 'authorize-results__failure'),
         );

Anyway after manual testing, the "Next steps" links are always escaped, I will work on it

Again, I see no other way of handling $GLOBALS['base_url'], as in authorize.php, it is wrongly initialized in the Url generator.
In core/install.php, the same applies, and they directly use global $base_url;

So either we pass the correct absolute url from the batch result, or we initialize like it is done in core install.

Suggestions are welcome.

+++ b/core/lib/Drupal/Core/Updater/Module.php
@@ -108,11 +109,27 @@ public function getSchemaUpdates() {
+    $default_options = [
+      '#type'    => 'link',
+      '#options' => [
+        'absolute' => TRUE,
+        'base_url' => $GLOBALS['base_url'],
+      ],
+    ];
+    return [

If you are explicitly setting the base_url, should there be a test to ensure that the right link paths are being generated or is there already coverage for these?

I'm going to try to look at the base_url thing in general tonight.

OK, spoke for a while with @dawehner about this on IRC. Here is a comment that addresses the base_url usage. Didn't look at the fail.

We also agree that there needs to be test coverage on the links that generates to make sure they are correct.

I'm going to create an issue to address this, as having to use the global like this is just wrong, but unavoidable. It also happens in a few places in core.

Wouldn't call this a true followup, but created this, #2548095: Add option to Url() to force the site base_url to be used. The final version of the patch should probably reference this as a @todo so we can track the places we need to fix.

Just added the @todo for #2548095: Add option to Url() to force the site base_url to be used.

I agree with #2548095: Add option to Url() to force the site base_url to be used (the current behavior seems pretty nutty) but shouldn't we be dealing with the workaround in #2547667: Fix broken page title Update Manager (authorize.php) rather than this issue?

I'm going to take a quick look at the failing test...

#166, I didn't remember about that issue. It turns out to be a systemic problem, as #2540416: Move update.php back to a front controller currently has a similar hack, and would pop up again if we and another front controller (not sure why we would, but it's possible). So, it may be best to track base_url thing as a separate issue.

This took longer to figure out than it should have, but it looks like the code removed in #152 is still needed. (Without that, the preprocess function was not being called at all.) In addition to adding that back, I also reverted the change to the "report" variable from that patch as I think it is intended for the template only, not something the caller should pass in. This should pass tests now.

On the topic of the URLs, I now sort of see see why it's in this patch. Previously the double-escaping was fixed automatically by the rest of the code added in this issue, but now it requires changes to the URL generation too (I guess something must have changed with the SafeMarkup system in the meantime). So this is now touching the same code that #2547667: Fix broken page title Update Manager (authorize.php) would need to touch and it may therefore make sense to fix them both at once after all - in which issue, I'm not sure :) Whichever issue we fix it in should fix the theme installation links too, though (not just the module installation links), but I haven't done that here.

I also reverted the change to the "report" variable from that patch as I think it is intended for the template only, not something the caller should pass in. This should pass tests now.

The hook_theme definition acts as a contract that the template knows what variables to expect. The caller doesn't have to pass those variables in, but the template needs to have some assurance that it will be getting that variable passed in. That is why I did that.

Thank you very much @David_Rothstein for tracking down that test fail, that sounds like a tricky one, I wonder when that changed(if it did).

For context on why those links need to be changed. They get built up as links in the batch, but on the next request the list which was in SafeMarkup::safe_strings is lost. I thought we solved this some time ago, but I guess not. Regardlress, nice fix changing them to render arrays, because that side steps the early rendering of the links and lets the safety be dealt with on render instead of before.

I'm just going to clean up with removing the array value indenting. We don't do that normally for coding standards(even though I've spotted a bunch in core).
https://www.drupal.org/coding-standards#operators
https://www.drupal.org/coding-standards#array

I was testing things out and we missed the update task links converted to attribute links. Add those into the mix.

Will post manual testing screenshots next. Gotta do a bit of research how to fake batch + installer to authorize test...

Whoops missing use statement for Url.

Manual Testing:

Install:

Update: