Task

Use Twig instead of PHPTemplate

Remaining

  • none
Theme function name Conversion status
theme_authorize_message Removed in #75
theme_authorize_report converted
theme_install_page To be removed via #1885714: Remove theme_install_page()
theme_task_list To be converted via #2329505: Convert theme_task_list to Twig template
theme_update_page Removed by #1885850: Remove theme_update_page()!

Manual testing steps

#1757550: [Meta] Convert core theme functions to Twig templates
#1885714: Remove theme_install_page()
#1885850: Remove theme_update_page()
#1189822: Convert maintenance-page.html.twig to HTML5

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug Report
Issue priority Major
Unfrozen changes Unfrozen because it a bug fix
Prioritized changes yes because it's a bug fix.
Disruption nope
Files: 
CommentFileSizeAuthor
#189 interdiff.txt930 bytesjoelpittet
#189 theme_maintenance_inc-1885564-189.patch12.2 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,529 pass(es). View
#181 interdiff-176-181.txt3.9 KBmpdonadio
#181 theme_maintenance_inc-1885564-181.patch12.03 KBmpdonadio
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,552 pass(es). View
#181 theme_maintenance_inc-1885564-test-only.patch1.15 KBmpdonadio
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,516 pass(es), 6 fail(s), and 0 exception(s). View
#176 interdiff.txt2.51 KBDavid_Rothstein
#176 theme_maintenance_inc-1885564-176.patch11.67 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,468 pass(es). View
#175 interdiff.txt3.42 KBDavid_Rothstein
#175 theme_maintenance_inc-1885564-175.patch11.52 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,460 pass(es). View
#175 theme_maintenance_inc-1885564-175-TESTS-ONLY.patch867 bytesDavid_Rothstein
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,463 pass(es), 1 fail(s), and 0 exception(s). View
#174 theme_maintenance_inc-1885564-174.patch8.79 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,456 pass(es). View
#171 theme_maintenance_inc-1885564-171.patch8.3 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,452 pass(es). View
#171 interdiff.txt1003 bytesjoelpittet
#170 theme_maintenance_inc-1885564-170.patch7.46 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,472 pass(es). View
#170 interdiff.txt3.21 KBjoelpittet
#168 interdiff.txt2.28 KBDavid_Rothstein
#168 theme_maintenance_inc-1885564-168.patch7.5 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,468 pass(es). View
#164 interdiff-161-164.txt600 bytesmpdonadio
#164 theme_maintenance_inc-1885564-164.patch6.73 KBmpdonadio
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,457 pass(es), 1 fail(s), and 0 exception(s). View
#161 interdiff-158-161.txt648 bytesmpdonadio
#161 theme_maintenance_inc-1885564-161.patch6.67 KBmpdonadio
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,447 pass(es), 1 fail(s), and 0 exception(s). View
#158 1885564-158-theme_maintenance_to_twig.patch6.49 KBSebCorbin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,420 pass(es), 1 fail(s), and 0 exception(s). View
#158 interdiff.txt2.78 KBSebCorbin
#155 1885564-155.patch4.41 KBSebCorbin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,400 pass(es), 1 fail(s), and 0 exception(s). View
#152 interdiff.txt2.55 KBjoelpittet
#151 1885564-151.patch5.34 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,580 pass(es). View
#147 interdiff.txt3.12 KBrteijeiro
#147 theme_maintenance_inc-1885564-147.patch5.34 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,587 pass(es). View
#145 interdiff.txt4.55 KBSebCorbin
#145 theme_maintenance_inc-1885564-145.patch8.6 KBSebCorbin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,828 pass(es). View
#143 Screen Shot 2015-02-14 at 19.27.48.png53.18 KBSebCorbin
#139 theme_maintenance_inc-1885564-139.patch4.46 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,831 pass(es). View
#136 1885564-theme_maintenance_inc-136.patch4.69 KBDevin Carlson
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,419 pass(es). View
#134 Authorize_file_system_changes___Site-Install.png137.43 KBjoelpittet
#133 1885564-theme_maintenance_inc-133.patch4.65 KBakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,032 pass(es). View
#130 Screen Shot 2014-09-24 at 22.16.28.png79.77 KBlauriii
#130 Screen Shot 2014-09-24 at 22.16.39.png124.24 KBlauriii
#129 theme_maintenance_inc-1885564-129.patch4.39 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,567 pass(es). View
#125 1885564-125-authorize-after.png52.86 KBCottser
#125 1885564-125-authorize-before.png117.11 KBCottser
#121 interdiff.txt719 bytesjoelpittet
#121 1885564-twig-theme-maintenance-121.patch4.38 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,289 pass(es). View
#119 interdiff.txt4.08 KBjoelpittet
#119 1885564-twig-theme-maintenance-119.patch4.38 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,044 pass(es), 51 fail(s), and 3,193 exception(s). View
#118 1885564-twig-theme-maintenance-118.patch7.39 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,774 pass(es). View
#115 1885564-twig-theme_maintenance-115_4.patch7.15 KBSebCorbin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,905 pass(es). View
#114 1885564-twig-theme_maintenance-114_3.patch7.27 KBSebCorbin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,855 pass(es). View
#97 1885564-twig-theme_maintenance-98.patch7.04 KBSebCorbin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,754 pass(es). View
#95 1885564-twig-theme_maintenance-95.patch5.29 KBSebCorbin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,492 pass(es), 214 fail(s), and 39 exception(s). View
#90 1885564-twig-theme_maintenance-90.patch7.04 KBtrevorkjorlien
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,272 pass(es). View
#85 1885564-twig-theme_maintenance-85.patch7.05 KBaboros
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,809 pass(es). View
#79 1885564-twig-theme_maintenance-79.patch7.18 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 64,483 pass(es). View
#75 1885564-75-twig-theme-maintenance.patch7.1 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 58,763 pass(es). View
#75 interdiff.txt2.47 KBjoelpittet
#62 1885564-62-twig-theme-maintenance.patch7.7 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 57,953 pass(es), 2 fail(s), and 1 exception(s). View
#62 interdiff.txt3.64 KBjoelpittet
#60 interdiff.txt3.61 KBjoelpittet
#60 1885564-60-twig-theme-maintenance.patch332.28 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 57,905 pass(es). View
#54 interdiff.txt2.93 KBjoelpittet
#54 1885564-54-twig-theme-maintenance.patch7.92 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#47 twig-maintainence-page-theme-only-1885564-47.patch7.8 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 57,342 pass(es). View
#47 interdiff.txt523 bytesdrupalninja99
#44 twig-maintainence-page-theme-only-1885564-44.patch7.64 KBdrupalninja99
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#44 interdiff.txt614 bytesdrupalninja99
#36 twig-maintainence-page-theme-only-1885564-36.patch7.71 KBsocketwench
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#29 twig-maintainence-page-theme-only-1885564-29.patch8.02 KBshanethehat
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-maintainence-page-theme-only-1885564-29.patch. Unable to apply patch. See the log in the details link for more information. View
#25 1885564-25.patch14.13 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 54,199 pass(es). View
#25 interdiff.txt8.24 KBCottser
#23 1885564-23.patch7.9 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,901 pass(es). View
#23 interdiff.txt2.84 KBCottser
#22 1885564-22.patch7.51 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 53,952 pass(es), 1 fail(s), and 0 exception(s). View
#22 interdiff.txt5.01 KBCottser
#21 1885564-21.patch8.58 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,930 pass(es). View
#21 interdiff.txt6.08 KBCottser
#18 1885564-18.patch8.36 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 53,657 pass(es). View
#14 1885564-14.patch5.05 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 49,659 pass(es). View
#14 interdiff.txt5.33 KBCottser
#10 1885564-10.patch8.36 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#10 interdiff.txt2.72 KBCottser
#3 core-replace_theme_funcs_in_maintenance_inc_with_twig-1885564-3.patch8.16 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#1 core-replace_theme_funcs_in_maintenance_inc_with_twig-1885564-1.patch8.01 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

