Files: 
CommentFileSizeAuthor
#103 drupal8.taxonomy-module.1974492-102.patch34.7 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,280 pass(es).
[ View ]
#103 interdiff.txt527 bytesdisasm
#102 drupal8.taxonomy-module.1974492-102.patch34.7 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#102 interdiff.patch6.45 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#100 drupal8.taxonomy-module.1974492-100.patch34.39 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,480 pass(es), 46 fail(s), and 32 exception(s).
[ View ]
#97 taxo-1974492-97.patch34.38 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,992 pass(es).
[ View ]
#97 interdiff.txt8.12 KBtim.plunkett
#91 taxonomy-overview-terms-1974492-91.patch35.93 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 58,204 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#87 1974492-87.patch18.85 KBwamilton
PASSED: [[SimpleTest]]: [MySQL] 58,083 pass(es).
[ View ]
#87 interdiff.txt2.06 KBwamilton
#82 taxonomy-overview-terms-1974492-82.patch37.03 KBrobeano
FAILED: [[SimpleTest]]: [MySQL] 58,113 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#79 taxonomy-overview-terms-1974492-79.patch27.98 KBrobeano
FAILED: [[SimpleTest]]: [MySQL] 58,178 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#75 taxonomy-overview-terms.patch38.53 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 58,068 pass(es).
[ View ]
#73 taxonomy-overview-terms.patch38.61 KBmdrummond
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.admin.inc.
[ View ]
#70 1974492-70.patch37.59 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,146 pass(es).
[ View ]
#70 interdiff.txt3.04 KBjibran
#65 1974492-65.patch38.02 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,491 pass(es).
[ View ]
#65 interdiff.txt22.18 KBjibran
#63 1974492-63.patch21.53 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,460 pass(es).
[ View ]
#63 interdiff.txt828 bytesjibran
#61 1974492-61.patch21.59 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 57,428 pass(es).
[ View ]
#61 interdiff.txt658 bytesjibran
#59 1974492-59.patch21.59 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 57,447 pass(es), 10 fail(s), and 33 exception(s).
[ View ]
#59 interdiff.txt1.92 KBjibran
#57 taxonomy-overview-1974492.57.interdiff.txt1.45 KBlarowlan
#57 taxonomy-overview-1974492.57.patch21.99 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.admin.inc.
[ View ]
#51 taxonomy-overview-1974492.51.interdiff.txt1.11 KBlarowlan
#51 taxonomy-overview-1974492.51.patch36.61 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,126 pass(es).
[ View ]
#49 taxonomy-overview-1974492.49.interdiff.txt1.6 KBlarowlan
#49 taxonomy-overview-1974492.49.patch36.64 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,183 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#47 taxonomy-overview-1974492.47.interdiff.txt1.49 KBlarowlan
#47 taxonomy-overview-1974492.47.patch36.68 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 56,815 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#45 1974492-convert-taxonomy-overview-45.interdiff.txt573 bytestim-e
#45 1974492-convert-taxonomy-overview-45.patch36.66 KBtim-e
FAILED: [[SimpleTest]]: [MySQL] 56,800 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#41 1974492-convert-taxonomy-overview.patch36.66 KBtim-e
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#41 1974492-convert-taxonomy-overview.interdiff.txt608 bytestim-e
#39 drupal-convert_taxonomy_overview-1974492-39.patch36.65 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 57,167 pass(es), 61 fail(s), and 24 exception(s).
[ View ]
#32 drupal-convert_taxonomy_overview-1974492-32.patch36.55 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert_taxonomy_overview-1974492-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#24 taxonomy-overview.1974492.24.interdiff.txt578 bytesnick_schuch
#24 taxonomy-overview.1974492.24.patch36.4 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] 55,389 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
#17 taxonomy-overview.1974492.17.interdiff.txt30.35 KBlarowlan
#17 taxonomy-overview.1974492.17.patch36.51 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,480 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
#11 taxonomy-overview.1974492.11.patch37.95 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,194 pass(es), 125 fail(s), and 5 exception(s).
[ View ]
#7 taxonomy-overview.1974492.6.interdiff.txt9.23 KBlarowlan
#7 taxonomy-overview.1974492.6.patch59.35 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,208 pass(es), 128 fail(s), and 5 exception(s).
[ View ]
#7 taxonomy-overview.1974492.6.do-not-test.patch250.18 KBlarowlan
#3 taxonomy-overview.1974492.3.patch56.91 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 54,720 pass(es), 58 fail(s), and 6 exception(s).
[ View ]
#3 taxonomy-overview.1974492.3.do-not-test.patch34.29 KBlarowlan

