Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase
taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() converted to formInterface
Last reference to confirm_form() needs deeper refactoring in #1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller
Follow-up issues
#1988712: Move taxonomy_check_vocabulary_hierarchy() into storage controller
#2004978: Clean-up resetCache() for taxonomy
Comment | File | Size | Author |
---|---|---|---|
#68 | interdiff.txt | 861 bytes | andypost |
#68 | 1946456-tax-delete-68.patch | 17.26 KB | andypost |
#66 | interdiff.txt | 824 bytes | andypost |
#66 | 1946456-tax-delete-66.patch | 17.27 KB | andypost |
#65 | interdiff.txt | 1.88 KB | andypost |
Comments
Comment #1
andypostInitial patch.
taxonomy_vocabulary_confirm_reset_alphabetical() is not converted
Module routes for vocabularies should change after #1891690: Use EntityListController for vocabularies
Comment #3
andypostShould be better
Probably this one should be postponed on #1947432: Add a generic EntityAccessCheck to replace entity_page_access()
Comment #4
andypostVocabulary should work. not sure about language element injection so testing
Comment #6
Crell CreditAttribution: Crell commentedThis should be $taxonomyTerm. Also, "the feed"? Copy/paste error?
$vocabulary, or $taxonomyVocabuarly.
Also, I think the two new access checkers here are redundant with #1947432: Add a generic EntityAccessCheck to replace entity_page_access().
Comment #7
tim.plunkettComment #8
andypostI think better to file change notice but convert form names
taxonomy_term_confirm_delete - taxonomy_term_delete_form
taxonomy_vocabulary_confirm_delete - taxonomy_vocabulary_delete_form
Comment #9
tim.plunkettWhy? We haven't done that anywhere else. Why create more work.
Comment #10
andypostok, so revert names
Comment #11
tim.plunkettThis looks great! RTBC if green.
Comment #12
alexpottThis conflicts with #1818560: Convert taxonomy entities to the new Entity Field API which is now rtbc and considerably bigger... so lets postpone this one on that.
Comment #13
alexpott#1818560: Convert taxonomy entities to the new Entity Field API has landed... time for a reroll...
Comment #14
andypostJust a re-roll, confirm forms already written to use proper id() label() bundle() methods
Comment #15
andypostScope limited, last confirm_form() has own issue #1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller
Comment #16
tim.plunkettThis should type hint with the interface now.
We should inject the entity manager.
Comment #17
andypostFiled #1980982: Clean-up procedural wrappers for taxonomy
Probably
Cache::invalidateTags()
could be injected too but not sure howComment #19
andypostInterface everywhere
Comment #21
andypostAnd last one
Comment #22
dawehnerJust some small nitpicks :)
It might be cleaner to just store the storage controller and not the full entity manager.
Should have a "\" at the front.
Comment #23
andypostI think better to have entity-manager injected while this operates on term & vocabulary both
Comment #24
dawehnerThis will at least cause problems with #1909418: Allow Entity Controllers to have their dependencies injected
Comment #25
andypostre-roll
Comment #26
dawehnerWhat about just storing the storage controller instead of the full entity manager?
Comment #27
andypostFiled #1988712: Move taxonomy_check_vocabulary_hierarchy() into storage controller
So having EM injected is temporary solution - reorder should happen in term storage controller on post delete (same as image style does)
So here's patch with updated link in @todo
Comment #27.0
andypostUpdated issue summary.
Comment #29
andypost#27: 1946456-tax-delete-27.patch queued for re-testing.
EDIT the broken test passes locally
Comment #30
aspilicious CreditAttribution: aspilicious commented#27: 1946456-tax-delete-27.patch queued for re-testing.
Comment #31
andypostAddressed #26 and changed to interfaces
Comment #32
andypostbetter to unify it
Comment #33
andypostSo make both menu items similar to easy fix in #1834002: Configuration delete operations are all over the place
EDIT same used for
admin/structure/block/manage/%block/delete
Comment #34
aspilicious CreditAttribution: aspilicious commentedLets do this
Comment #35
alexpottWe need a follow up to move this to
TermStorageController::resetCache()
This is unnecessary - it'll happen
VocabularyStorageController::postDelete()
Comment #35.0
alexpottfollow-ups
Comment #36
andypostFiled follow-up #2004978: Clean-up resetCache() for taxonomy and added to summary
Added @todo and fix #35-2
Comment #37
Crell CreditAttribution: Crell commentedBack to Alex.
Comment #38
amateescu CreditAttribution: amateescu commentedPlease see my comment in #2004978-1: Clean-up resetCache() for taxonomy. Basically, the interdiff from #36 should be reverted (a.k.a. the correct/committable patch is in #33).
The fact that
VocabularyStorageController::postDelete()
currently invalidates persistent caches is a bug for which we need to open a new issue.Setting to NW for reuploading/rerolling if needed the patch from #33.
Comment #39
andypostRe-roll of #33 and RTBC because no changes
#923826: $entity_delete() should use transactions points why
Comment #40
amateescu CreditAttribution: amateescu commentedYou mean reroll of #33, right?
Comment #41
andypostYes, it was the #33 just against current HEAD
Comment #42
catch#39: 1946456-tax-delete-39.patch queued for re-testing.
Comment #44
andypostComment #46
andypost#44: 1946456-tax-delete-44.patch queued for re-testing.
Comment #48
andypostFix ControllerInterface been moved
Comment #49
Crell CreditAttribution: Crell commentedComment #50
alexpottNeeds rework in light of #2011018: Reconcile entity forms and confirm forms
Comment #51
andypostMerge and small fix for comment about storage controller
Comment #52
tim.plunkettLet's use single quotes here, we have more with than without
Comment #53
andypostThey are strings so I leave them without quotes as contact.routing.yml
Suppose we could clean it latter
Comment #54
tim.plunkettJust stop making changes to existing code. Most of core uses single quotes in cases like this, please be consistent.
Comment #55
andypostSuppose #2011018: Reconcile entity forms and confirm forms needs follow-up because
hook_form_alter()
fires on entity forms (see screens) sopath_form_taxonomy_term_form_alter()
adds path widgetThis is a new code
Comment #56
andypostPatch on top of #2011018-37: Reconcile entity forms and confirm forms
Comment #57
ParisLiakos CreditAttribution: ParisLiakos commentedComment #58
disasm CreditAttribution: disasm commentedreroll attached.
Comment #59
disasm CreditAttribution: disasm commentedComment #61
andypostEntityControllerInterface
should pass moduleHandler to parent constructorAlso changed routing for consistency
Comment #62
tim.plunkettShould be VocabularyStorageControllerInterface
Otherwise RTBC
Comment #63
andypostMakes sense, missed introduction of the interface
Comment #64
dawehnerThis is a great patch.
It would be cool to name it something like vocabularyStorageController to make it clear that we deal with vocabs and not terms.
Maybe a newline somewhere in this long codeblock would help to read it better.
Comment #65
andypostDone
Comment #66
andypoststorage->load() now receive single ID
Comment #68
andypostreset() is not needed now
Comment #69
jibranTested on simplytest.me working fine. RTBC for me.
#2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu() is in so shall we fix it in here or in #2032587: Clear menu_router code out of function menu_local_tasks() when all actions and tabs are converted to plugins?
Comment #70
tim.plunkettHere please. That's for killing the code in menu.inc, not doing the conversions
Comment #71
dawehnerWe can't convert this here, as the parent items are not routes yet.
Comment #72
tim.plunkettAh, very true!
Comment #73
alexpottCommitted 3172ea4 and pushed to 8.x. Thanks!
t() is injectable now... but this should be injected into \Drupal\Core\Entity\EntityFormController and so it out of scope here...
Comment #74
alexpottCreated #2046511: EntityFormController needs the translation_service injected
Comment #75.0
(not verified) CreditAttribution: commentedUpdated issue summary.