jenlampton’s picture

Issue summary: View changes

added changes

jenlampton’s picture

Status: Active » Needs review
FileSize
8.01 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

There is some weirdness in this patch that works - but maybe there's a better way to do these things (feedback appreciated):

- in theme.inc, drupal_common_theme, I had to add a bunch of 'preprocess functions' definitions to get the preprocess functions to run for these new higher-level bootstrap templates.

- in theme.maintenance.inc both template_preprocess_install_page and template_preprocess_update_page I needed to change the template file called to maintenance-page, and did so by setting $variables['theme_hook_original'].

Status: Needs review » Needs work
Anonymous’s picture

Issue summary: View changes

related

jenlampton’s picture

Status: Needs work » Needs review
FileSize
8.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

okay, I may be out of my depth a bit here. I was able to get these changes working in our sandbox to confirm that the twig templates are rendering correctly, but I may not know how to apply these changes directly to core. I could use some help :)

Here's how I got these working in our sandbox:

1) changed the maintenance theme in settings.php to stark.
2) changed stark to use twig as it's theme engine.
3a) add the template files into the stark theme.
3b) update theme.inc to include new information about preprocess functions
3c) replace the theme functions in theme.maintenance.inc with preprocess functions

Visit the installer, confirm that stark is the theme being used.
Change the twig templates to confirm that they are being rendered.
Everything works.

