Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Testing issue #242324: Going to page 2 on "list terms" page doesn't display additional terms, I found no pager on the taxonomy terms overview page.
Steps to Reproduce
- Install D8 using the Standard install profile (so that taxonomy is enabled and a vocabulary is created for us).
- Create many taxonomy terms
- Downloading the devel module and using devel_generate is recommended
- Default "terms per page" setting is 100, so either generate more than 100 terms OR adjust the terms_per_page_admin (to set via settings.php, use
$config['taxonomy.settings']['terms_per_page_admin'] = 5;
Beta phase evaluation
Issue category | Bug because the pager for taxonomy term lists doesn't show up. |
---|---|
Issue priority | Major, because this makes it nearly impossible to administer some taxonomies through the UI. |
Unfrozen changes | None. |
Prioritized changes | Prioritized, because the main goal of this issue is to fix a bug. |
Disruption | None. |
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff-39.txt | 611 bytes | lucaslg |
#41 | no_pager_shows_on-2425535-41.patch | 2.64 KB | lucaslg |
#39 | interdiff-31.txt | 1.31 KB | lucaslg |
#39 | no_pager_shows_on-2425535-39.patch | 2.64 KB | lucaslg |
#31 | interdiff.txt | 2.82 KB | tim.plunkett |
Comments
Comment #1
lucaschain CreditAttribution: lucaschain commentedThis bug is being caused because the $form['pager_pager'] value is not being set. Also, there was a bug that displayed the same term in two pages, this patch fixes that too
Comment #2
lucaschain CreditAttribution: lucaschain commentedComment #3
lucaschain CreditAttribution: lucaschain commentedComment #4
balagan CreditAttribution: balagan commentedI confirm that applying this patch the pager shows up properly, and there is no extra term displayed regarding the terms_per_page_admin setting.
Comment #5
balagan CreditAttribution: balagan commentedWell, as I see, the array_pop() conflicts with chx's patch here https://www.drupal.org/node/242324#comment-9618045
Applying both patches, only 2 terms show up with the setting terms_per_page_admin: 3
I think we should omit this array_pop(), since hopefully all the logic behind paging will be changed.
Comment #6
balagan CreditAttribution: balagan commentedComment #7
balagan CreditAttribution: balagan commentedComment #8
balagan CreditAttribution: balagan commentedI have removed the comments and the array_pop() function.
Comment #9
balagan CreditAttribution: balagan commentedComment #11
roderikOK - I agree that it is better to not add the array_pop() function. But that means this issue is effectively postponed on #242324: Going to page 2 on "list terms" page doesn't display additional terms, right?
Officially marking as such, for clarity.
If anyone sees the linked issue solved, please unpostpone and review this. I'm not removing the "needs manual testing" tag, but most (all?) of the manual testing has been done and screenshots posted.
Comment #12
balagan CreditAttribution: balagan commented@roderik, I think the Going to page 2 on "list terms" page doesn't display additional terms issue should not postpone this issue. This patch simply displays the pager, whatever the logic is behind it. I don't see any problem for reviewing this issue. Let's just display that pager, no matter what it shows, so I am reopening this.
Comment #13
balagan CreditAttribution: balagan commentedComment #14
roderik> This patch simply displays the pager, whatever the logic is behind it.
Fair enough :)
Comment #15
akalata CreditAttribution: akalata commentedI updated the issue summary with testing steps, but in my manual testing the patch did not cause the pager to appear.
Comment #16
balagan CreditAttribution: balagan commented@akalata, indeed, it does not solve the issue anymore.
I had the following notice:
Notice: Undefined index: #element in template_preprocess_pager() (line 176 of ...\drupal8\core\includes\pager.inc).
Same notice for #parameters (line 177), #quantity (line 178), and an unnamed index (line 182).
The referred code in pager.inc is:
It seems another issue is holding back this patch.
Comment #17
balagan CreditAttribution: balagan commentedI have created an issue for the missing indices. https://www.drupal.org/node/2472361
Comment #18
balagan CreditAttribution: balagan commentedComment #19
mondrakeHave you tried using #type => pager instead of #theme => pager
see https://www.drupal.org/node/2446539
Comment #20
nicoloye CreditAttribution: nicoloye at Actency commented@mondrake, I confirm that your solution do works for me. Good catch.
Here is balagan patch with your suggestion.
Comment #21
balagan CreditAttribution: balagan commentedIt works indeed, and then the other issue with indices is gone. Gonna review it properly though.
Comment #22
balagan CreditAttribution: balagan commentedI confirm, the patch applies properly, and the pager shows up.
Anyone knows about the changes that made it necessary to use #type instead of #theme? 2 months ago using #theme was OK.
Comment #23
nicoloye CreditAttribution: nicoloye at Actency commentedI think it comes from this : https://www.drupal.org/node/2446539
It seems it's some cache content related issue.
Comment #24
balagan CreditAttribution: balagan commented@nicoloye: Thanks, indeed!
Comment #25
roderikGreat work.
But fact that the pager is missing, but we do not get any notification of this, means that... we need a test for this.
Please see https://www.drupal.org/contributor-tasks/write-tests if necessary.
It looks like core/modules/taxonomy/src/Tests/TaxonomyTermIndentationTest is a good example to use as a basis for this.
Comment #26
balagan CreditAttribution: balagan commentedSince now only writing tests are needed, removing Novice tag
Comment #27
akalata CreditAttribution: akalata commentedI think I'll try to tackle this after lunch!
Comment #28
lucaslg CreditAttribution: lucaslg commentedI wrote a test which :
- Create a vocabulary
- Set term_per_page_admin to 3
- Create 6 terms
- Go to the overview page
- Check the pager is visible
- Go to page 2
- Check the pager is visible
I didn't look at the content of the page (if terms are placed on the right place) because it is not coherent as long as #242324: Going to page 2 on "list terms" page doesn't display additional terms is not fixed.
I wonder if a test is needed to check the absence of a pager when there is only one page of result.
Comment #30
jhedstromConfirmed that the patch in #28resolves the issue. Bumping status to major and removing the 'needs tests' tag since there are now tests.
Comment #31
tim.plunkettCleaned up the test a bit, there was no need to load it after creating it.
Otherwise, looks great.
Because I'm only tweaking, still marking RTBC.
Comment #32
roderik@lucaslg:
> I wonder if a test is needed to check the absence of a pager when there is only one page of result.
I think that's a good idea and it seems easy to do inside your existing test (tim.plunkett-cleaned version) with just a few lines addition.
I'm not going to change RTBC status for that but please go ahead and post a new version of the patch (and set Needs Review) if you want to :)
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThis patch looks great already, but let's do @lucaslg proposal as mentioned in #32.
Nitpick, but this wrapping is not correct and the sentence should end in a dot.
Comment #34
lucaslg CreditAttribution: lucaslg commentedOk, i'll have a look tomorrow.
Comment #35
lucaslg CreditAttribution: lucaslg commentedHere we go, it is now checking that there is no pager when there is only one page displayed.
@pjonckiere : Excuse me but I don't understand the wrapping problem. What should I change ? The two lines are both less than 80 characters.
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous at XIO commented@lucaslg: "Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over", so the word 'than' should move up a line (see the standards for comments).
I just tested the patch manually and it works as expected. The tests look great too. If you can just fix the comment, I'll RTBC if the patch comes back green.
Fwiw: while testing, I encountered the bug you mention in #28, but I also think this is out of scope for this ticket, since it will be dealt with in #242324: Going to page 2 on "list terms" page doesn't display additional terms.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAdded a beta evaluation to the issue summary.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedTypo and minor clarification.
Comment #39
lucaslg CreditAttribution: lucaslg commentedThanks for the explanation pjonckiere. I stopped at the first coding standard page which does not mention it.
The comment is fixed.
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedYeah, comments guidelines are separate. The guidelines as a whole are quite the read :)
The comment is fixed, but it seems like you introduced a trailing space (the red mark in dreditor, see below). I think that could be fixed on commit.
Thanks for sticking with the issue. This is a very nice first patch!
Comment #41
lucaslg CreditAttribution: lucaslg commentedah, trailing spaces, I already had troubles with them during the sprint. My IDE is now set correctly to get rid of them.
It should be all good now.
Thank you all for your help and guidelines.
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedOk, brilliant. I didn't want to put it in "Needs work" for a single space.
Still looks good!
Comment #43
balagan CreditAttribution: balagan commentedUnassigning, I guess you forgot to do it.
Comment #44
xjmNice find. Agreed that this is major since it pretty much prevents large vocabularies from being administered in that UI. Thanks for the beta evaluation and the test-only patch in #28.
Technically this is only supposed to be a single line of 80 characters or fewer. I reworded it slightly on commit to fix that:
Reference: https://www.drupal.org/node/1354#drupal (in the bulleted list).
This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x (with credit for the reviews as well). Thanks everyone!
As a followup, we should check whether there are any duplicates of this issue that can be closed.
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedI looked through the taxonomy issue queue, but couldn't find any duplicates.