Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
To test this code
- Visit the taxonomy administration page at (admin/structure/taxonomy)
- add a few terms to the tags vocabulary
- re-order the tags so one is indented (by dragging one under and to the right of another)
- confirm that the indentation shows as expected
Comment | File | Size | Author |
---|---|---|---|
#78 | replace-theme-with-render-2009680-78.patch | 760 bytes | Eric_A |
#76 | replace-theme-with-render-2009680-76.patch | 657 bytes | Eric_A |
#72 | replace-theme-with-render-test-only-2009680-72.patch | 2.02 KB | DarthDrupal |
#72 | replace-theme-with-render-2009680-72.patch | 2.98 KB | DarthDrupal |
#72 | interdiff-2009680-69-72.txt | 950 bytes | DarthDrupal |
Comments
Comment #1
Samvel CreditAttribution: Samvel commentedComment #2
Samvel CreditAttribution: Samvel commentedattached
p.s. Don't use this one, see #3
Comment #3
Samvel CreditAttribution: Samvel commentedups, small mistake in #2
new attached
Comment #4
Samvel CreditAttribution: Samvel commented+ CodeSprintUA
Comment #5
podaroksimple replacement
looks good for me
RTBC if bot green
Comment #7
Samvel CreditAttribution: Samvel commentedattached new one
Comment #9
andypostEDIT tests are passes locally, seems bot troubles
#7: replace-theme-with-render-2009680-7.patch queued for re-testing.
Comment #10
podarokRTBC
Comment #11
andypostshould be
then simply
Comment #12
andypostbecause of loop
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedthis has been sitting idle for a while, feel free to re-assign.
Comment #14
clayball CreditAttribution: clayball commentedI'll take this.. getting my feet wet here. This seems straight forward enough. New patch forthcoming.
Comment #15
clayball CreditAttribution: clayball commentedPatch attached.. I hope this is correct.
Comment #17
clayball CreditAttribution: clayball commentedLets try this again.. added missing '('
Comment #19
clayball CreditAttribution: clayball commentedI'll get this to pass if it kills me!
Comment #21
clayball CreditAttribution: clayball commentedThis is getting a little.. silly.. sorry..
forgot to set to 'needs review', grrrrr
Comment #22
clayball CreditAttribution: clayball commentedtrying again
Comment #24
clayball CreditAttribution: clayball commentedthis version looks good on my end... again, sorry for all the posts.
hope this patch is what's expected.
Comment #26
clayball CreditAttribution: clayball commentedsomeone 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.
Comment #27
DarthDrupal CreditAttribution: DarthDrupal commentedI'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.
Comment #28
DarthDrupal CreditAttribution: DarthDrupal commentedOps sorry I didn't notice indications at #12.
Re-attached patch.
Comment #29
fran seva CreditAttribution: fran seva commented#28: replace-theme-with-render-2009680-28.patch queued for re-testing.
Comment #30
sbudker1 CreditAttribution: sbudker1 commentedTested 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.
Comment #31
sbudker1 CreditAttribution: sbudker1 commentedTested 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.
Comment #32
alexpottThis is actually fixing a bug... therefore I think we need to add a test for this...
To test...
Current result
With this patch
Comment #33
DarthDrupal CreditAttribution: DarthDrupal commentedOk, I'll try!
Comment #34
andypostSeems 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
Comment #35
DarthDrupal CreditAttribution: DarthDrupal commentedI 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
Comment #36
thedavidmeister CreditAttribution: thedavidmeister commented@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.
Comment #37
DarthDrupal CreditAttribution: DarthDrupal commented@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 :)
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commentedApply 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.Comment #39
DarthDrupal CreditAttribution: DarthDrupal commentedThanks for the hint!
Patch + Test attached!
Comment #40
jenlamptonAwesome work, removing tags :)
Comment #41
thedavidmeister CreditAttribution: thedavidmeister commentedMake sure to remove your debug code when you put up a patch for review.
Comment #42
DarthDrupal CreditAttribution: DarthDrupal commentedOps 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?
Comment #43
DarthDrupal CreditAttribution: DarthDrupal commentedTest class name typing error in #42.
Fixed and attached again
Comment #44
tsphethean CreditAttribution: tsphethean commentedMinor code style tweaks needed:
blank line needed between methods
blank line needed between methods, and method description also needed
Comment #45
adamcowboy CreditAttribution: adamcowboy commentedDibs.
Comment #46
DarthDrupal CreditAttribution: DarthDrupal commentedFixed code style issues.
Comment #48
DarthDrupal CreditAttribution: DarthDrupal commented#46: replace-theme-with-render-2009680-46.patch queued for re-testing.
Comment #50
adamcowboy CreditAttribution: adamcowboy commentedI'll give it a go.
Comment #51
adamcowboy CreditAttribution: adamcowboy commentedComment #52
azinoman CreditAttribution: azinoman commentedComment #53
azinoman CreditAttribution: azinoman commentedGood job Adam, this code is as smooth as a baby's bottom
Comment #54
azinoman CreditAttribution: azinoman commentedComment #55
DarthDrupal CreditAttribution: DarthDrupal commentedtestTermIndentation() comment fixed and removed changes to default.settings.php file.
Comment #56
thedavidmeister CreditAttribution: thedavidmeister commented+ * Creating and posting three terms.
trailing whitespace.
Comment #57
DarthDrupal CreditAttribution: DarthDrupal commentedFixed #56.
Comment #58
tsphethean CreditAttribution: tsphethean commentedThis is looking good to me now.
Comment #59
star-szrThis is looking very good overall, here are some nitpicks :)
Needs an @file docblock per http://drupal.org/node/1354#file and there should be a blank line before and after the docblock.
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.
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.
There should be a blank line between the last function and the end of the class.
If we check !empty($indentation) before rendering, why do we set $indentation to an empty array?
Comment #60
star-szr…and the test class name should end with Test, i.e. TaxonomyTermsIndentationTest.
Edit: and the class definition needs a docblock :)
Comment #61
DarthDrupal CreditAttribution: DarthDrupal commentedI'm working on it
Comment #62
DarthDrupal CreditAttribution: DarthDrupal commentedOk, 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.
Comment #63
star-szrThanks @DarthDrupal!
I missed the loop :) that makes sense then, I would go with the first patch.
This should be Contains \Drupal… per https://drupal.org/node/1354#file.
This is a summary line so should fit on one line of 80 characters or less per http://drupal.org/node/1354#drupal.
s/html/HTML/
Comment #64
DarthDrupal CreditAttribution: DarthDrupal commentedOk let's go further with the original patch (I missed the loop too :) ).
Fixed as indicated in #63.
Comment #65
DarthDrupal CreditAttribution: DarthDrupal commentedOps fixed extra full stop in class comment block :)
Comment #66
star-szrLooking much much better, thanks for sticking with this @DarthDrupal!
"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 :)
Comment #67
DarthDrupal CreditAttribution: DarthDrupal commentedOk, singularized the three sentences :D
Comment #68
star-szrI keep on forgetting to mention but interdiffs are great, really helpful especially in cases like this :)
Missed a couple here. Also... test names are not usually Title Cased like that. Maybe just "Taxonomy term indentation"?
Comment #69
DarthDrupal CreditAttribution: DarthDrupal commentedI 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
Comment #70
star-szrKeep 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 :)
'description' should end in a period per https://drupal.org/node/325974.
Should be 'Tests' per http://drupal.org/node/1354#functions.
Comment #71
hussainwebI apologize for jumping in, but I really wanted to try this out too. BTW, I have made changes as per #70.
Comment #72
DarthDrupal CreditAttribution: DarthDrupal commentedPatch in #71 seems good.
Just added the issue number in the patch file name and the interdiff file (thanks to Cottser for teaching me ;) ).
Comment #73
star-szrLooks good to me :)
Comment #74
star-szr#72: replace-theme-with-render-2009680-72.patch queued for re-testing.
Comment #75
catchCommitted/pushed to 8.x, thanks!
Comment #76
Eric_A CreditAttribution: Eric_A commentedSorry, 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.
Comment #78
Eric_A CreditAttribution: Eric_A commentedComment #80
Eric_A CreditAttribution: Eric_A commentedSorry 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.
Comment #81
DarthDrupal CreditAttribution: DarthDrupal commentedYes 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!
Comment #82.0
(not verified) CreditAttribution: commentedto test