So now for core...

1) I assume I don't need to change the maintenance theme, we want this to work with seven (?)
2) I assume I dont have to change seven to use twig as the engine, twig templates should work anyway (?)
3a) add the template files into the system module since this is where we actually want them
3b) update theme.inc to include new information about preprocess functions
3c) replace the theme functions in theme.maintenance.inc with preprocess functions

Visit the installer, utter fail.

Attached patch should still fail because of my confusion about the above, but fixes a missing use Attribute line.

Status: Needs review » Needs work
Anonymous’s picture

Issue summary: View changes

related

jenlampton’s picture

Issue summary: View changes

clean up

c4rl’s picture

Title: Replace all theme functions in theme.maintenance.inc with Twig templates » Convert theme.maintenance.inc to Twig

More general title. Updated summary.

koppie’s picture

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

mbrett5062’s picture

@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;
Cottser’s picture

Assigned: jenlampton » Cottser
Cottser’s picture

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
8.36 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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'];
Cottser’s picture

Assigned: Cottser » Unassigned

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

Cottser’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 1885564-10.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
5.05 KB
PASSED: [[SimpleTest]]: [MySQL] 49,659 pass(es). View

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.

Cottser’s picture

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

c4rl’s picture

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.

c4rl’s picture

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

Cottser’s picture

FileSize
8.36 KB
PASSED: [[SimpleTest]]: [MySQL] 53,657 pass(es). View

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

Cottser’s picture

Assigned: Unassigned » Cottser
Status: Needs review » Needs work

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

Cottser’s picture

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
8.58 KB
PASSED: [[SimpleTest]]: [MySQL] 53,930 pass(es). View

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.

Cottser’s picture

FileSize
5.01 KB
7.51 KB
FAILED: [[SimpleTest]]: [MySQL] 53,952 pass(es), 1 fail(s), and 0 exception(s). View

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

Cottser’s picture

FileSize
2.84 KB
7.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,901 pass(es). View

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.

Cottser’s picture

Issue summary: View changes

Improve summary

Cottser’s picture

Status: Needs review » Needs work

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

Cottser’s picture

Issue summary: View changes

Note status of theme_install_page() and theme_update_page().

