Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Issue tags: -Needs manual testing

Actually, this one probably doesn't need manual testing because this is only used in automated tests.

star-szr’s picture

This already has a preprocess function:
http://api.drupal.org/api/drupal/core%21modules%21views%21tests%21views_...

The theme function is a one-liner:
http://api.drupal.org/api/drupal/core%21modules%21views%21tests%21views_...

In theory we just need to create a dead simple .html.twig template and remove the theme function, the only thing is I can't see where the theme function is defined. I found this, but not sure if we need to change it to pick up a template file - not familiar with the plugin annotation yet.

/**
 * Provides a test plugin for the mapping style.
 *
 * @ingroup views_style_plugins
 *
 * @Plugin(
 *   id = "mapping_test",
 *   title = @Translation("Field mapping"),
 *   help = @Translation("Maps specific fields to specific purposes."),
 *   theme = "views_view_mapping_test",
 *   display_types = {"normal", "test"}
 * )
 */
class MappingTest extends Mapping {
star-szr’s picture

Okay, @tim.plunkett was able to clear this up for me and explain how this works. Thanks Tim!

So the theme implementation is defined in the style plugin annotation (code in comment #3).

Then views_theme() adds hook_theme() definitions for all style plugins:

  $plugins = views_get_plugin_definitions();

  // Register theme functions for all style plugins
  foreach ($plugins as $type => $info) {
    foreach ($info as $plugin => $def) {
      
      ...

      if (!function_exists('theme_' . $def['theme'])) {
        $hooks[$def['theme']]['template'] = drupal_clean_css_identifier($def['theme']);
      }
    }
  }

The last part checks for a template file based on the theme definition name (if there is no theme function), in this case views-view-mapping-test.html.twig.

So this should be a very straightforward conversion. Remove the theme function and add a simple template - done :)

widukind’s picture

Assigned: Unassigned » widukind
widukind’s picture

Status: Active » Needs review
FileSize
1.79 KB

Patch attached.

The variable description in the code docs for the template_preprocess function and the twig template are in part guesswork, any suggestions for more accurate descriptions are much appreciated.

Thanks!

widukind’s picture

FileSize
1.79 KB
610 bytes

sloppiness in the previous patch, missed to close the docblock properly in the template.
fixed in this updated patch.

star-szr’s picture

This is looking very good. Thank you, @widukind! :)

+++ b/core/modules/views/templates/views-view-mapping-test.html.twigundefined
@@ -0,0 +1,14 @@
+ * @see theme_preprocess()

@see template_preprocess()

widukind’s picture

FileSize
1.79 KB
705 bytes

Thanks @Cottser, fixed the code comment.

thedavidmeister’s picture

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

One nitpick, should be RTBC after this.

+ * - element: The view content.

+ *   - rows: A list of view rows.
+ *   - options: Various view options, including the row style mapping.
+ *   - view: The executed view.

Capital V for View.

widukind’s picture

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

capitalized View in code docs.

thedavidmeister’s picture

Assigned: widukind » thedavidmeister

The docs look good to me. I'm assigning to myself as I'm going to apply the patch locally and test it out.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

@Cottser, thanks for the explanation in #3 and #4 as to how this even works, I was thoroughly confused for a bit >.<

