Replace theme() with drupal_render() in taxonomy module

attached

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

ups, small mistake in #2

new attached

+ CodeSprintUA

simple replacement
looks good for me
RTBC if bot green

attached new one

EDIT tests are passes locally, seems bot troubles

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

RTBC

+++ 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) : '',

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

then + (empty($indetion))

because of loop

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

Patch attached.. I hope this is correct.

Lets try this again.. added missing '('

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

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

forgot to set to 'needs review', grrrrr

trying again

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

hope this patch is what's expected.

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.

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.

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

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

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.

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.

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

With this patch

Ok, I'll try!

Seems duplicate #2020841: Order of terms broken after 'Reset to alphabetical' and 'Save Order'
Also related #1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller depends on sorting

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

Thanks for the hint!
Patch + Test attached!

Awesome work, removing tags :)

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?

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

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

Dibs.

Fixed code style issues.

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

I'll give it a go.

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

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

Fixed #56.

This is looking good to me now.

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?

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

Edit: and the class definition needs a docblock :)

I'm working on it

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.

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/

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

Ops fixed extra full stop in class comment block :)

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

Ok, singularized the three sentences :D

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"?

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

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.

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

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

Looks good to me :)

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

Committed/pushed to 8.x, thanks!

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.

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.

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!