Cottser’s picture

Issue summary: View changes

Add conversion summary table

Cottser’s picture

Status: Needs work » Needs review
FileSize
8.24 KB
14.13 KB
PASSED: [[SimpleTest]]: [MySQL] 54,199 pass(es). View

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.

Cottser’s picture

Issue summary: View changes

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

Cottser’s picture

Issue tags: +Needs manual testing

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

Cottser’s picture

Issue summary: View changes

Update conversion table, add testing steps

Cottser’s picture

Assigned: Cottser » Unassigned

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.

Cottser’s picture

Issue summary: View changes

remove sandbox

Cottser’s picture

Issue summary: View changes

Update conversion table

c4rl’s picture

Title: Convert theme.maintenance.inc to Twig » theme.maintenance.inc - Convert theme_ functions to Twig

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.

c4rl’s picture

Issue summary: View changes

Another tweak

c4rl’s picture

Issue summary: View changes

Updated issue summary.

shanethehat’s picture

FileSize
8.02 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch twig-maintainence-page-theme-only-1885564-29.patch. Unable to apply patch. See the log in the details link for more information. View
intergalactic overlords’s picture

Status: Needs review » Needs work

The last submitted patch, twig-maintainence-page-theme-only-1885564-29.patch, failed testing.

socketwench’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, twig-maintainence-page-theme-only-1885564-29.patch, failed testing.

hanpersand’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +Twig

The last submitted patch, twig-maintainence-page-theme-only-1885564-29.patch, failed testing.

socketwench’s picture

Status: Needs work » Needs review
FileSize
7.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Reroll!

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, twig-maintainence-page-theme-only-1885564-36.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig
johnshortess’s picture

Assigned: Unassigned » johnshortess

Manually testing this now...

aspilicious’s picture

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

Status: Needs review » Needs work

The last submitted patch, twig-maintainence-page-theme-only-1885564-36.patch, failed testing.

johnshortess’s picture

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.

Cottser’s picture

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

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
614 bytes
7.64 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

I removed the preprocess function line.

Status: Needs review » Needs work

The last submitted patch, twig-maintainence-page-theme-only-1885564-44.patch, failed testing.

drupalninja99’s picture

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.

drupalninja99’s picture

FileSize
523 bytes
7.8 KB
PASSED: [[SimpleTest]]: [MySQL] 57,342 pass(es). View

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

drupalninja99’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -Twig

The last submitted patch, twig-maintainence-page-theme-only-1885564-47.patch, failed testing.

drupalninja99’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +Twig
Cottser’s picture

Assigned: johnshortess » Unassigned
Issue tags: +Needs profiling

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

thedavidmeister’s picture

Issue tags: +Novice

manual testing is a novice task.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll before it can be tested or profiled.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.92 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
2.93 KB

Cleaned up a bit and re-rolled.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -Needs profiling, -Twig

The last submitted patch, 1885564-54-twig-theme-maintenance.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1885564-54-twig-theme-maintenance.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Needs profiling, +Twig

The last submitted patch, 1885564-54-twig-theme-maintenance.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
332.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,905 pass(es). View
3.61 KB

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

Cottser’s picture

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.64 KB
7.7 KB
FAILED: [[SimpleTest]]: [MySQL] 57,953 pass(es), 2 fail(s), and 1 exception(s). View

Whoops

royko_at_duo’s picture

Assigned: Unassigned » royko_at_duo
royko_at_duo’s picture

Assigned: royko_at_duo » Unassigned

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

royko_at_duo’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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.

Cottser’s picture

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.

catch’s picture

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

Cottser’s picture

Issue tags: -Needs profiling

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.

cosmicdreams’s picture

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.

Cottser’s picture

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

Cottser’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 62: 1885564-62-twig-theme-maintenance.patch, failed testing.

joelpittet’s picture

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.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.47 KB
7.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,763 pass(es). View

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.

