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.
Follow up for #1971384: [META] Convert page callbacks to controllers
Convert admin/structure/taxonomy/%taxonomy_vocabulary to new routing system
Related & Follow up
#2044435: Convert pager.inc to a service
#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller
#1976298: Move taxonomy_get_tree() and associated functions to Taxonomy storage, deprecate procedural wrappers.
#1974210: Convert admin/structure/forum to a Controller
#2049121: Regression: Moving out term children on term listing page is broken
Comment | File | Size | Author |
---|---|---|---|
#103 | drupal8.taxonomy-module.1974492-102.patch | 34.7 KB | disasm |
#103 | interdiff.txt | 527 bytes | disasm |
#102 | drupal8.taxonomy-module.1974492-102.patch | 34.7 KB | disasm |
#102 | interdiff.patch | 6.45 KB | disasm |
#100 | drupal8.taxonomy-module.1974492-100.patch | 34.39 KB | disasm |
Comments
Comment #1
larowlanPostponed on #1947432: Add a generic EntityAccessCheck to replace entity_page_access()
Comment #2
andypostRelated #1938932: Remove taxonomy_overview_terms() and taxonomy_overview_vocabularies() theme functions in favour of table #type and list controller depends on it
Comment #3
larowlanFirst patch includes #1947432: Add a generic EntityAccessCheck to replace entity_page_access()
Second patch (do-not-test) is just the changes here.
Follow-ups for todos found :
Comment #4
larowlanComment #6
andypostAlso related #1891690: Use EntityListController for vocabularies
Comment #7
larowlanAn interdiff.
A patch including #1947432: Add a generic EntityAccessCheck to replace entity_page_access()
And the patch with just this issues changes (the do-not-test).
Comment #8
andypostAny reason shortcut included in patch?
Comment #10
larowlanLooks like I did the patch a wrong
Comment #11
larowlanRe-roll against HEAD now that #1947432: Add a generic EntityAccessCheck to replace entity_page_access() is in
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedi think this slipped from somewhere else
should be {inheritdoc}
lets use $this->request here
Since we are there, lets fix this spacing
moduleHandler
Comment #13
andypostSuppose better to convert path to remove add/edit limit #1978112: Convert taxonomy admin path to follow other core entity patterns
Comment #15
larowlanin light of #13
Comment #16
larowlanPath change is in
Comment #17
larowlanSo this passed on everything Taxonomy/Forum related locally except #1953800: Make the database connection serializable when you click the 'reset to alphabetical' - postponing on that (after bot run).
Follow-ups/todos as per #3.
Comment #19
larowlanComment #20
larowlan#1953800: Make the database connection serializable is in
Comment #21
larowlan#17: taxonomy-overview.1974492.17.patch queued for re-testing.
Comment #23
andypost/manage was lost
Comment #24
nick_schuch CreditAttribution: nick_schuch commentedFix as per #23
Comment #25
andypostLooks rtbc!
not sure it make sense to have empty menu link
Comment #26
andypostwhy no use ->label()
Comment #27
larowlanWe need the menu link for breadcrumbs?
Comment #29
andypostComment #30
andypostWorking on #1976296-4: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller found that table sort currently broken
@larowlan Just do not use properties directly when there's a method for
Comment #31
larowlanComment #32
disasm CreditAttribution: disasm commentedreroll attached.
Comment #34
maggo CreditAttribution: maggo commentedReviewing last patch
Comment #35
PanchoRetitled in line with the other issues, so it is more easily found.
Comment #36
PanchoAlso retesting to see if it still applies.
#32: drupal-convert_taxonomy_overview-1974492-32.patch queued for re-testing.
Comment #38
maggo CreditAttribution: maggo commentedComment #39
disasm CreditAttribution: disasm commentedreroll!
Comment #41
tim-e CreditAttribution: tim-e commentedReroll of #39 fix bug
Comment #42
tim-e CreditAttribution: tim-e commentedFix tags
Comment #43
tim-e CreditAttribution: tim-e commentedComment #45
tim-e CreditAttribution: tim-e commentedReroll and fix breadcrumbs
Comment #47
larowlanI think we missed the changes from #2020841: Order of terms broken after 'Reset to alphabetical' and 'Save Order'
Comment #49
larowlanWe're so close to removing forum.admin.inc, this is the last blocker.
Comment #51
larowlanComment #52
jibranwhy not just
'#href' => $uri['path'],
Comment #53
larowlangreen!
Comment #54
jibranI don't think parent method has third param.
Should be
VocabularyInterface
This should be the part of
TermListController
but we don't have one in Trem.php. I think we should create TermListController?Comment #55
tim.plunkettWe don't want a term list controller, what we really want is a View. But we don't have draggable views in core... So maybe we do need a list controller :(
Comment #56
larowlanThis is a wscci conversion, is term list controller in scope here? And it blocks a forum conversion (the last one).
Agree on the VocabularyInterface and the {@inheritdoc} comment.
Comment #57
larowlanReroll against head and fixes issues identified at #54 except the list controller, which I think is out of scope as we don't yet have generic table-drag support.
Comment #59
jibrannice ;)
#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller is almost ready. Can we combine the patch?
Comment #61
jibranhehe
Comment #62
dawehnerWow taxonomy is way over my head.
Why not use the $page variable defined above?
Comment #63
jibranSame here :).
Fixed #62.
Comment #64
klausiWhy is the function taxonomy_overview_terms() not removed from taxonomy.admin.inc?
BTW: #2049121: Regression: Moving out term children on term listing page is broken, I guess that is not somehow fixed by this issue?
Comment #65
jibranGood catch @klausi. Thanks for the review.
taxonomy_overview_terms()
.taxonomy_overview_terms_submit()
.OverviewTerms::buildForm()
andOverviewTerms::submitForm()
TranslationManager
.Comment #66
xjmOverall this is looking really great; every single thing I thought of while reviewing turned out to be already addressed in a followup and so I found only nitpicks:
Merge artifact?
Wow. Dependencies. We've got 'em!
Need a docblock here.
Global alert. Do we have an existing issue for this? It appears that only this form and Views use these globals in core.
This needs to be rewrapped with the extra indentation from being inside a class.
All minor cleanups that a novice could do.
Let's collect all the followups and related issues and put them in a list in the summary, and additionally just note the API changes in a brief list. We don't need more than that because the meta covers it.
After that, I'd like to see some really thorough manual testing for this patch, with devel generate. This form has been really unstable historically, so let's test a bunch of scenarios, and also document thoroughly what was tested. Suggestions:
I realize this might seem redundant given that it's mostly just moved code, but the existing test coverage for this is not as robust as one might hope, and that wall of logic in the form builder is terrifying. We can also use scenarios that we test as the basis for followups to expand the test coverage if needed.
NW for the minor cleanups.
Comment #67
larowlan#2044435: Convert pager.inc to a service is for the pager logic, which we postponed as 'too late', if you could chime in there @xjm we could restart it.
Comment #68
xjmReading the related issues... I'm not sure #2009680: Replace theme() with drupal_render() in taxonomy module fixed whatever it was expected to fix. It'd be actually good for someone to do the testing I describe above on just HEAD and file issues so we can determine what's out of scope here.
Comment #69
xjmOh, and for this issue, let's just stick an @todo in there for #67.
Comment #69.0
xjmadded https://drupal.org/node/2044435
Comment #70
jibranFixed #66 added issues to summary.
Comment #71
xjmThe novice task is also resolved.
Comment #72
jibran#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller is committed. This needs re-roll.
Comment #73
RainbowArrayRe-rolled the patch. This is of my first re-rolls, so if I botched this, my apologies.
Comment #75
RainbowArrayTrying another re-roll.
Comment #77
disasm CreditAttribution: disasm commented#75: taxonomy-overview-terms.patch queued for re-testing.
Comment #78
star-szrThanks for working on this @mdrummond! I think the reroll needs to be revisited…
This shouldn't be added back, it was removed in #1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller.
Comment #79
robeano CreditAttribution: robeano commentedI rerolled the last patch and removed taxonomy_vocabulary_confirm_reset_alphabetical_submit(). This is ready for another review.
Comment #80
star-szrThanks for working on this @robeano! I think this is missing some deletions. It might be easier to start again with rerolling the patch from #70.
Patch from #70 - 5 files changed, 552 insertions, 384 deletions.
Patch from #79: 5 files changed, 553 insertions, 117 deletions.
Comment #81
robeano CreditAttribution: robeano commentedThanks @Cottser! I'm on it.
Comment #82
robeano CreditAttribution: robeano commentedAlright, one more time here. This looks a lot closer to the patch at comment #70. Ready for review.
Comment #84
jibran#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller is committed.
Comment #85
puregin CreditAttribution: puregin commented#82: taxonomy-overview-terms-1974492-82.patch queued for re-testing.
Comment #87
wamilton CreditAttribution: wamilton commentedDidn't need a re-roll, applied cleanly.
Removed calls to the removed confirmation form callback and the control flow blocks around said. One less assertion fails for that.
Comment #88
disasm CreditAttribution: disasm commentedComment #89
Gaelan CreditAttribution: Gaelan commentedI think we lost the part of the patch that removed the old form.
Comment #90
pfrenssenSetting to needs work as per #89.
Comment #91
effulgentsia CreditAttribution: effulgentsia commentedThis is just a straight reroll of #82 with the interdiff in #87 applied on top. I have not reviewed it at all.
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedComment #93
jibranWhy is this not the part of new submit function?
Comment #94
disasm CreditAttribution: disasm commentedThat's a different issue: #1987866: Convert taxonomy_vocabulary_add() to a new style controller
Comment #95
effulgentsia CreditAttribution: effulgentsia commentedI'm hoping others here can answer #93 and drive this issue to RTBC. Again, all I did in #91 was reroll :)
[Edit: this was an xpost with #94]
Comment #97
tim.plunkettIt was missing the submit for the reset button.
Also, updated to FormBase.
This honestly should be a list controller, but the paging is way too complex for that... :(
Comment #98
jibranI have manually tested the patch it is working fine. It also fixes the fails in #91 checked locally. It also addresses #93. Its also accommodates changes after #2059245: Add a FormBase class containing useful methods.
@xjm said in #66
It is RTBC for me but I worked on this patch let's wait for @xjm.
Comment #99
tim.plunkettThis needs a reroll for FormBase
Comment #100
disasm CreditAttribution: disasm commentedreroll. It already is using FormBase though, so no changes in that. Basically just removed taxonomy.admin.inc, and merged taxonomy.routing.yml
Comment #102
disasm CreditAttribution: disasm commentedNote sure if it's the full cause of the failures, but there's a menu item pointing at taxonomy.admin.inc which doesn't exist. removing file attribute on it. That will at least kill a few exceptions. Also, looks like there may be a type hinting issue.
Comment #103
disasm CreditAttribution: disasm commenteduploaded wrong interdiff file.
Comment #104
jibranBack to @xjm for review.
Comment #105
dawehnerI could not find anything, it passes, so RTBC.
Comment #106
webchickAwesome work, folks!
Committed and pushed to 8.x. Thanks!
Comment #107.0
(not verified) CreditAttribution: commentedAdded related issues