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

CommentFileSizeAuthor
#68 interdiff.txt861 bytesandypost
#68 1946456-tax-delete-68.patch17.26 KBandypost
#66 interdiff.txt824 bytesandypost
#66 1946456-tax-delete-66.patch17.27 KBandypost
#65 interdiff.txt1.88 KBandypost
#65 1946456-tax-delete-65.patch17.28 KBandypost
#63 interdiff.txt1.25 KBandypost
#63 1946456-tax-delete-63.patch17.24 KBandypost
#61 interdiff.txt2.37 KBandypost
#61 1946456-tax-delete-61.patch17.24 KBandypost
#58 drupal-tax_delete-1946456-58.patch17.06 KBdisasm
#56 1946456-tax-delete-56.patch18.83 KBandypost
#56 interdiff.txt3.2 KBandypost
#55 term_delete.png10.63 KBandypost
#55 term_delete2.png3.88 KBandypost
#51 interdiff.txt9.46 KBandypost
#51 1946456-tax-delete-51.patch17 KBandypost
#48 interdiff.txt639 bytesandypost
#48 1946456-tax-delete-48.patch16.51 KBandypost
#44 1946456-tax-delete-44.patch16.5 KBandypost
#39 1946456-tax-delete-39.patch16.26 KBandypost
#36 interdiff.txt1.5 KBandypost
#36 1946456-tax-delete-36.patch16.28 KBandypost
#33 interdiff.txt548 bytesandypost
#33 1946456-tax-delete-33.patch16.26 KBandypost
#31 interdiff.txt4.77 KBandypost
#31 1946456-tax-delete-31.patch16.23 KBandypost
#27 interdiff.txt1.03 KBandypost
#27 1946456-tax-delete-27.patch14.97 KBandypost
#25 1946456-tax-delete-24.patch14.97 KBandypost
#23 interdiff.txt1.66 KBandypost
#23 1946456-tax-delete-23.patch15.26 KBandypost
#21 interdiff.txt1.2 KBandypost
#21 1946456-tax-delete-21.patch15.25 KBandypost
#19 interdiff.txt1.36 KBandypost
#19 1946456-tax-delete-19.patch15.22 KBandypost
#17 interdiff.txt3.71 KBandypost
#17 1946456-tax-delete-17.patch14.06 KBandypost
#14 1946456-tax-14.patch13.22 KBandypost
#10 interdiff.txt1.07 KBandypost
#10 1946456-tax-10.patch13.21 KBandypost
#8 1946456-tax-8.patch13.21 KBandypost
#4 interdiff.txt4.3 KBandypost
#4 1946456-tax-4.patch18.43 KBandypost
#3 interdiff.txt816 bytesandypost
#3 1946456-tax-3.patch14.13 KBandypost
#1 1946456-tax-1.patch14.13 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
14.13 KB

Initial patch.
taxonomy_vocabulary_confirm_reset_alphabetical() is not converted
Module routes for vocabularies should change after #1891690: Use EntityListController for vocabularies

Status: Needs review » Needs work

The last submitted patch, 1946456-tax-1.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
14.13 KB
816 bytes

Should be better

Probably this one should be postponed on #1947432: Add a generic EntityAccessCheck to replace entity_page_access()

andypost’s picture

FileSize
18.43 KB
4.3 KB

Vocabulary should work. not sure about language element injection so testing

Status: Needs review » Needs work

The last submitted patch, 1946456-tax-4.patch, failed testing.

Crell’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDelete.php
@@ -0,0 +1,81 @@
+  /**
+   * The feed the items being deleted belong to.
+   *
+   * @var \Drupal\taxonomy\Plugin\Core\Entity\Term
+   */
+  protected $taxonomy_term;

This should be $taxonomyTerm. Also, "the feed"? Copy/paste error?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyDelete.php
@@ -0,0 +1,80 @@
+  /**
+   * The vocabulary being deleted.
+   *
+   * @var \Drupal\taxonomy\Plugin\Core\Entity\Vocabulary
+   */
+  protected $taxonomy_vocabulary;

$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().

tim.plunkett’s picture

Status: Needs work » Postponed
andypost’s picture

Status: Postponed » Needs review
FileSize
13.21 KB

I 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

tim.plunkett’s picture

Why? We haven't done that anywhere else. Why create more work.

andypost’s picture

FileSize
13.21 KB
1.07 KB

ok, so revert names

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This looks great! RTBC if green.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed

This 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.

alexpott’s picture

Status: Postponed » Needs work
andypost’s picture

Status: Needs work » Needs review
FileSize
13.22 KB

Just a re-roll, confirm forms already written to use proper id() label() bundle() methods

andypost’s picture

