Files: 
CommentFileSizeAuthor
#59 twig-themes-views-ui-container-1915026-59.patch2.89 KBdrupalninja99
PASSED: [[SimpleTest]]: [MySQL] 57,122 pass(es).
[ View ]
#59 interdiff.txt630 bytesdrupalninja99
#57 twig-themes-views-ui-container-1915026-57.patch2.72 KBrvilar
FAILED: [[SimpleTest]]: [MySQL] 56,747 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#52 twig-1915026-52.patch2.72 KBeromero1
PASSED: [[SimpleTest]]: [MySQL] 58,269 pass(es).
[ View ]
#48 twig-1915026-48.patch2.71 KBeromero1
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#44 1915026-44.patch2.71 KBwaynethayer
PASSED: [[SimpleTest]]: [MySQL] 58,134 pass(es).
[ View ]
#44 interdiff.txt1.15 KBwaynethayer
#42 1915026-42-twig-views-ui-container.patch2.71 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 56,121 pass(es).
[ View ]
#42 interdiff.txt2.21 KBjoelpittet
#39 1915026-39-views-ui-container.patch2.97 KBdsdeiz
PASSED: [[SimpleTest]]: [MySQL] 57,322 pass(es).
[ View ]
#35 1915026-35-views-ui-container.patch3.03 KBHydra
PASSED: [[SimpleTest]]: [MySQL] 55,792 pass(es).
[ View ]
#34 1915026-34-views-ui-container.patch3.03 KBecheese
PASSED: [[SimpleTest]]: [MySQL] 55,797 pass(es).
[ View ]
#34 interdiff.txt315 bytesecheese
#32 1915026-32-views-ui-container.patch3.04 KBecheese
FAILED: [[SimpleTest]]: [MySQL] 55,482 pass(es), 47 fail(s), and 625 exception(s).
[ View ]
#30 interdiff.txt1.85 KBjoelpittet
#30 1915026-30-twig-views-ui-container.patch3.09 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 55,235 pass(es).
[ View ]
#26 1915026-26.patch3.1 KB2ndmile
FAILED: [[SimpleTest]]: [MySQL] 54,014 pass(es), 340 fail(s), and 63 exception(s).
[ View ]
#26 interdiff.txt1.32 KB2ndmile
#24 1915026-23.patch3.12 KB2ndmile
FAILED: [[SimpleTest]]: [MySQL] 54,123 pass(es), 332 fail(s), and 62 exception(s).
[ View ]
#24 interdiff.txt1.58 KB2ndmile
#22 1915026-22.patch2.94 KB2ndmile
FAILED: [[SimpleTest]]: [MySQL] 54,062 pass(es), 340 fail(s), and 63 exception(s).
[ View ]
#22 interdiff.txt1.63 KB2ndmile
#15 1915026-interdiff-12-15.txt1.33 KBsteveoliver
#15 1915026-15.patch2.9 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 53,412 pass(es).
[ View ]
#12 1915026-12.patch2.86 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,269 pass(es).
[ View ]
#10 1915026-10.patch2.89 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,218 pass(es).
[ View ]
#5 drupal-twig-views-ui-container--1915026-5.patch2.09 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 50,573 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#1 drupal-twig-views-ui-container--1915026-1.patch2.45 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 50,554 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Comments

Title:Convert theme_views_ui_container to Twig.Convert theme_views_ui_container to Twig
Project:ARCHIVE: Drupal 8 Twig Sandbox» 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
StatusFileSize
new2.45 KB
FAILED: [[SimpleTest]]: [MySQL] 50,554 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

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

Issue tags:+Twig

Why you steal tags, bot?

Status:Needs review» Needs work
StatusFileSize
new2.09 KB
FAILED: [[SimpleTest]]: [MySQL] 50,573 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 53,218 pass(es).
[ View ]

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

:(

Assigned:steveoliver» Unassigned
StatusFileSize
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 53,269 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

You can configure that :)

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

StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,412 pass(es).
[ View ]
new1.33 KB

Status:Reviewed & tested by the community» Needs review

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.

Issue tags:+Needs manual testing

Tagging.

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.

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!

Assigned:Unassigned» 2ndmile

Status:Needs work» Needs review
StatusFileSize
new1.63 KB
new2.94 KB
FAILED: [[SimpleTest]]: [MySQL] 54,062 pass(es), 340 fail(s), and 63 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new3.12 KB
FAILED: [[SimpleTest]]: [MySQL] 54,123 pass(es), 332 fail(s), and 62 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
new3.1 KB
FAILED: [[SimpleTest]]: [MySQL] 54,014 pass(es), 340 fail(s), and 63 exception(s).
[ View ]

Trying again.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new3.09 KB
PASSED: [[SimpleTest]]: [MySQL] 55,235 pass(es).
[ View ]
new1.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.

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

Assigned:Unassigned» echeese
Status:Needs work» Needs review
StatusFileSize
new3.04 KB
FAILED: [[SimpleTest]]: [MySQL] 55,482 pass(es), 47 fail(s), and 625 exception(s).
[ View ]

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.

Assigned:echeese» Unassigned
Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new315 bytes
new3.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,797 pass(es).
[ View ]

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

StatusFileSize
new3.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,792 pass(es).
[ View ]

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.

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

Issue tags:+Novice, +Needs reroll

Needs a re-roll before profiling.

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,322 pass(es).
[ View ]

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.

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

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

Issue tags:-Needs reroll
StatusFileSize
new2.21 KB
new2.71 KB
PASSED: [[SimpleTest]]: [MySQL] 56,121 pass(es).
[ View ]

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

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

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"

Status:Needs work» Needs review
Issue tags:-Needs manual testing
StatusFileSize
new1.15 KB
new2.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,134 pass(es).
[ View ]

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>

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.

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.

Issue tags:+Novice

tagging

StatusFileSize
new2.71 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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!

Status:Needs work» Reviewed & tested by the community

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

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,269 pass(es).
[ View ]

This one should work

dibs!

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

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.

Status:Reviewed & tested by the community» Needs work

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

Assigned:Unassigned» rvilar
Status:Needs work» Needs review
StatusFileSize
new2.72 KB
FAILED: [[SimpleTest]]: [MySQL] 56,747 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new630 bytes
new2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 57,122 pass(es).
[ View ]

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.

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!

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.

Issue summary:View changes

Updated issue summary.