Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steveoliver’s picture

Title: Convert theme_views_ui_container to Twig. » Convert theme_views_ui_container to Twig
Project: » Drupal core
Version: » 8.x-dev
Component: Twig templates conversion (front-end branch) » views_ui.module
Assigned: Unassigned » steveoliver
Status: Active » Needs review
Issue tags: +Twig
FileSize
2.45 KB

Moving to core queue.

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, drupal-twig-views-ui-container--1915026-1.patch, failed testing.

steveoliver’s picture

Hmmm... someone wanna take a stab at this -- seems like it should be working.

steveoliver’s picture

Issue tags: +Twig

Why you steal tags, bot?

steveoliver’s picture

Status: Needs review » Needs work
FileSize
2.09 KB
 function template_preprocess_views_ui_display_tab_setting(&$variables) {
@@ -51,7 +51,7 @@ function template_preprocess_views_ui_display_tab_bucket(&$variables) {
   $variables['content'] = $element['#children'];
   $variables['title'] = $element['#title'];
-  $variables['actions'] = !empty($element['#actions']) ? render($element['#actions']) : '';
+  $variables['actions'] = !empty($element['#actions']) ? $element['#actions'] : '';
 }
 

This needs to be removed (is removed in updated patch). It was part of another patch.

Other than that, I don't see how this isn't passing tests...

steveoliver’s picture

Status: Needs work » Needs review

Expect it to fail, as it does locally. Still don't get why. Expect facepalm.

The last submitted patch, drupal-twig-views-ui-container--1915026-5.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -8,11 +8,11 @@
+ * Prepares variables for views-ui-container.html.twig.

Let's use "Preprocess variables for views-ui-container.html.twig" like template_preprocess_page/template_preprocess_node does it.

steveoliver’s picture

Re: #8: I know 'prepares' is a departure from what we've done historically, but maybe we consider using 'prepares variables' instead of 'preprocesses' to help clarify what it is we're doing in these functions? See #1825464: [meta] Documentation blocks for preprocess functions and #1913208: [policy] Standardize template preprocess function documentation.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.89 KB

The fails are from whitespace in the template somehow. So this should pass (changed the template). We should also wrap t() to assert the message text I think, as the markup text in the calling code is doing that.

dawehner’s picture

This looks perfect beside of one small thing.

+++ b/core/modules/views/views_ui/templates/views-ui-container.html.twigundefined
@@ -0,0 +1,16 @@
\ No newline at end of file

:(

damiankloip’s picture

Assigned: steveoliver » Unassigned
FileSize
2.86 KB

Ha, I blame that on PHPStorm, sublime did that for me ;)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

You can configure that :)

xjm’s picture

#12: 1915026-12.patch queued for re-testing.

steveoliver’s picture

steveoliver’s picture

Status: Reviewed & tested by the community » Needs review
star-szr’s picture

This is looking great so far, thanks to everyone who's worked on the issue!

Minor documentation nitpick:

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -8,11 +8,13 @@
+ * Prepares variables for Views UI containers.

Should be something more like "Prepares variables for Views UI container templates". Also the $variables are not documented at all.

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

thedavidmeister’s picture

Status: Needs review » Needs work

Review of #15:

+ * - attributes: An array of HTML attributes to apply to the container element.
+ * - children: An array of renderable elements such as dropbuttons and tabs.

We're not talking about PHP data types in Twig templates.

Preprocess needs an @param for the $variables array.

star-szr’s picture

Issue tags: +Novice

Tagging as a novice task for the documentation tweaks in #17 and #19. Also, @thedavidmeister and myself are talking about the same $variables array in both our comments, see #1913208: [policy] Standardize template preprocess function documentation for an example preprocess docblock that has the variables documented.

Here's the reference for PHP data types: http://drupal.org/node/1823416#datatypes

If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!

2ndmile’s picture

Assigned: Unassigned » 2ndmile
2ndmile’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
2.94 KB

Newb alert. Please review and let me know if I am in the ballpark implementing #17 and #19.

Status: Needs review » Needs work

The last submitted patch, 1915026-22.patch, failed testing.

2ndmile’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
3.12 KB

Disregard #22. Please review. Implementing #17 and #19.

Status: Needs review » Needs work

The last submitted patch, 1915026-23.patch, failed testing.

2ndmile’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
3.1 KB

Trying again.

Status: Needs review » Needs work

The last submitted patch, 1915026-26.patch, failed testing.

star-szr’s picture

Docs are looking much better here, thanks @2ndmile!

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -8,11 +8,19 @@
+  dsm($variables);

This is probably what is causing all the test failures because we only changed documentation between #15 and #26 from what I can see. And don't worry, it happens :)

I hate to say it after you've worked so hard on the preprocess docs @2ndmile but we might be able to lose the preprocess function by changing the hook_theme() definition to use 'variables' => array('children', 'attributes') instead of 'render element' (same function where the 'templates' key is being added). See #1939086-36: Convert theme_dropbutton_wrapper() to Twig.

