Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().

To test this code

  1. Visit the taxonomy administration page at (admin/structure/taxonomy)
  2. add a few terms to the tags vocabulary
  3. re-order the tags so one is indented (by dragging one under and to the right of another)
  4. confirm that the indentation shows as expected
CommentFileSizeAuthor
#78 replace-theme-with-render-2009680-78.patch760 bytesEric_A
#76 replace-theme-with-render-2009680-76.patch657 bytesEric_A
#72 replace-theme-with-render-test-only-2009680-72.patch2.02 KBDarthDrupal
#72 replace-theme-with-render-2009680-72.patch2.98 KBDarthDrupal
#72 interdiff-2009680-69-72.txt950 bytesDarthDrupal
#71 taxonomy-drupal-render-tests-only-71.patch2.02 KBhussainweb
#71 taxonomy-drupal-render-71.patch2.98 KBhussainweb
#69 replace-theme-with-render-2009680-69.patch2.98 KBDarthDrupal
#67 replace-theme-with-render-2009680-67.patch2.99 KBDarthDrupal
#65 replace-theme-with-render-2009680-65.patch2.99 KBDarthDrupal
#64 replace-theme-with-render-2009680-64.patch2.99 KBDarthDrupal
#62 replace-theme-with-render-with-empty-func-2009680-62.patch3.02 KBDarthDrupal
#62 replace-theme-with-render-with-isset-func-2009680-62.patch2.99 KBDarthDrupal
#57 replace-theme-with-render-2009680-57.patch2.75 KBDarthDrupal
#55 replace-theme-with-render-2009680-55.patch2.75 KBDarthDrupal
#50 twig-2009680-45.patch2.87 KBadamcowboy
#46 replace-theme-with-render-2009680-46.patch2.73 KBDarthDrupal
#43 replace-theme-with-render-2009680-43.patch2.63 KBDarthDrupal
#42 replace-theme-with-render-2009680-42.patch2.58 KBDarthDrupal
#39 replace-theme-with-render-2009680-39.patch2.92 KBDarthDrupal
#35 TaxonomyTermsIndentation.test1.6 KBDarthDrupal
#32 taxonomy_depth_broken.png12.32 KBalexpott
#32 taxonomy_depth_fixed-3.png12.32 KBalexpott
#28 replace-theme-with-render-2009680-28.patch987 bytesDarthDrupal
#27 replace-theme-with-render-2009680-27.patch957 bytesDarthDrupal
#24 replace-theme-with-render-2009680-24.patch978 bytesclayball
#22 replace-theme-with-render-2009680-21.patch977 bytesclayball
#21 replace-theme-with-render-2009680-21.patch977 bytesclayball
#19 replace-theme-with-render-2009680-19.patch974 bytesclayball
#17 replace-theme-with-render-2009680-17.patch974 bytesclayball
#15 replace-theme-with-render-2009680-15.patch973 bytesclayball
#7 replace-theme-with-render-2009680-7.patch976 bytesSamvel
#2 replace-theme-with-render-2009680-2.patch908 bytesSamvel
#3 replace-theme-with-render-2009680-3.patch909 bytesSamvel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Samvel’s picture

Assigned: Unassigned » Samvel
Samvel’s picture

attached

p.s. Don't use this one, see #3

Samvel’s picture

Status: Active » Needs review
FileSize
909 bytes

ups, small mistake in #2

new attached

Samvel’s picture

Issue tags: +CodeSprintUA

+ CodeSprintUA

podarok’s picture

Status: Needs review » Reviewed & tested by the community

simple replacement
looks good for me
RTBC if bot green

Status: Reviewed & tested by the community » Needs work

The last submitted patch, replace-theme-with-render-2009680-3.patch, failed testing.

Samvel’s picture

Status: Needs work » Needs review
FileSize
976 bytes

attached new one

Status: Needs review » Needs work
Issue tags: -Novice, -CodeSprintUA

The last submitted patch, replace-theme-with-render-2009680-7.patch, failed testing.

andypost’s picture

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

EDIT tests are passes locally, seems bot troubles

#7: replace-theme-with-render-2009680-7.patch queued for re-testing.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

RTBC

andypost’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -241,8 +241,12 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
+    $indentation =  array('#theme' => 'indentation');
+    if (isset($term->depth->value)) {
+      $indentation['#size'] = $term->depth->value;
+    }

should be

if (isset($term->depth->value) && $term->depth->value > 0) {
  $indentation = array(
    '#theme' => 'indentation',
    '#size' = $term->depth->value,
  );
}
+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -241,8 +241,12 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
-      '#prefix' => isset($term->depth->value) && $term->depth->value > 0 ? theme('indentation', array('size' => $term->depth->value)) : '',
+      '#prefix' => isset($term->depth->value) && $term->depth->value > 0 ? drupal_render($indentation) : '',
       '#type' => 'link',
       '#title' => $term->label(),
       '#href' => "taxonomy/term/" . $term->id(),

