Convert theme_views_ui_container to Twig

Moving to core queue.

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

Why you steal tags, bot?

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

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

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

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.

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.

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

:(

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

You can configure that :)

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

I'd run with this, to keep inline with #1913208: [policy] Standardize template preprocess function documentation.

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.

Tagging.

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.

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!

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

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

Trying again.

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.

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

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

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

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

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

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

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.

Needs a re-roll before profiling.

Re-rolling.

#39: 1915026-39-views-ui-container.patch queued for re-testing.

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

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

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"

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>

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.

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

tagging

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

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!

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

This one should work

dibs!

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

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

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

This patch uses String::format instead of format_string

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.

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

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