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

larowlan’s picture

andypost’s picture

larowlan’s picture

larowlan’s picture

Status:Postponed» Needs review

Status:Needs review» Needs work

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

andypost’s picture

larowlan’s picture

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

andypost’s picture

Any reason shortcut included in patch?

Status:Needs review» Needs work

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

larowlan’s picture

Looks like I did the patch a wrong

larowlan’s picture

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

+++ 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

andypost’s picture

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.

larowlan’s picture

Status:Needs work» Postponed

in light of #13

larowlan’s picture

Status:Postponed» Active

Path change is in

larowlan’s picture

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.

larowlan’s picture

Status:Needs work» Postponed
larowlan’s picture

Status:Postponed» Active
larowlan’s picture

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.

andypost’s picture

+++ 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

nick_schuch’s picture

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

andypost’s picture

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

andypost’s picture

+++ 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()

larowlan’s picture

We need the menu link for breadcrumbs?

Status:Needs review» Needs work

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

andypost’s picture

Issue tags:+Needs reroll
andypost’s picture

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

larowlan’s picture

Assigned:larowlan» Unassigned
disasm’s picture

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.

maggo’s picture

Assigned:Unassigned» maggo

Reviewing last patch

Pancho’s picture

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

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

Pancho’s picture

Status:Needs work» Needs review
Issue tags:-Needs screenshots, -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 screenshots, +Needs manual testing, +WSCCI, +Needs reroll, +FormInterface, +WSCCI-conversion

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

maggo’s picture

Assigned:maggo» Unassigned
disasm’s picture

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.

tim-e’s picture

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

tim-e’s picture

Issue tags:+blocker, +WSCCI-conversion

Fix tags

tim-e’s picture

Status:Needs work» Needs review

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

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

tim-e’s picture

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.

larowlan’s picture

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.

larowlan’s picture

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.

larowlan’s picture

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

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

larowlan’s picture

green!

jibran’s picture

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?

tim.plunkett’s picture

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

larowlan’s picture

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.

larowlan’s picture

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.

jibran’s picture

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.

jibran’s picture

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;
?>
dawehner’s picture

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?

jibran’s picture

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.

klausi’s picture

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?

jibran’s picture

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.
xjm’s picture

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.

larowlan’s picture

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

xjm’s picture

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.

xjm’s picture

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

xjm’s picture

jibran’s picture

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.

xjm’s picture

Issue tags:-Novice

The novice task is also resolved.

jibran’s picture

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.

mdrummond’s picture

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.

mdrummond’s picture

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.

disasm’s picture

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

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

Cottser’s picture

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.

robeano’s picture

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.

Cottser’s picture

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.

robeano’s picture

Thanks @Cottser! I'm on it.

robeano’s picture

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.

jibran’s picture

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.

puregin’s picture

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.

wamilton’s picture

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.

disasm’s picture

Status:Needs work» Needs review
Gaelan’s picture

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

pfrenssen’s picture

Status:Needs review» Needs work

Setting to needs work as per #89.

effulgentsia’s picture

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.

effulgentsia’s picture

Component:base system» taxonomy.module
jibran’s picture

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

disasm’s picture

effulgentsia’s picture

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.

tim.plunkett’s picture

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

jibran’s picture

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.

tim.plunkett’s picture

Status:Needs review» Needs work

This needs a reroll for FormBase

disasm’s picture

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.

disasm’s picture

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.

disasm’s picture

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

uploaded wrong interdiff file.

jibran’s picture

Back to @xjm for review.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

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.

Anonymous’s picture

Issue summary:View changes

Added related issues