CommentFileSizeAuthor
#103 drupal8.taxonomy-module.1974492-102.patch34.7 KBdisasm
#103 interdiff.txt527 bytesdisasm
#102 drupal8.taxonomy-module.1974492-102.patch34.7 KBdisasm
#102 interdiff.patch6.45 KBdisasm
#100 drupal8.taxonomy-module.1974492-100.patch34.39 KBdisasm
#97 taxo-1974492-97.patch34.38 KBtim.plunkett
#97 interdiff.txt8.12 KBtim.plunkett
#91 taxonomy-overview-terms-1974492-91.patch35.93 KBeffulgentsia
#87 1974492-87.patch18.85 KBwamilton
#87 interdiff.txt2.06 KBwamilton
#82 taxonomy-overview-terms-1974492-82.patch37.03 KBrobeano
#79 taxonomy-overview-terms-1974492-79.patch27.98 KBrobeano
#75 taxonomy-overview-terms.patch38.53 KBRainbowArray
#73 taxonomy-overview-terms.patch38.61 KBRainbowArray
#70 1974492-70.patch37.59 KBjibran
#70 interdiff.txt3.04 KBjibran
#65 1974492-65.patch38.02 KBjibran
#65 interdiff.txt22.18 KBjibran
#63 1974492-63.patch21.53 KBjibran
#63 interdiff.txt828 bytesjibran
#61 1974492-61.patch21.59 KBjibran
#61 interdiff.txt658 bytesjibran
#59 1974492-59.patch21.59 KBjibran
#59 interdiff.txt1.92 KBjibran
#57 taxonomy-overview-1974492.57.interdiff.txt1.45 KBlarowlan
#57 taxonomy-overview-1974492.57.patch21.99 KBlarowlan
#51 taxonomy-overview-1974492.51.interdiff.txt1.11 KBlarowlan
#51 taxonomy-overview-1974492.51.patch36.61 KBlarowlan
#49 taxonomy-overview-1974492.49.interdiff.txt1.6 KBlarowlan
#49 taxonomy-overview-1974492.49.patch36.64 KBlarowlan
#47 taxonomy-overview-1974492.47.interdiff.txt1.49 KBlarowlan
#47 taxonomy-overview-1974492.47.patch36.68 KBlarowlan
#45 1974492-convert-taxonomy-overview-45.interdiff.txt573 bytestim-e
#45 1974492-convert-taxonomy-overview-45.patch36.66 KBtim-e
#41 1974492-convert-taxonomy-overview.patch36.66 KBtim-e
#41 1974492-convert-taxonomy-overview.interdiff.txt608 bytestim-e
#39 drupal-convert_taxonomy_overview-1974492-39.patch36.65 KBdisasm
#32 drupal-convert_taxonomy_overview-1974492-32.patch36.55 KBdisasm
#24 taxonomy-overview.1974492.24.interdiff.txt578 bytesnick_schuch
#24 taxonomy-overview.1974492.24.patch36.4 KBnick_schuch
#17 taxonomy-overview.1974492.17.interdiff.txt30.35 KBlarowlan
#17 taxonomy-overview.1974492.17.patch36.51 KBlarowlan
#11 taxonomy-overview.1974492.11.patch37.95 KBlarowlan
#7 taxonomy-overview.1974492.6.interdiff.txt9.23 KBlarowlan
#7 taxonomy-overview.1974492.6.patch59.35 KBlarowlan
#7 taxonomy-overview.1974492.6.do-not-test.patch250.18 KBlarowlan
#3 taxonomy-overview.1974492.3.patch56.91 KBlarowlan
#3 taxonomy-overview.1974492.3.do-not-test.patch34.29 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

andypost’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

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
FileSize
37.95 KB
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
FileSize
36.51 KB
30.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
FileSize
36.4 KB
578 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
FileSize
36.55 KB

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
FileSize
36.65 KB

reroll!

Status: Needs review » Needs work

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

tim-e’s picture

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
FileSize
36.66 KB
573 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 review » Needs work

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
36.64 KB
1.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
FileSize
36.61 KB
1.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
FileSize
21.99 KB
1.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
FileSize
1.92 KB
21.59 KB
+++ 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
FileSize
658 bytes
21.59 KB

hehe


-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

FileSize
828 bytes
21.53 KB

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
FileSize
22.18 KB
38.02 KB

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 service 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
FileSize
3.04 KB
37.59 KB

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.

RainbowArray’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
38.61 KB

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.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
38.53 KB

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.

star-szr’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
FileSize
27.98 KB

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

star-szr’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
FileSize
37.03 KB

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

FileSize
2.06 KB
18.85 KB

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
FileSize
35.93 KB

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
FileSize
8.12 KB
34.38 KB

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
FileSize
34.39 KB

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
FileSize
6.45 KB
34.7 KB

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

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