dawehner’s picture

+++ b/core/modules/views/views_ui/views_ui.theme.incundefined
@@ -8,11 +8,19 @@
+  $variables['children'] = $variables['element']['#children'];

Afaik the core standard for "render element" templates is to use the same name: see theme_menu_tree, template_preprocess_menu_tree

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.09 KB
1.85 KB

@Cottser could you explain #28 a bit further? Would making that change just work or is that an API change? @dawehner are you refering to #28?

Here is a re-roll from #15 including all of @2ndmile changes, though the patches didn't apply well so the interdiff is from #15 and manually grabbed the changes from those patches.

star-szr’s picture

Assigned: 2ndmile » Unassigned
Status: Needs review » Needs work

@joelpittet - thanks! Check the interdiff at http://drupal.org/node/1939086#comment-7294324, it should just work. This is only used in #theme_wrappers just like theme_dropbutton_wrapper() was. Not sure that it's an API change because it's called in exactly the same way - but maybe.

@dawehner it sounds like following that convention (#29) the template variable would be 'container' or 'views_ui_container', but I don't think it would make sense to do that. Only the 'attributes' are used for printing the container itself and the contents are printed inside the container. If we wanted to change the variable name 'content' or 'contents' maybe but then we wouldn't be able to get rid of the preprocess function :)

+++ b/core/modules/views/views_ui/templates/views-ui-container.html.twigundefined
@@ -0,0 +1,16 @@
+<div{{ attributes }}>{{ children }}</div>

CNW to try removing the preprocess function, this would be a good novice task. @2ndmile - unassigning for now, feel free to pick this back up if you have time :)
echeese’s picture

Assigned: Unassigned » echeese
Status: Needs work » Needs review
FileSize
3.04 KB

Rerolled the patch in #30 because it did not apply. I am working on a fix

Status: Needs review » Needs work

The last submitted patch, 1915026-32-views-ui-container.patch, failed testing.

echeese’s picture

Assigned: echeese » Unassigned
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
315 bytes
3.03 KB

Moved the template to the correct folder, tests should pass now.

Hydra’s picture

The path actualy changed of the views test, here is a new version

Hydra’s picture

Okay, manual testing was going well for the template file usage.
The test itself is dying when trying to run it but this might more be a VDC issue, then a Twig issue.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +needs profiling
joelpittet’s picture

Issue tags: +Novice, +Needs reroll

Needs a re-roll before profiling.

dsdeiz’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

Re-rolling.

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

The last submitted patch, 1915026-39-views-ui-container.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing, +needs profiling, +Twig, +Needs reroll, +VDC
joelpittet’s picture

Thank you @dsdeiz. Next we need to confirm output.

Here's a bit of a cleanup by removing the preprocess and Views to views.

waynethayer’s picture

Status: Needs review » Needs work

Tested manually from:
/admin/structure/views/view/user_admin_people

Wrapper div (div.views-display-top) is missing id and classes:
class="views-display-top clearfix" id="views-display-top"

waynethayer’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
1.15 KB
2.71 KB

Worked on this with @Cottser and found the bug. Classes and id are now being added to wrapper div.

Before:
<div class="views-display-top clearfix" id="views-display-top"><div class="dropbutton-wrapper dropbutton-processed dropbutton-multiple"><div class="dropbutton-widget"><ul id="views-display-extra-actions" class="dropbutton dropbutton-icon-processed"><li class="edit-details odd first dropbutton-action"><a href="/admin/structure/views/nojs/edit-details/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Edit view name/description</a></li><li class="dropbutton-toggle"><button type="button" role="button"><span class="dropbutton-arrow"><span class="visually-hidden">List additional actions</span></span></button></li><li class="analyze even dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/analyze/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Analyze view</a></li><li class="clone odd dropbutton-action secondary-action"><a href="/admin/structure/views/view/user_admin_people/clone">Clone view</a></li><li class="reorder even last dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/reorder-displays/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Reorder displays</a></li></ul></div></div><h2 class="visually-hidden">Secondary tabs</h2><ul id="views-display-menu-tabs" class="tabs secondary views-ui-render-add-view-button-processed-processed"><li class="active"><a href="/admin/structure/views/view/user_admin_people/edit/page_1">Page<span class="visually-hidden">(active tab)</span></a></li><li class="add"><a href="#"><span class="icon add"></span>Add</a><ul class="action-list" style="display:none;"><li class="last"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-attachment" name="op" value="Attachment"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-block" name="op" value="Block"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-embed" name="op" value="Embed"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-feed" name="op" value="Feed"></li><li class="first"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-page" name="op" value="Page"></li></ul></li></ul></div>