Cottser’s picture

Issue tags: +Twig conversion
Cottser’s picture

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.18 KB
PASSED: [[SimpleTest]]: [MySQL] 64,483 pass(es). View

Rerolled.

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

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

          $message = '' . $log_message['message'] . '';
          $classes = array('failure');

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

joelpittet’s picture

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

Cottser’s picture

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

joelpittet’s picture

longwave’s picture

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

joelpittet’s picture

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

aboros’s picture

FileSize
7.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,809 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, 85: 1885564-twig-theme_maintenance-85.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

trevorkjorlien’s picture

Assigned: Unassigned » trevorkjorlien

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

Please be kind!

trevorkjorlien’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,272 pass(es). View

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

Status: Needs review » Needs work

The last submitted patch, 90: 1885564-twig-theme_maintenance-90.patch, failed testing.

trevorkjorlien’s picture

Status: Needs work » Needs review
vollepeer’s picture

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

joelpittet’s picture

@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.

SebCorbin’s picture

Assigned: trevorkjorlien » SebCorbin
FileSize
5.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,492 pass(es), 214 fail(s), and 39 exception(s). View
+++ 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

SebCorbin’s picture

Status: Needs review » Needs work

Just resetting the status to get the patch tested

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
7.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,754 pass(es). View

.twig files were missing

The last submitted patch, 95: 1885564-twig-theme_maintenance-95.patch, failed testing.

LewisNyman’s picture

Issue tags: +frontend

Are the needs tags here still relevant?

joelpittet’s picture

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".

lauriii’s picture

steinmb’s picture

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

Cottser’s picture

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.

mbrett5062’s picture

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

Cottser’s picture

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

mbrett5062’s picture

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.

Cottser’s picture

@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 :)

mbrett5062’s picture

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.

mbrett5062’s picture

FileSize
3.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1885564_0.diff. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 109: 1885564.diff, failed testing.

joelpittet’s picture

@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.

Cottser’s picture

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."

SebCorbin’s picture

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)

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
7.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,855 pass(es). View

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:

SebCorbin’s picture

FileSize
7.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,905 pass(es). View

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

Cottser’s picture

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?

SebCorbin’s picture

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'
joelpittet’s picture

FileSize
7.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,774 pass(es). View

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

joelpittet’s picture

Assigned: SebCorbin » Unassigned
FileSize
4.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,044 pass(es), 51 fail(s), and 3,193 exception(s). View
4.08 KB

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

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

FileSize
4.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,289 pass(es). View
719 bytes

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.

The last submitted patch, 119: 1885564-twig-theme-maintenance-119.patch, failed testing.

joelpittet’s picture

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

Cottser’s picture

Title: theme.maintenance.inc - Convert theme_ functions to Twig » theme.maintenance.inc (authorize.php) - Convert theme_ functions to Twig
Issue summary: View changes
Issue tags: -Needs steps to reproduce

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

Cottser’s picture

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

  • webchick committed 511d9f7 on 8.0.x
    Issue #1885564 by Cottser, SebCorbin, joelpittet, drupalninja99,...
Cottser’s picture

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

  • webchick committed df9431d on 8.0.x
    Revert "Issue #1885564 by Cottser, SebCorbin, joelpittet, drupalninja99...
lauriii’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,567 pass(es). View

Did small reroll for this

lauriii’s picture

Here's few screenshots from the page

mdrummond’s picture

Status: Needs review » Fixed

Wrong issue, laurii?

Cottser’s picture

Status: Fixed » Needs review

Right issue, see the last handful of comments.

akalata’s picture

FileSize
4.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,032 pass(es). View

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.

joelpittet’s picture

Status: Needs review » Needs work
FileSize
137.43 KB

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.

joelpittet’s picture

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.

Devin Carlson’s picture

FileSize
4.69 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,419 pass(es). View

Reroll of #133.

Devin Carlson’s picture