then simply

'#prefix' => isset($indentation) ? drupal_render($indentation) : '',
andypost’s picture

$indention = array();
if () {
  // as #11
}

then + (empty($indetion))

because of loop

thedavidmeister’s picture

Assigned: Samvel » Unassigned

this has been sitting idle for a while, feel free to re-assign.

clayball’s picture

Assigned: Unassigned » clayball

I'll take this.. getting my feet wet here. This seems straight forward enough. New patch forthcoming.

clayball’s picture

Status: Needs work » Needs review
FileSize
973 bytes

Patch attached.. I hope this is correct.

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-15.patch, failed testing.

clayball’s picture

Status: Needs work » Needs review
FileSize
974 bytes

Lets try this again.. added missing '('

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-17.patch, failed testing.

clayball’s picture

Status: Needs work » Needs review
FileSize
974 bytes

I'll get this to pass if it kills me!

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-19.patch, failed testing.

clayball’s picture

This is getting a little.. silly.. sorry..

forgot to set to 'needs review', grrrrr

clayball’s picture

Status: Needs work » Needs review
FileSize
977 bytes

trying again

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-21.patch, failed testing.

clayball’s picture

Status: Needs work » Needs review
FileSize
978 bytes

this version looks good on my end... again, sorry for all the posts.

hope this patch is what's expected.

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-24.patch, failed testing.

clayball’s picture

Assigned: clayball » Unassigned

someone else can look at this.. not sure what the fail message:

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

has anything to do with my changes.

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
957 bytes

I'm trying to making some debug and I noticed that $term->depth already has the size of the indentation and $term->depth->value obviously gives this error:

Trying to get property of non-object in taxonomy_overview_terms()

Do you think that $term->depth will become an object in the next releases?
I fixed it for the moment. Patch attached.

DarthDrupal’s picture

Ops sorry I didn't notice indications at #12.
Re-attached patch.

fran seva’s picture

sbudker1’s picture

Status: Needs review » Reviewed & tested by the community

Tested the patch and it seemed to work! I added a few terms to the tags vocabulary and every thing seemed normal. After that I re-order the tags so one is indented and confirmed that the indentation showed up as expected.

sbudker1’s picture

Tested the patch and it seemed to work! I added a few terms to the tags vocabulary and every thing seemed normal. After that I re-order the tags so one is indented and confirmed that the indentation showed up as expected.

alexpott’s picture

Category: task » bug
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
FileSize
12.32 KB
12.32 KB

This is actually fixing a bug... therefore I think we need to add a test for this...

To test...

  1. Create a vocabulary
  2. Create some terms
  3. Drag one of the terms to be "parented" another term
  4. Press save

Current result

taxonomy_depth_broken.png

With this patch

taxonomy_depth_fixed-3.png

DarthDrupal’s picture

Assigned: Unassigned » DarthDrupal

Ok, I'll try!

andypost’s picture

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
1.6 KB

I wrote the test! It's the first time I write a functional test so sorry if I made mistakes :)
I read the other tests in the taxonomy module folder (core/modules/taxonomy/lib/Drupal/taxonomy/Tests) to have some directions about the right sintax.
I hope I did it in the right way.

In the test I:

- create a vocabulary
- create three terms
- create the $edit array to simulate that the second term has been dragged under the first
- call the drupalPost method passing in the $edit array and clicking the submit button with label 'Save'
- call an assertion to check if in the raw html the indentation div element is present (

 

)

In my local drupal installation the test passed correctly.
Let me know if I did something wrong so I can improve my "test approach" :D

thedavidmeister’s picture

Status: Needs review » Needs work

@DarthDrupal, you need to roll the tests into the patch rather than provide them in a separate file, otherwise the testbots can't run them.

DarthDrupal’s picture

@thedavidmeister I looked for some documentation about that but I didn't find anything.
Can you give me some hint on how to roll the patch and the test together?

Sorry but it's the first time for me :)

thedavidmeister’s picture

Apply the patch in #28, add your tests, stage all of your changes in git together with the changes from the patch.

Now if you do $ git diff --cached you should see the patch and your changes in the diff.

$ git diff --cached > 2009680-39.patch will write it into a patch file you can upload here.

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
2.92 KB

Thanks for the hint!
Patch + Test attached!

jenlampton’s picture

Issue tags: -Needs tests, -CodeSprintUA

Awesome work, removing tags :)

thedavidmeister’s picture