After:
<div class="views-display-top clearfix" id="views-display-top"><div class="dropbutton-wrapper dropbutton-processed dropbutton-multiple"><div class="dropbutton-widget"><ul id="views-display-extra-actions" class="dropbutton dropbutton-icon-processed"><li class="edit-details odd first dropbutton-action"><a href="/admin/structure/views/nojs/edit-details/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Edit view name/description</a></li><li class="dropbutton-toggle"><button type="button" role="button"><span class="dropbutton-arrow"><span class="visually-hidden">List additional actions</span></span></button></li><li class="analyze even dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/analyze/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Analyze view</a></li><li class="clone odd dropbutton-action secondary-action"><a href="/admin/structure/views/view/user_admin_people/clone">Clone view</a></li><li class="reorder even last dropbutton-action secondary-action"><a href="/admin/structure/views/nojs/reorder-displays/user_admin_people/page_1" class="views-ajax-link views-ajax-processed-processed">Reorder displays</a></li></ul></div></div><h2 class="visually-hidden">Secondary tabs</h2><ul id="views-display-menu-tabs" class="tabs secondary views-ui-render-add-view-button-processed-processed"><li class="active"><a href="/admin/structure/views/view/user_admin_people/edit/page_1">Page<span class="visually-hidden">(active tab)</span></a></li><li class="add"><a href="#"><span class="icon add"></span>Add</a><ul class="action-list" style="display:none;"><li class="last"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-attachment" name="op" value="Attachment"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-block" name="op" value="Block"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-embed" name="op" value="Embed"></li><li><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-feed" name="op" value="Feed"></li><li class="first"><input class="add-display button form-submit" formnovalidate="formnovalidate" type="submit" id="edit-displays-top-add-display-page" name="op" value="Page"></li></ul></li></ul></div>

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -needs profiling

Here are some profiling results:

Scenario:

Stark, /admin/structure/views/view/user_admin_people/

=== 8.x..8.x compared (51c6814c41b47..51c681e794599):

ct  : 214,201|214,201|0|0.0%
wt  : 1,111,506|1,108,867|-2,639|-0.2%
cpu : 1,066,761|1,065,171|-1,590|-0.1%
mu  : 19,120,568|19,120,568|0|0.0%
pmu : 19,321,936|19,321,936|0|0.0%


http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c6814c41b47&...


=== 8.x..1915026-twig-views-ui-container compared (51c6814c41b47..51c6824f48b42):

ct  : 214,201|214,280|79|0.0%
wt  : 1,111,506|1,108,512|-2,994|-0.3%
cpu : 1,066,761|1,065,633|-1,128|-0.1%
mu  : 19,120,568|19,144,880|24,312|0.1%
pmu : 19,321,936|19,346,968|25,032|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51c6814c41b47&...

RTBC as per #13 and the profiling results are good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views_ui/lib/Drupal/views_ui/Tests/DisplayTest.phpundefined
@@ -251,7 +251,7 @@ public function testDisplayAreas() {
+      $this->assertEqual((string) $element[0], t('The selected display type does not utilize @type plugins', array('@type' => $type)));

This should use format_string() and not t()... t() should not be used in tests.

jenlampton’s picture

Issue tags: +Novice

tagging

eromero1’s picture

FileSize
2.71 KB

Took a shot at rerolling it. Hopefully it runs smoother than egyptian silk.

sbudker1’s picture

Patch seemed to work! I did not fine any errors in my testing. I dont know if it was as smooth as silk though ;). Good Work!

sbudker1’s picture

Status: Needs work » Reviewed & tested by the community
star-szr’s picture

Status: Reviewed & tested by the community » Needs work

#48 is identical to #44, we still need to change the t() to format_string() like @alexpott mentioned in #46 :)

eromero1’s picture

Status: Needs work » Needs review
FileSize
2.72 KB

This one should work

sbudker1’s picture

dibs!

sbudker1’s picture

Patch worked! Built a view and content showed up normally, I did not come across any problems.

sbudker1’s picture

Status: Needs review » Reviewed & tested by the community

Patch worked! Built a view and content showed up normally, I did not come across any problems.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs work

Sorry.. We should use String::format() and not format_string().

rvilar’s picture

Assigned: Unassigned » rvilar
Status: Needs work » Needs review
FileSize
2.72 KB

This patch uses String::format instead of format_string

Status: Needs review » Needs work

The last submitted patch, twig-themes-views-ui-container-1915026-57.patch, failed testing.

drupalninja99’s picture

Status: Needs work » Needs review
FileSize
630 bytes
2.89 KB

It looks like the tests were failing bc String::format didn't have this use statement:

use Drupal\Component\Utility\String;

I added this line and re-ran the failing test locally and it passed. Attaching the re-rolled patch.

azinoman’s picture

Assigned: rvilar » Unassigned
Status: Needs review » Reviewed & tested by the community

This patch worked! Added a view and things looked fine! Changing to reviewed and tested by the community!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 637beb9 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.