Title: Convert all confirm_form() in taxonomy.admin.inc to the new form interface » Convert taxonomy_term_confirm_delete() and taxonomy_vocabulary_confirm_delete() to the new form interface
tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDelete.phpundefined
@@ -0,0 +1,81 @@
+use Drupal\taxonomy\Plugin\Core\Entity\Term;
...
+  public function buildForm(array $form, array &$form_state, Term $taxonomy_term = NULL) {

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyDelete.phpundefined
@@ -0,0 +1,80 @@
+use Drupal\taxonomy\Plugin\Core\Entity\Vocabulary;
...
+  public function buildForm(array $form, array &$form_state, Vocabulary $taxonomy_vocabulary = NULL) {

This should type hint with the interface now.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDelete.phpundefined
@@ -0,0 +1,81 @@
+    taxonomy_check_vocabulary_hierarchy(entity_load('taxonomy_vocabulary', $this->term->bundle()), array('tid' => $this->term->id()));

We should inject the entity manager.

andypost’s picture

Status: Needs work » Needs review
FileSize
14.06 KB
3.71 KB

Filed #1980982: Clean-up procedural wrappers for taxonomy
Probably Cache::invalidateTags() could be injected too but not sure how

Status: Needs review » Needs work

The last submitted patch, 1946456-tax-delete-17.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
15.22 KB
1.36 KB

Interface everywhere

Status: Needs review » Needs work

The last submitted patch, 1946456-tax-delete-19.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
15.25 KB
1.2 KB

And last one

dawehner’s picture

Just some small nitpicks :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDelete.phpundefined
@@ -0,0 +1,112 @@
+    $vocabulary = $this->entityManager->getStorageController('taxonomy_vocabulary')->load(array($this->term->bundle()));

It might be cleaner to just store the storage controller and not the full entity manager.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -428,7 +431,7 @@ function taxonomy_vocabulary_delete_multiple(array $vids) {
+ * @param Drupal\taxonomy\VocabularyInterface $vocabulary

Should have a "\" at the front.

andypost’s picture

FileSize
15.26 KB
1.66 KB

I think better to have entity-manager injected while this operates on term & vocabulary both

dawehner’s picture

I think better to have entity-manager injected while this operates on term & vocabulary both

This will at least cause problems with #1909418: Allow Entity Controllers to have their dependencies injected

andypost’s picture

FileSize
14.97 KB

re-roll

dawehner’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDelete.phpundefined
@@ -0,0 +1,112 @@
+    $vocabulary = $this->entityManager->getStorageController('taxonomy_vocabulary')->load(array($this->term->bundle()));

What about just storing the storage controller instead of the full entity manager?

andypost’s picture

FileSize
14.97 KB
1.03 KB

Filed #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

andypost’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion

The last submitted patch, 1946456-tax-delete-27.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#27: 1946456-tax-delete-27.patch queued for re-testing.

EDIT the broken test passes locally

aspilicious’s picture

#27: 1946456-tax-delete-27.patch queued for re-testing.

andypost’s picture

FileSize
16.23 KB
4.77 KB

Addressed #26 and changed to interfaces

andypost’s picture

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -279,14 +280,10 @@ function taxonomy_menu() {
     'type' => MENU_LOCAL_TASK,
     'context' => MENU_CONTEXT_INLINE,

@@ -330,6 +327,12 @@ function taxonomy_menu() {
+    'type' => MENU_CONTEXT_INLINE,

better to unify it

andypost’s picture

FileSize
16.26 KB
548 bytes

So 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

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Lets do this

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDelete.phpundefined
@@ -0,0 +1,115 @@
+    Cache::invalidateTags(array('content' => TRUE));

We need a follow up to move this to TermStorageController::resetCache()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyDelete.phpundefined
@@ -0,0 +1,83 @@
+    Cache::invalidateTags(array('content' => TRUE));

This is unnecessary - it'll happen VocabularyStorageController::postDelete()

alexpott’s picture

Issue summary: View changes

follow-ups

andypost’s picture

Status: Needs work » Needs review
FileSize
16.28 KB
1.5 KB

Filed follow-up #2004978: Clean-up resetCache() for taxonomy and added to summary
Added @todo and fix #35-2

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Back to Alex.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

Please 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.

andypost’s picture

FileSize
16.26 KB

Re-roll of #33 and RTBC because no changes
#923826: $entity_delete() should use transactions points why

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

You mean reroll of #33, right?

andypost’s picture

Yes, it was the #33 just against current HEAD

catch’s picture

#39: 1946456-tax-delete-39.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +FormInterface, +WSCCI-conversion

The last submitted patch, 1946456-tax-delete-39.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
16.5 KB

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion

The last submitted patch, 1946456-tax-delete-44.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#44: 1946456-tax-delete-44.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +FormInterface, +WSCCI-conversion

The last submitted patch, 1946456-tax-delete-44.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
16.51 KB
639 bytes

Fix ControllerInterface been moved

Crell’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
andypost’s picture

Status: Needs work » Needs review
FileSize
17 KB
9.46 KB

Merge and small fix for comment about storage controller

tim.plunkett’s picture

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -1,16 +1,16 @@
+    _entity_form: taxonomy_term.delete
   requirements:
-    _entity_access: 'taxonomy_term.delete'
+    _entity_access: taxonomy_term.delete
...
-    _form: '\Drupal\taxonomy\Form\VocabularyDelete'
+    _entity_form: taxonomy_vocabulary.delete
   requirements:
-    _entity_access: 'taxonomy_vocabulary.delete'
+    _entity_access: taxonomy_vocabulary.delete

Let's use single quotes here, we have more with than without

andypost’s picture

They are strings so I leave them without quotes as contact.routing.yml

Suppose we could clean it latter

tim.plunkett’s picture

Status: Needs review » Needs work

Just stop making changes to existing code. Most of core uses single quotes in cases like this, please be consistent.

andypost’s picture

FileSize
3.88 KB
10.63 KB

Suppose #2011018: Reconcile entity forms and confirm forms needs follow-up because hook_form_alter() fires on entity forms (see screens) so path_form_taxonomy_term_form_alter() adds path widget

term_delete.png

term_delete2.png

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -1,6 +1,21 @@
+taxonomy_term_delete:
+  pattern: '/taxonomy/term/{taxonomy_term}/delete'
+  defaults:
+    _entity_form: taxonomy_term.delete
+  requirements:
+    _entity_access: taxonomy_term.delete
+
+taxonomy_vocabulary_delete:
+  pattern: '/admin/structure/taxonomy/manage/{taxonomy_vocabulary}/delete'
+  defaults:
+    _entity_form: taxonomy_vocabulary.delete
+  requirements:
+    _entity_access: taxonomy_vocabulary.delete

This is a new code

andypost’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
18.83 KB
ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
disasm’s picture

reroll attached.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-tax_delete-1946456-58.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
17.24 KB
2.37 KB

EntityControllerInterface should pass moduleHandler to parent constructor
Also changed routing for consistency

tim.plunkett’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDeleteForm.phpundefined
@@ -0,0 +1,99 @@
+  public function __construct(ModuleHandlerInterface $module_handler, EntityStorageControllerInterface $storage_controller) {

Should be VocabularyStorageControllerInterface

Otherwise RTBC

andypost’s picture

FileSize
17.24 KB
1.25 KB

Makes sense, missed introduction of the interface

dawehner’s picture

This is a great patch.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDeleteForm.phpundefined
@@ -0,0 +1,99 @@
+  protected $storageController;
...
+    $this->storageController = $storage_controller;

It would be cool to name it something like vocabularyStorageController to make it clear that we deal with vocabs and not terms.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDeleteForm.phpundefined
@@ -0,0 +1,99 @@
+    $this->entity->delete();
+    $vocabulary = $this->storageController->load(array($this->entity->bundle()));
+    // @todo Move to storage controller http://drupal.org/node/1988712
+    taxonomy_check_vocabulary_hierarchy(reset($vocabulary), array('tid' => $this->entity->id()));
+    drupal_set_message(t('Deleted term %name.', array('%name' => $this->entity->label())));
+    watchdog('taxonomy', 'Deleted term %name.', array('%name' => $this->entity->label()), WATCHDOG_NOTICE);
+    $form_state['redirect'] = 'admin/structure/taxonomy';
+    Cache::invalidateTags(array('content' => TRUE));

Maybe a newline somewhere in this long codeblock would help to read it better.

andypost’s picture

FileSize
17.28 KB
1.88 KB

Done

andypost’s picture

FileSize
17.27 KB
824 bytes

storage->load() now receive single ID

Status: Needs review » Needs work

The last submitted patch, 1946456-tax-delete-66.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
17.26 KB
861 bytes

reset() is not needed now

jibran’s picture

Issue tags: -Needs reroll

Tested on simplytest.me working fine. RTBC for me.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -275,14 +276,10 @@ function taxonomy_menu() {
     'type' => MENU_LOCAL_TASK,

@@ -326,6 +323,13 @@ function taxonomy_menu() {
+    'type' => MENU_LOCAL_TASK,

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

tim.plunkett’s picture

Here please. That's for killing the code in menu.inc, not doing the conversions

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

We can't convert this here, as the parent items are not routes yet.

tim.plunkett’s picture

Ah, very true!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3172ea4 and pushed to 8.x. Thanks!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/TermDeleteForm.phpundefined
@@ -0,0 +1,101 @@
+    return t('Are you sure you want to delete the term %title?', array('%title' => $this->entity->label()));
...
+    return t('Deleting a term will delete all its children if there are any. This action cannot be undone.');
...
+    return t('Delete');
...
+    drupal_set_message(t('Deleted term %name.', array('%name' => $this->entity->label())));

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyDeleteForm.phpundefined
@@ -0,0 +1,64 @@
+    return t('Are you sure you want to delete the vocabulary %title?', array('%title' => $this->entity->label()));
...
+    return t('Deleting a vocabulary will delete all the terms in it. This action cannot be undone.');
...
+    return t('Delete');
...
+    drupal_set_message(t('Deleted vocabulary %name.', array('%name' => $this->entity->label())));

t() is injectable now... but this should be injected into \Drupal\Core\Entity\EntityFormController and so it out of scope here...

alexpott’s picture

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.