Status: Needs review » Needs work
+      //'terms[tid:1:0][term][tid]' => 1,
+      //'terms[tid:1:0][term][parent]' => '',
+      //'terms[tid:1:0][term][depth]' => 0,
+      //'terms[tid:1:0][weight]' => 0,
+      'terms[tid:' . $term2->id() . ':0][term][tid]' => 2,
+      'terms[tid:' . $term2->id() . ':0][term][parent]' => 1,
+      'terms[tid:' . $term2->id() . ':0][term][depth]' => 1,
+      'terms[tid:' . $term2->id() . ':0][weight]' => 1,
+      //'terms[tid:3:0][term][tid]' => 3,
+      //'terms[tid:3:0][term][parent]' => 2,
+      //'terms[tid:3:0][term][depth]' => 2,
+      //'terms[tid:3:0][weight]' => 2,

Make sure to remove your debug code when you put up a patch for review.

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
2.58 KB

Ops sorry, comments removed!

Btw I'm not sure the test ran in #39...
If I click on View Details I don't see my test listed in the Drupal\taxonomy\Tests\ section..
Is that normal?

DarthDrupal’s picture

Test class name typing error in #42.
Fixed and attached again

tsphethean’s picture

Status: Needs review » Needs work

Minor code style tweaks needed:

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentation.phpundefined
@@ -0,0 +1,38 @@
+  }
+  public function setUp() {

blank line needed between methods

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentation.phpundefined
@@ -0,0 +1,38 @@
+    $this->vocabulary = $this->createVocabulary();
+  }
+  function testTermIndentation() {

blank line needed between methods, and method description also needed

adamcowboy’s picture

Dibs.

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
2.73 KB

Fixed code style issues.

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

The last submitted patch, replace-theme-with-render-2009680-46.patch, failed testing.

DarthDrupal’s picture

Status: Needs work » Needs review

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

The last submitted patch, replace-theme-with-render-2009680-46.patch, failed testing.

adamcowboy’s picture

FileSize
2.87 KB

I'll give it a go.

adamcowboy’s picture

Status: Needs work » Needs review
azinoman’s picture

Assigned: DarthDrupal » azinoman
azinoman’s picture

Good job Adam, this code is as smooth as a baby's bottom

azinoman’s picture

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

Assigned: azinoman » DarthDrupal
Status: Reviewed & tested by the community » Needs review
FileSize
2.75 KB

testTermIndentation() comment fixed and removed changes to default.settings.php file.

thedavidmeister’s picture

Status: Needs review » Needs work

+ * Creating and posting three terms.

trailing whitespace.

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
2.75 KB

Fixed #56.

tsphethean’s picture

Status: Needs review » Reviewed & tested by the community

This is looking good to me now.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

This is looking very good overall, here are some nitpicks :)

  1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentation.phpundefined
    @@ -0,0 +1,44 @@
    +<?php
    +/**
    + * Testing functionality of terms indentation in
    + * admin/structure/taxonomy/manage/tags
    + */
    +namespace Drupal\taxonomy\Tests;
    

    Needs an @file docblock per http://drupal.org/node/1354#file and there should be a blank line before and after the docblock.

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentation.phpundefined
    @@ -0,0 +1,44 @@
    +class TaxonomyTermsIndentation extends TaxonomyTestBase {
    +  public static $modules = array('taxonomy');
    

    Needs an empty line between the class definition and the $modules property definition.

    The $modules property needs a docblock now as well, see other tests for an example.

  3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentation.phpundefined
    @@ -0,0 +1,44 @@
    +  /**
    +   * Creating and posting three terms.
    +   * Then checking to see if a div with class indentation exists.
    +   */
    +  function testTermIndentation() {
    

    Please see http://drupal.org/node/1354#functions, this should probably have a summary line like "Tests term indentation." Then it could have another paragraph below with the rest of the text, or you could use inline comments to describe the different steps of the test.

  4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentation.phpundefined
    @@ -0,0 +1,44 @@
    +  }
    +}
    

    There should be a blank line between the last function and the end of the class.

  5. +++ b/core/modules/taxonomy/taxonomy.admin.incundefined
    @@ -159,8 +159,15 @@ function taxonomy_overview_terms($form, &$form_state, Vocabulary $vocabulary) {
    +    $indentation = array();
    ...
    +      '#prefix' => !empty($indentation) ? drupal_render($indentation) : '',
    

    If we check !empty($indentation) before rendering, why do we set $indentation to an empty array?

star-szr’s picture

…and the test class name should end with Test, i.e. TaxonomyTermsIndentationTest.

Edit: and the class definition needs a docblock :)

DarthDrupal’s picture

I'm working on it

DarthDrupal’s picture

Ok, added all the missing docblocks and blank lines.