For anyone who is wondering *which* tests this is relevant to (since there's quite a lot of Views tests), look in /core/modules/views/lib/Drupal/views/Tests/Plugin/StyleMappingTest.php

They're under "Views Plugins" in SimpleTest.

The template renders what ends up in the $output variable twice in the testMappedOutput() method.

Original markup - output 1:

<div  class="view view-test-style-mapping view-id-test_style_mapping view-display-id-default view-dom-id-5ab87e1757584f698f9328971e779a69bf96de6a288019add1d906349c839b2a">
        
  
  
      <div class="view-content">
      <div class="views-row-mapping-test"><div class="title_field-name">title_field:John</div><div class="name_field-name">name_field:John</div><div class="numeric_field-age">numeric_field:25</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:George</div><div class="name_field-name">name_field:George</div><div class="numeric_field-age">numeric_field:27</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Ringo</div><div class="name_field-name">name_field:Ringo</div><div class="numeric_field-age">numeric_field:28</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Paul</div><div class="name_field-name">name_field:Paul</div><div class="numeric_field-age">numeric_field:26</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Meredith</div><div class="name_field-name">name_field:Meredith</div><div class="numeric_field-age">numeric_field:30</div></div>    </div>
  
  
  
  
  
  
</div>

Original markup - output 2:

<div  class="view view-test-style-mapping view-id-test_style_mapping view-display-id-default view-dom-id-01cf4cab1f96317cd66cccbe66cba077f2d3f49fb6aa3b0c502149bc6952283f">
        
  
  
      <div class="view-content">
      <div class="views-row-mapping-test"><div class="title_field-name">title_field:John</div><div class="name_field-job">name_field:Singer</div><div class="numeric_field-age">numeric_field:25</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:George</div><div class="name_field-job">name_field:Singer</div><div class="numeric_field-age">numeric_field:27</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Ringo</div><div class="name_field-job">name_field:Drummer</div><div class="numeric_field-age">numeric_field:28</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Paul</div><div class="name_field-job">name_field:Songwriter</div><div class="numeric_field-age">numeric_field:26</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Meredith</div><div class="name_field-job">name_field:Speaker</div><div class="numeric_field-age">numeric_field:30</div></div>    </div>
  
  
  
  
  
  
</div>

New markup - output 1:

<div  class="view view-test-style-mapping view-id-test_style_mapping view-display-id-default view-dom-id-ac19368f0267233ca2282f07102dcd1e76ca395f4f726e706ecd6e34641b578d">
        
  
  
      <div class="view-content">
      <div class="views-row-mapping-test"><div class="title_field-name">title_field:John</div><div class="name_field-name">name_field:John</div><div class="numeric_field-age">numeric_field:25</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:George</div><div class="name_field-name">name_field:George</div><div class="numeric_field-age">numeric_field:27</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Ringo</div><div class="name_field-name">name_field:Ringo</div><div class="numeric_field-age">numeric_field:28</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Paul</div><div class="name_field-name">name_field:Paul</div><div class="numeric_field-age">numeric_field:26</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Meredith</div><div class="name_field-name">name_field:Meredith</div><div class="numeric_field-age">numeric_field:30</div></div>
    </div>
  
  
  
  
  
  
</div>

New markup - output 2:

<div  class="view view-test-style-mapping view-id-test_style_mapping view-display-id-default view-dom-id-c63400b092a73dd49209d0fd23c3d71caf7f0e96248c866eb62daab15c4eead6">
        
  
  
      <div class="view-content">
      <div class="views-row-mapping-test"><div class="title_field-name">title_field:John</div><div class="name_field-job">name_field:Singer</div><div class="numeric_field-age">numeric_field:25</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:George</div><div class="name_field-job">name_field:Singer</div><div class="numeric_field-age">numeric_field:27</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Ringo</div><div class="name_field-job">name_field:Drummer</div><div class="numeric_field-age">numeric_field:28</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Paul</div><div class="name_field-job">name_field:Songwriter</div><div class="numeric_field-age">numeric_field:26</div></div><div class="views-row-mapping-test"><div class="title_field-name">title_field:Meredith</div><div class="name_field-job">name_field:Speaker</div><div class="numeric_field-age">numeric_field:30</div></div>
    </div>
  
  
  
  
  
  
</div>
thedavidmeister’s picture

Assigned: thedavidmeister » Unassigned
dawehner’s picture

+++ b/core/modules/views/tests/views_test_data/views_test_data.moduleundefined
@@ -75,7 +75,15 @@ function views_test_data_preprocess_views_view_table(&$variables) {
+ *   - view: The executed View.

This line somehow differs from other patches, but I think we can clean this up as follow ups.

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs work

#15 - as in, it should say "The View object"?

widukind’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
897 bytes

"The View object" it is.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community
Fabianx’s picture

Issue tags: -Novice, -Twig, -VDC

#17: 1963764-17.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Twig, +VDC

The last submitted patch, 1963764-17.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Tagging for reroll.

widukind’s picture

Assigned: Unassigned » widukind
widukind’s picture

Status: Needs work » Needs review
FileSize
1.73 KB

rerolled patch from 1963764-17

star-szr’s picture

Issue tags: -Novice, -Needs reroll

Thanks @widukind!

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC from #18

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

The views test modules have moved :)

curl https://drupal.org/files/1963764-23.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1771  100  1771    0     0   1562      0  0:00:01  0:00:01 --:--:--  1914
error: core/modules/views/tests/views_test_data/views_test_data.module: does not exist in index
pwieck’s picture

Status: Needs work » Needs review
FileSize
1.76 KB

rerolled.

Status: Needs review » Needs work

The last submitted patch, 1963764-27.patch, failed testing.

pwieck’s picture

Got to kick #27 back to the coders. I'm just a reroll monkey. Seems like an odd place to fail.

One-time link is no longer valid. Other UserPasswordResetTest.php 81
Drupal\user\Tests\UserPasswordResetTest->testUserPasswordReset()

pwieck’s picture

Status: Needs work » Needs review

#27: 1963764-27.patch queued for re-testing.

pwieck’s picture

Ready for review! The test bots have been flaky today .

pwieck’s picture

Issue tags: -Needs reroll

Removing tag -Needs Reroll

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Seems fine, back to RTBC.

star-szr’s picture

Assigned: widukind » Unassigned
Status: Reviewed & tested by the community » Needs work

We could lowercase 'V' in 'View' and 'Views' throughout the patch though :)

pwieck’s picture

Assigned: Unassigned » pwieck

Fixing patch to reflex #34

pwieck’s picture

FileSize
1.76 KB

Fixed docs with capital V's to lowercase in View and Views.

pwieck’s picture

Status: Needs work » Needs review

Forgot status change

star-szr’s picture

Changes look good, thanks @pwieck. I will come back to this tonight and re-RTBC unless someone beats me to it.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Okay, I'm happy :)

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/templates/views-view-mapping-test.html.twigundefined
@@ -0,0 +1,15 @@
+ * @see template_preprocess()

Let's remove this line per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.

pwieck’s picture

Fixing this

pwieck’s picture

Made new policy change as per #40

pwieck’s picture

Assigned: pwieck » Unassigned

#42 passed and ready for review

dawehner’s picture

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

Back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e294988 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Add commit message to summary