Status: Needs work » Needs review
Devin Carlson’s picture

Status: Needs review » Needs work
joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,831 pass(es). View

Re-rolling for the markup/class changes.

joelpittet’s picture

Status: Needs review » Needs work
+++ 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.

joelpittet’s picture

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.

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,853 pass(es). View

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

SebCorbin’s picture

Issue summary: View changes
FileSize
53.18 KB

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

joelpittet’s picture

@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?

SebCorbin’s picture

FileSize
8.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,828 pass(es). View
4.55 KB

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

joelpittet’s picture

Status: Needs review » Needs work

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.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
5.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,587 pass(es). View
3.12 KB

Fixed @joelpittet suggestions in #146

akalata’s picture

Status: Needs review » Needs work

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.

mpdonadio’s picture

Status: Needs work » Postponed
Issue tags: +Needs reroll

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.

joelpittet’s picture

Status: Postponed » Needs review
Issue tags: -Needs reroll
FileSize
5.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,580 pass(es). View

Needs both for manual testing properly. Rerolled`

joelpittet’s picture

FileSize
4.48 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1885564-152.patch. Unable to apply patch. See the log in the details link for more information. View
2.55 KB

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.

SebCorbin queued 152: 1885564-152.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 152: 1885564-152.patch, failed testing.

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
4.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,400 pass(es), 1 fail(s), and 0 exception(s). View

Rerolled

SebCorbin’s picture

Assigned: Unassigned » SebCorbin
Status: Needs review » Needs work

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

The last submitted patch, 155: 1885564-155.patch, failed testing.

SebCorbin’s picture

Status: Needs work » Needs review
FileSize
2.78 KB
6.49 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,420 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 158: 1885564-158-theme_maintenance_to_twig.patch, failed testing.

mpdonadio’s picture

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

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
6.67 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,447 pass(es), 1 fail(s), and 0 exception(s). View
648 bytes

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.

mpdonadio’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 161: theme_maintenance_inc-1885564-161.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
6.73 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,457 pass(es), 1 fail(s), and 0 exception(s). View
600 bytes

Status: Needs review » Needs work

The last submitted patch, 164: theme_maintenance_inc-1885564-164.patch, failed testing.

David_Rothstein’s picture

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

mpdonadio’s picture

#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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
7.5 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,468 pass(es). View
2.28 KB

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.

joelpittet’s picture

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.

joelpittet’s picture

FileSize
3.21 KB
7.46 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,472 pass(es). View

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

joelpittet’s picture

FileSize
1003 bytes
8.3 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,452 pass(es). View

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

joelpittet’s picture

Assigned: SebCorbin » Unassigned
Issue tags: -Needs manual testing
FileSize
8.45 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,458 pass(es). View
520 bytes

Whoops missing use statement for Url.

joelpittet’s picture

Manual Testing:

Install:

Update:

joelpittet’s picture

FileSize
983 bytes
8.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,456 pass(es). View

That "Run database updates" link needs that base URL thing *facepalm* I bet the other two links in there need that too, but not sure how to manually test that, any ideas?

I lucked out because have an older version of devel laying about to upgrade. But to test that you can just change the version number in the .info.yml and remember to enable the module (or go to the settings and enabled checking of disabled modules http://d8.dev/admin/reports/updates/settings)

David_Rothstein’s picture

FileSize
867 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,463 pass(es), 1 fail(s), and 0 exception(s). View
11.52 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,460 pass(es). View
3.42 KB

That "Run database updates" link needs that base URL thing *facepalm* I bet the other two links in there need that too, but not sure how to manually test that, any ideas?

Fixed those in the attached (I didn't come up with a way to test them manually except by hacking the code in authorize.php to force them to appear).

I also fixed the theme installation links, as mentioned in my earlier comment.

Finally, I wrote an automated test for the module installation links (one of the links, at any rate). Automated tests for module update links and theme installation/update links would be possible after #2548511: Write tests for the Update Manager downloading and applying updates for a module and #2548519: Extend Update Manager tests to handle installing/updating themes in addition to modules, respectively.

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.

Hm, I see the point, but don't some templates have tons and tons of variables? I don't believe this is something we've ever enforced before, and it has the side effect of allowing/encouraging people to pass them in when they're not supposed to. (I think of hook_theme() more as a contract for code which is building up data and passing it in to the rendering system, personally.)

However would a simple way to make everyone happy be to just get rid of the 'report' variable entirely? I'm not sure it's necessary - messages get passed in and the template needs messages. The 'messages' variable could just be restructured by the preprocess function to be what the template expects.

David_Rothstein’s picture

FileSize
11.67 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,468 pass(es). View
2.51 KB

However would a simple way to make everyone happy be to just get rid of the 'report' variable entirely? I'm not sure it's necessary - messages get passed in and the template needs messages. The 'messages' variable could just be restructured by the preprocess function to be what the template expects.

Like this, I mean.

The last submitted patch, 175: theme_maintenance_inc-1885564-175-TESTS-ONLY.patch, failed testing.

joelpittet’s picture

However would a simple way to make everyone happy be to just get rid of the
'report' variable entirely?

Yes that would make me happy:) was thinking similar thing.

Will review changes in the morning thanks for hack fixing those!

joelpittet’s picture

Hm, I see the point, but don't some templates have tons and tons of variables? I don't believe this is something we've ever enforced before, and it has the side effect of allowing/encouraging people to pass them in when they're not supposed to. (I think of hook_theme() more as a contract for code which is building up data and passing it in to the rendering system, personally.)

It also acts as default values for those templates with many variables(or they end up being dumped into 'render element' when things wild. Which are well/sane starting points that the template can fallback on which is making those variables optional. A real interface would really help this (D9). FYI, I've been pitching that "Define(hook_theme) what the template is expecting, build (render array), theme/markup(template)" in our hook_theme to twig template theme system talks. I could be wrong, but it's likely nobody paid attention to my blatherings:P. The render array building frequently skips variables: take item_list for example, the title and type keys are usually skipped as 90% of the time you are just spitting out a UL list.

I'd RTBC this, but I think we should get another set of eyes on the matter. Thanks for the fixes they look great to me.

@mpdonadio Any thoughts on further automated tests since you tagged it?

mpdonadio’s picture

+++ b/core/modules/update/src/Tests/UpdateUploadTest.php
@@ -75,6 +75,10 @@ public function testUploadModule() {
+    // Check that the link to install another module appears on the page and
+    // that the user can access it.
+    $this->clickLink(t('Install another module'));
+    $this->assertResponse(200);
   }

This doesn't really test that the link is correct, just that is a valid page. Maybe


$this->assertLinkByHref(Url::fromRoute('update.module_install')->toString());
$this->clickLink(t('Install another module'));
$this->assertResponse(200);

would be better? I'll post a patch later tonight if I have time (not on dev machine right now).

mpdonadio’s picture

Issue tags: -Needs tests
FileSize
1.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,516 pass(es), 6 fail(s), and 0 exception(s). View
12.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,552 pass(es). View
3.9 KB

These test changes work locally for me, and think they will be good in the long run and prevent any regressions involving fixing the base_url @todo.

Also changed uses of `new Url()` in the patch to `Url::fromRoute()`, which is the preferred usage per the docblock on Url::__construct().

The last submitted patch, 181: theme_maintenance_inc-1885564-test-only.patch, failed testing.

joelpittet’s picture

RTBC++, thanks for the test coverage @mpdonadio!

I'll find someone to do one last look through/test and hopefully RTBC.

mpdonadio’s picture

FileSize
12.01 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,520 pass(es). View
1.03 KB

Just updating the comment above the new assertions to reflect what they do.

akalata’s picture

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

Looks good here!

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -442,6 +442,13 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
+        // Load the includes, as they may contain preprocess functions.
+        if (isset($info['includes'])) {
+          foreach ($info['includes'] as $include_file) {
+            include_once $this->root . '/' . $include_file;
+          }
+        }

It took me a minute to figure out why these changes to Registry.php were needed; they were added in #145, suggested to be removed in #146, and added back in #168 when it was realized that they were actually necessary.

I tested both locally and on simpletest.me following the manual testing steps. Errors were successfully reported both times (different errors, was unable to get a success), visual result and HTML is identical to HEAD.

joelpittet’s picture

@SebCorbin Sorry I didn't realize you fixed a few of these things before! Back in February before all that crazy url stuff even happened(I think you may be able to predict the future and will bring you to the track next time I meet you)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
-    $this->clickLink(t('Install another module'));
-    $this->assertResponse(200);
+    $this->assertLink(t('Install another module'));
+    $this->assertLinkByHref(Url::fromRoute('update.module_install')->toString());
+    $this->assertLink(t('Enable newly added modules'));
+    $this->assertLinkByHref(Url::fromRoute('system.modules_list')->toString());
+    $this->assertLink(t('Administration pages'));
+    $this->assertLinkByHref(Url::fromRoute('system.admin')->toString());

I'm on board with the new tests, but not with removing the previous ones. The main reason is that assertLinkByHref() does partial link matching, so if you ask it to find "admin/modules" but in fact "admin/modules/install" is the only link on the page, it will return TRUE even though the link you're looking for isn't there. So it's a little fragile to rely on that alone. It's fine to have them and is a good way to check multiple links, but I just think we should keep the clickLink() + assertResponse() for that one link also, as it actually ensures that the link takes you somewhere on the site.

(For what it's worth, we could add a $this->assertUrl('admin/modules/install') after clicking, and then we'd also be checking that it took you to the expected location... that is probably worth doing.)

I don't have time to do a patch right now, but should be a pretty easy change to get this back to RTBC.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
885 bytes
12.21 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,531 pass(es). View

@David_Rothstein I agree, here's the patch to add it back.

joelpittet’s picture

FileSize
12.2 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,529 pass(es). View
930 bytes

Without the redundancy.

akalata’s picture

Status: Needs review » Reviewed & tested by the community

Confirming #189 addresses concerns from #187.

joelpittet’s picture

Category: Task » Bug report
Priority: Normal » Major
Issue summary: View changes

Changing to a bug report because our tests prove as much in #181 and adding beta evaluation.

joelpittet’s picture

Issue summary: View changes

  • webchick committed 6bca825 on 8.0.x
    Issue #1885564 by joelpittet, SebCorbin, Cottser, mpdonadio,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks a lot for this. This addresses some pretty big user-facing bugs in Update Manager (yay for expanded test coverage).

Overall this looks like some great consolidation from the theming side. I'm very sad about hunks like this:

-  $results['tasks'][] = t('<a href="@update">Run database updates</a>', array('@update' => \Drupal::url('system.db_update')));
+  $results['tasks'][] = [
+    '#type' => 'link',
+    '#url' => Url::fromRoute('system.db_update'),
+    '#title' => t('Run database updates'),
+    // Since this is being called outsite of the primary front controller,
+    // the base_url needs to be set explicitly to ensure that links are
+    // relative to the site root.
+    // @todo Simplify with https://www.drupal.org/node/2548095
+    '#options' => [
+      'absolute' => TRUE,
+      'base_url' => $GLOBALS['base_url'],
+    ],
+  ];

...as you now need to write like 10 lines of PHP to make a link. :( But that wasn't invented here. Thanks for spinning off the follow-up issue to look into how to make links like this a bit fewer lines of code from now on. :)

No complaints here!

Committed and pushed to 8.0.x. YEAH!

Status: Fixed » Closed (fixed)

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