Regarding the fifth point in #59 I followed andypost indications at #12.

However I think that could be enough using the isset() function instead of empty() here

+ '#prefix' => !empty($indentation) ? drupal_render($indentation) : '',

I attach two different version of the same patch, one using empty() and the $indentation array initialization and the other with just the isset() function instead of empty() so you can decide what is better.

star-szr’s picture

Status: Needs review » Needs work

Thanks @DarthDrupal!

I missed the loop :) that makes sense then, I would go with the first patch.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentationTest.phpundefined
@@ -0,0 +1,61 @@
+ * Definition of Drupal\taxonomy\Tests\TaxonomyTermsIndentationTest.

This should be Contains \Drupal… per https://drupal.org/node/1354#file.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentationTest.phpundefined
@@ -0,0 +1,61 @@
+/**
+ * Testing functionality of terms indentation in
+ * admin/structure/taxonomy/manage/tags.
+ */
+class TaxonomyTermsIndentationTest extends TaxonomyTestBase {

This is a summary line so should fit on one line of 80 characters or less per http://drupal.org/node/1354#drupal.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentationTest.phpundefined
@@ -0,0 +1,61 @@
+    // Submit the edited form and check for html indentation element presence.

s/html/HTML/

DarthDrupal’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

Ok let's go further with the original patch (I missed the loop too :) ).
Fixed as indicated in #63.

DarthDrupal’s picture

Ops fixed extra full stop in class comment block :)

star-szr’s picture

Looking much much better, thanks for sticking with this @DarthDrupal!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentationTest.phpundefined
@@ -0,0 +1,60 @@
+ * Testing terms indentation functionality in terms list page.
...
+      'description' => 'Ensure that the terms indentation works properly',
...
+   * Test terms indentation.

"term indentation functionality", "term list page", and "terms indentation" - all should be singular I think.

And I think that's the end of my nitpicks on this issue :)

DarthDrupal’s picture

Ok, singularized the three sentences :D

star-szr’s picture

I keep on forgetting to mention but interdiffs are great, really helpful especially in cases like this :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermsIndentationTest.phpundefined
@@ -0,0 +1,60 @@
+ * Testing term indentation functionality in terms list page.
...
+      'name' => 'Taxonomy Terms Indentation',

Missed a couple here. Also... test names are not usually Title Cased like that. Maybe just "Taxonomy term indentation"?

DarthDrupal’s picture

I agree, fixed test name.
I singularized the filename and the class name too.

PS: thanks for all your hints it's my first patch and my first test so every counsel is good for me :D

star-szr’s picture

Status: Needs review » Needs work

Keep up the great work @DarthDrupal!

I brought this down locally and ran the test without the fix and it failed as expected. Generally we post test-only patches on the issue though, see the bottom of https://drupal.org/contributor-tasks/write-tests for an example. So I would recommend posting a test-only and combined patch on this issue, generally that is done when the test is added to the issue or when changes are made to the test. We want to make sure that the test works properly and would expose any regressions in the future.

…and I said I was done but I found two more tiny things to update. Then I will happily RTBC the patch :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.phpundefined
@@ -0,0 +1,60 @@
+      'description' => 'Ensure that the term indentation works properly',

'description' should end in a period per https://drupal.org/node/325974.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTermIndentationTest.phpundefined
@@ -0,0 +1,60 @@
+   * Test term indentation.

Should be 'Tests' per http://drupal.org/node/1354#functions.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
2.02 KB

I apologize for jumping in, but I really wanted to try this out too. BTW, I have made changes as per #70.

DarthDrupal’s picture

Patch in #71 seems good.
Just added the issue number in the patch file name and the interdiff file (thanks to Cottser for teaching me ;) ).

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me :)

star-szr’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Eric_A’s picture

Status: Fixed » Needs review
FileSize
657 bytes

Sorry, but this reverted typed data conversions for no real reason. It works now but won't soon, so it needs to be fixed anyways and the fix is trivial.

The revert was done in #27 as a fix, but the real fix was done later when the new local variable $indentation was initialized every loop. From then on the term depth objects again were only accessed when set.

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-76.patch, failed testing.

Eric_A’s picture

Status: Needs work » Needs review
FileSize
760 bytes

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009680-78.patch, failed testing.

Eric_A’s picture

Status: Needs work » Fixed

Sorry for the noise, you guys did a very nice coding job. The only thing is that the issue summary (and the issue itself) is all about the conversion and very little about the bug fix.

DarthDrupal’s picture

Yes Eric_A maybe we didn't focus on the bug enough, sorry :)

however I wanna thank all of you guys who taught me how to approach with patch submission and writing functional tests.

Thank you all!

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

Anonymous’s picture

Issue summary: View changes

to test