Comments

Status:Postponed» Needs review

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview.1974492.3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new250.18 KB
new59.35 KB
FAILED: [[SimpleTest]]: [MySQL] 55,208 pass(es), 128 fail(s), and 5 exception(s).
[ View ]
new9.23 KB

An 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).

Any reason shortcut included in patch?

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview.1974492.6.patch, failed testing.

Looks like I did the patch a wrong

Status:Needs work» Needs review
StatusFileSize
new37.95 KB
FAILED: [[SimpleTest]]: [MySQL] 55,194 pass(es), 125 fail(s), and 5 exception(s).
[ View ]

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -227,6 +227,8 @@ function shortcut_set_switch_access($account = NULL) {
+ *
+ * @deprecated, use \Drupal\shortcut\Access\LinkDeleteAccessCheck instead.

i think this slipped from somewhere else

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,498 @@
+   * Injects plugin manager.

should be {inheritdoc}

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,498 @@
+    $page            = isset($_GET['page']) ? $_GET['page'] : 0;

lets use $this->request here

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,498 @@
+    $page_increment  = $this->config->get('terms_per_page_admin');
+    // Elements shown on this page.
+    $page_entries    = 0;
+    // Elements at the root level before this page.
+    $before_entries  = 0;
+    // Elements at the root level after this page.
+    $after_entries   = 0;
+    // Elements at the root level on this page.
+    $root_entries    = 0;
+
+    // Terms from previous and next pages are shown if the term tree would have
+    // been cut in the middle. Keep track of how many extra terms we show on each
+    // page of terms.
+    $back_step    = NULL;
+    $forward_step = 0;

Since we are there, lets fix this spacing

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,498 @@
+      if (module_invoke('translation_entity', 'translate_access', $term)) {

moduleHandler

Suppose better to convert path to remove add/edit limit #1978112: Convert taxonomy admin path to follow other core entity patterns

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview.1974492.11.patch, failed testing.

Status:Needs work» Postponed

in light of #13

Status:Postponed» Active

Path change is in

Status:Active» Needs review
StatusFileSize
new36.51 KB
FAILED: [[SimpleTest]]: [MySQL] 55,480 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
new30.35 KB

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

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview.1974492.17.patch, failed testing.

Status:Needs work» Postponed

Status:Postponed» Active

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

#17: taxonomy-overview.1974492.17.patch queued for re-testing.

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

The last submitted patch, taxonomy-overview.1974492.17.patch, failed testing.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -321,14 +321,8 @@ function taxonomy_menu() {
+  $items['admin/structure/taxonomy/%taxonomy_vocabulary'] = array(
+    'route_name' => 'taxonomy_overview_terms',

/manage was lost

Status:Needs work» Needs review
StatusFileSize
new36.4 KB
FAILED: [[SimpleTest]]: [MySQL] 55,389 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
new578 bytes

Fix as per #23

Looks rtbc!

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -24,6 +24,10 @@ public function form(array $form, array &$form_state) {
+    // @todo Title callbacks don't work with routes.
+    if ($vocabulary->name) {
+      drupal_set_title($vocabulary->name);
+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -322,13 +322,7 @@ function taxonomy_menu() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(4),
...
+    'route_name' => 'taxonomy_overview_terms',

not sure it make sense to have empty menu link

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,480 @@
+    // @todo entity_page_label does not work with new routing system.
+    drupal_set_title($taxonomy_vocabulary->label());
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyFormController.phpundefined
@@ -24,6 +24,10 @@ public function form(array $form, array &$form_state) {
+    // @todo Title callbacks don't work with routes.
+    if ($vocabulary->name) {
+      drupal_set_title($vocabulary->name);

why no use ->label()

We need the menu link for breadcrumbs?

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview.1974492.24.patch, failed testing.

Issue tags:+Needs reroll

Working 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

Assigned:larowlan» Unassigned

Status:Needs work» Needs review
StatusFileSize
new36.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert_taxonomy_overview-1974492-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reroll attached.

Status:Needs review» Needs work

The last submitted patch, drupal-convert_taxonomy_overview-1974492-32.patch, failed testing.

Assigned:Unassigned» maggo

Reviewing last patch

Title:Convert admin/structure/taxonomy/%taxonomy_vocabulary to FormInterface/ControllerConvert taxonomy_overview_terms to a Form Controller

Retitled in line with the other issues, so it is more easily found.

Status:Needs work» Needs review
Issue tags:-Needs screenshot, -Needs manual testing, -WSCCI, -Needs reroll, -FormInterface, -WSCCI-conversion

Also retesting to see if it still applies.

#32: drupal-convert_taxonomy_overview-1974492-32.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs screenshot, +Needs manual testing, +WSCCI, +Needs reroll, +FormInterface, +WSCCI-conversion

The last submitted patch, drupal-convert_taxonomy_overview-1974492-32.patch, failed testing.

Assigned:maggo» Unassigned

Status:Needs work» Needs review
StatusFileSize
new36.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,167 pass(es), 61 fail(s), and 24 exception(s).
[ View ]

reroll!

Status:Needs review» Needs work

The last submitted patch, drupal-convert_taxonomy_overview-1974492-39.patch, failed testing.

Issue tags:-WSCCI-conversion
StatusFileSize
new608 bytes
new36.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,136 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Reroll of #39 fix bug

Issue tags:+blocker, +WSCCI-conversion

Fix tags

Status:Needs work» Needs review

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

The last submitted patch, 1974492-convert-taxonomy-overview.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new36.66 KB
FAILED: [[SimpleTest]]: [MySQL] 56,800 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new573 bytes

Reroll and fix breadcrumbs

Status:Needs review» Needs work

The last submitted patch, 1974492-convert-taxonomy-overview-45.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new36.68 KB
FAILED: [[SimpleTest]]: [MySQL] 56,815 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.49 KB

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview-1974492.47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new36.64 KB
FAILED: [[SimpleTest]]: [MySQL] 57,183 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.6 KB

We're so close to removing forum.admin.inc, this is the last blocker.

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview-1974492.49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new36.61 KB
PASSED: [[SimpleTest]]: [MySQL] 57,126 pass(es).
[ View ]
new1.11 KB

Issue tags:-Needs reroll

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -245,7 +244,7 @@ public function buildForm(array $form, array &$form_state, Vocabulary $taxonomy_
-        '#href' => url($uri['path'], $uri['options']),

why not just '#href' => $uri['path'],

green!

Status:Needs review» Needs work

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,485 @@
+   * {@inheritdoc}

I don't think parent method has third param.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,485 @@
+  public function buildForm(array $form, array &$form_state, Vocabulary $taxonomy_vocabulary = NULL) {

Should be VocabularyInterface

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,485 @@
+      $operations = array(
+        'edit' => array(
+          'title' => t('edit'),
+          'href' => 'taxonomy/term/' . $term->id() . '/edit',
+          'query' => $destination,
+        ),
+        'delete' => array(
+          'title' => t('delete'),
+          'href' => 'taxonomy/term/' . $term->id() . '/delete',
+          'query' => $destination,
+        ),
+      );
+      if ($this->moduleHandler->invoke('content_translation', 'translate_access', array($term))) {
+        $operations['translate'] = array(
+          'title' => t('translate'),
+          'href' => 'taxonomy/term/' . $term->id() . '/translations',
+          'query' => $destination,
+        );
+      }
+      $form['terms'][$key]['operations'] = array(
+        '#type' => 'operations',
+        '#links' => $operations,

This should be the part of TermListController but we don't have one in Trem.php. I think we should create TermListController?

We 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 :(

Issue tags:+WSCCI-conversion

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

Status:Needs work» Needs review
StatusFileSize
new21.99 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.admin.inc.
[ View ]
new1.45 KB

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

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview-1974492.57.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new21.59 KB
FAILED: [[SimpleTest]]: [MySQL] 57,447 pass(es), 10 fail(s), and 33 exception(s).
[ View ]

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -395,6 +395,7 @@ function taxonomy_overview_terms_submit($form, &$form_state) {
+>>>>>>> origin/8.x

nice ;)

#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller is almost ready. Can we combine the patch?

Status:Needs review» Needs work

The last submitted patch, 1974492-59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new658 bytes
new21.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,428 pass(es).
[ View ]

hehe

<?php
-use Drupal\taxonomy\VocabularyInteface;
+use
Drupal\taxonomy\VocabularyInterface;
?>

Wow taxonomy is way over my head.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,497 @@
+    ($page = $this->request->query->get('page')) ?: 0;
...
+      $form_state['redirect'] = array(current_path(), ($this->request->query->get('page') ? array('query' => array('page' => $this->request->query->get('page'))) : array()));

Why not use the $page variable defined above?

StatusFileSize
new828 bytes
new21.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,460 pass(es).
[ View ]

Wow taxonomy is way over my head.

Same here :).
Fixed #62.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new22.18 KB
new38.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,491 pass(es).
[ View ]

Good catch @klausi. Thanks for the review.

  • Removed taxonomy_overview_terms().
  • Removed taxonomy_overview_terms_submit().
  • Added doc to OverviewTerms::buildForm() and OverviewTerms::submitForm()
  • Injected TranslationManager.

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +Novice

Overall 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:

  1. +++ b/core/modules/shortcut/shortcut.moduleundefined
    @@ -217,6 +217,8 @@ function shortcut_set_switch_access($account = NULL) {
    /**
      * Access callback for editing a link in a shortcut set.
    + *
    + * @deprecated, use \Drupal\shortcut\Access\LinkDeleteAccessCheck instead.
      */

    Merge artifact?

  2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
    @@ -0,0 +1,534 @@
    +use Drupal\Core\Form\FormInterface;
    +use Drupal\Core\Controller\ControllerInterface;
    +use Drupal\Core\Entity\EntityManager;
    +use Drupal\Core\Config\ConfigFactory;
    +use Drupal\Core\Database\Connection;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    +use Drupal\Core\StringTranslation\TranslationManager;
    +use Drupal\taxonomy\VocabularyInterface;
    +use Symfony\Component\HttpFoundation\Request;
    +use Symfony\Component\DependencyInjection\ContainerInterface;

    Wow. Dependencies. We've got 'em!

  3. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
    @@ -0,0 +1,534 @@
    +class OverviewTerms implements ControllerInterface, FormInterface {

    Need a docblock here.

  4. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
    @@ -0,0 +1,534 @@
    +    global $pager_page_array, $pager_total, $pager_total_items;

    Global alert. Do we have an existing issue for this? It appears that only this form and Views use these globals in core.

  5. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
    @@ -0,0 +1,534 @@
    +   * lowest to highest, but are not necessarily sequential. Numbers may be skipped

    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:

  • Test the scenarios before and after the patch so you know what to expect and what's not a regression (as there is at least one outstanding bug, and I think I also found an existing regression in HEAD while I was poking at it just now).
  • Test over 200 terms.
  • Test complex hierarchies.
  • Drag and drop to reorder and indent things.
  • Be creative. Try to break it.
  • Play with the "reset to alphabetical" feature.
  • Play with showing weights and manually entering them.

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.

#2044435: Convert pager.inc to a Pager component is for the pager logic, which we postponed as 'too late', if you could chime in there @xjm we could restart it.

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

Oh, and for this issue, let's just stick an @todo in there for #67.

Status:Needs work» Needs review
Issue tags:-Needs issue summary update
StatusFileSize
new3.04 KB
new37.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,146 pass(es).
[ View ]

Fixed #66 added issues to summary.

Issue tags:-Novice

The novice task is also resolved.

Status:Needs review» Needs work
Issue tags:+Needs reroll

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.phpundefined
@@ -0,0 +1,538 @@
+    // Check for confirmation forms.
+    if (isset($form_state['confirm_reset_alphabetical'])) {
+      // @todo this needs to be a separate route/controller for a confirm form.
+      //   Will be fixed in http://drupal.org/node/1976296.
+      $this->moduleHandler->loadInclude('taxonomy', 'admin.inc');
+      return taxonomy_vocabulary_confirm_reset_alphabetical($form, $form_state, $taxonomy_vocabulary->id());
+    }
...
+      // @todo this needs to be a separate method.
+      //   Will be fixed in http://drupal.org/node/1976296.
+      if ($form_state['values']['reset_alphabetical'] === TRUE) {
+        $this->moduleHandler->loadInclude('taxonomy', 'admin.inc');
+        return taxonomy_vocabulary_confirm_reset_alphabetical_submit($form, $form_state);

#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller is committed. This needs re-roll.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new38.61 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/taxonomy.admin.inc.
[ View ]

Re-rolled the patch. This is of my first re-rolls, so if I botched this, my apologies.

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview-terms.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new38.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,068 pass(es).
[ View ]

Trying another re-roll.

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

The last submitted patch, taxonomy-overview-terms.patch, failed testing.

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

#75: taxonomy-overview-terms.patch queued for re-testing.

Status:Needs review» Needs work

Thanks for working on this @mdrummond! I think the reroll needs to be revisited…

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -20,371 +20,38 @@ function taxonomy_vocabulary_add() {
+function taxonomy_vocabulary_confirm_reset_alphabetical_submit($form, &$form_state) {
+  db_update('taxonomy_term_data')
+    ->fields(array('weight' => 0))
+    ->condition('vid', $form_state['values']['vid'])
+    ->execute();
+  drupal_set_message(t('Reset vocabulary %name to alphabetical order.', array('%name' => $form_state['values']['name'])));
+  watchdog('taxonomy', 'Reset vocabulary %name to alphabetical order.', array('%name' => $form_state['values']['name']), WATCHDOG_NOTICE);
+  $form_state['redirect'] = 'admin/structure/taxonomy/manage/' . $form_state['values']['vid'];

This shouldn't be added back, it was removed in #1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller.

Status:Needs work» Needs review
StatusFileSize
new27.98 KB
FAILED: [[SimpleTest]]: [MySQL] 58,178 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

I rerolled the last patch and removed taxonomy_vocabulary_confirm_reset_alphabetical_submit(). This is ready for another review.

Status:Needs review» Needs work

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

Thanks @Cottser! I'm on it.

Status:Needs work» Needs review
StatusFileSize
new37.03 KB
FAILED: [[SimpleTest]]: [MySQL] 58,113 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Alright, one more time here. This looks a lot closer to the patch at comment #70. Ready for review.

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview-terms-1974492-82.patch, failed testing.

Issue tags:+Needs reroll

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/OverviewTerms.php
@@ -0,0 +1,538 @@
+      // @todo this needs to be a separate route/controller for a confirm form.
+      //   Will be fixed in http://drupal.org/node/1976296.
+      $this->moduleHandler->loadInclude('taxonomy', 'admin.inc');
+      return taxonomy_vocabulary_confirm_reset_alphabetical($form, $form_state, $taxonomy_vocabulary->id());
...
+      // @todo this needs to be a separate method.
+      //   Will be fixed in http://drupal.org/node/1976296.
+      if ($form_state['values']['reset_alphabetical'] === TRUE) {
+        $this->moduleHandler->loadInclude('taxonomy', 'admin.inc');
+        return taxonomy_vocabulary_confirm_reset_alphabetical_submit($form, $form_state);

#1976296: Convert taxonomy_vocabulary_confirm_reset_alphabetical() to FormInterface/Controller is committed.

Status:Needs work» Needs review
Issue tags:-Needs manual testing, -Needs reroll, -blocker, -FormInterface, -WSCCI-conversion

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Needs reroll, +blocker, +FormInterface, +WSCCI-conversion

The last submitted patch, taxonomy-overview-terms-1974492-82.patch, failed testing.

StatusFileSize
new2.06 KB
new18.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,083 pass(es).
[ View ]

Didn'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.

Status:Needs work» Needs review

I think we lost the part of the patch that removed the old form.

Status:Needs review» Needs work

Setting to needs work as per #89.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new35.93 KB
FAILED: [[SimpleTest]]: [MySQL] 58,204 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

This is just a straight reroll of #82 with the interdiff in #87 applied on top. I have not reviewed it at all.

Component:base system» taxonomy.module

+++ b/core/modules/taxonomy/taxonomy.admin.inc
@@ -19,372 +19,3 @@ function taxonomy_vocabulary_add() {
-  if ($form_state['triggering_element']['#value'] == t('Reset to alphabetical')) {
-    // Redirect to confirmation form for the reset action.
-    $form_state['redirect'] = 'admin/structure/taxonomy/manage/' . $form_state['taxonomy']['vocabulary']->id() . '/reset';
-    return;
-  }

Why is this not the part of new submit function?

I'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]

Status:Needs review» Needs work

The last submitted patch, taxonomy-overview-terms-1974492-91.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.12 KB
new34.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,992 pass(es).
[ View ]

It 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... :(

Issue tags:-Needs manual testing

I 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

  1. Test the scenarios before and after the patch so you know what to expect and what's not a regression (as there is at least one outstanding bug, and I think I also found an existing regression in HEAD while I was poking at it just now).
  2. Test over 200 terms.
  3. Test complex hierarchies.
  4. Drag and drop to reorder and indent things.
  5. Be creative. Try to break it.
  6. Play with the "reset to alphabetical" feature.
  7. Play with showing weights and manually entering them.
  1. A lot of bugs present in the HEAD. i.e. indent not working, pager is not working, sorting with pager is not working, and etc.
  2. Nothing new same bug which already present in HEAD.
  3. Drag and drop to reorder is working indent not working in HEAD and after this patch.
  4. It is already broken :(
  5. working fine with no pager same in the HEAD.
  6. working fine with no pager same in the HEAD.

It is RTBC for me but I worked on this patch let's wait for @xjm.

Status:Needs review» Needs work

This needs a reroll for FormBase

Status:Needs work» Needs review
StatusFileSize
new34.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,480 pass(es), 46 fail(s), and 32 exception(s).
[ View ]

reroll. It already is using FormBase though, so no changes in that. Basically just removed taxonomy.admin.inc, and merged taxonomy.routing.yml

Status:Needs review» Needs work

The last submitted patch, drupal8.taxonomy-module.1974492-100.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new34.7 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new527 bytes
new34.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,280 pass(es).
[ View ]

uploaded wrong interdiff file.

Back to @xjm for review.

Status:Needs review» Reviewed & tested by the community

I could not find anything, it passes, so RTBC.

Status:Reviewed & tested by the community» Fixed

Awesome work, folks!

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

Added related issues