For new vocabularies, taxonomy_menu must not generate messages

This is great that you spend some time working on Taxonomy Menu Johnv. But unfortunately this kind of patch will unlikely make it in the 1.x version. We already use our free time (which is not a lot) to integrate missing features into 2.x and polish it, which btw already takes care more or less of this issue. Don't get me wrong, this is great, but that would be some much greater if you could provide this kind of patch for the 2.x version instead ! :)

1.4 is stable, 2.x was created to handle internationalization at first but it ended up heavily modified, a lot of tests were added to make sure things are working correctly, which is not the case in 1.x. Also, all new features are now written against 2.x and won't be backported to 1.x. This is also a problem of maintainers being very busy and not having the time to maintain 6.x, 7.x-1.x and 7.x-2.x at the same time.

These changes make sense to me. It would be nice to reflect these changes in the functional tests. Should be pretty straight forward. But overall that looks good to me.

+++ b/taxonomy_menu.admin.incundefined
@@ -194,47 +194,54 @@ function taxonomy_menu_vocab_submit($form, &$form_state) {
+      // Menu location has been changed and is not disabled.
+      $update = TRUE;
+      $insert = TRUE;

There is one thing I don't feel right is that we need insert and update flags to TRUE at the same time ?

Tagging accordingly.

This seems clean to me and almost ready for a commit. Don't forget to merge with the latest dev, 2 commits have been added since your last patch.

+++ b/taxonomy_menu.admin.incundefined
@@ -180,53 +180,62 @@ function taxonomy_menu_form_taxonomy_form_vocabulary(&$form, &$form_state) {
+    variable_set('menu_rebuild_needed', TRUE);
+  }

Maybe these rebuild_needed could be set after the if/elseif statement but this is really minor.

Changing status just for the reroll against latest dev.

    //  - "0:0" : DISABLED
    //  - "main-menu:0" : MENU ROOT
    //  - "main-menu:123" : MENU ITEM ROOT

Both solutions would work, but I'm not sure why it would simplify the code. What you are saying is that we should save "main-menu:123" value (for ex) instead of both "main-menu" and "123" ?

I see what you mean for the admin part. But wouldn't we have to deconstruct the variable each time when "parent" or "menu" is used elsewhere in the module ?

I'll commit this when I have time.

Re-reading the code, I found the following:

+++ b/taxonomy_menu.admin.incundefined
@@ -180,71 +180,61 @@ function taxonomy_menu_form_taxonomy_form_vocabulary(&$form, &$form_state) {
+  // Do a full menu rebuild in case we have removed or moved the menu.

The comment is not correct, if the menu has moved, then we need an update, not a rebuild.

+++ b/taxonomy_menu.admin.incundefined
@@ -180,71 +180,61 @@ function taxonomy_menu_form_taxonomy_form_vocabulary(&$form, &$form_state) {
     // Update only menu links that are available in taxonomy_menu table.
     taxonomy_menu_menu_links_insert($vid);
   }

We need to insert menu links only if the previous menu location was "NONE/DISABLED". Which is already done in the first condition above I think. I'm pretty sure we can remove this condition.

These use cases should be added in testTaxonomyMenuVocabularyInterface so we don't have to wonder about this logic (mostly menu changes) later. Moreover this is just a matter of few lines in the tests.

Yes I think both functions do the same thing, we will need to clarify that better in another issue.

I'll provide the tests whenever I can find some free time, if nobody does, and commit this very patch. I'm sorry not being able to be more reactive on this, but there are just too many things I have to do right now.

Looks like this needs a reroll, there is no taxonomy_menu.admin.inc anymore.

edit: I updated the patch so it would affect the .module instead, but the patch still failed. Additionally, it doesn't look like there is a function _taxonomy_menu_flatten_form_settings anymore, so this will likely need more than just a reroll.

I'm attaching a new simplified patch that solves the problem of the 'The Taxonomy Menu has been removed.' message appearing for new vocabs.

  // If the menu had never been enabled, skip the rest.
  if ($current_setting === FALSE && is_numeric($form_state['values']['taxonomy_menu']['vocab_parent'])) {
    return;
  }

this one should pass tests.

Oh, the correct branch will help.

trying to get testing working - uploading patch from #35 again.

status

Thank you for your contributions.
Drupal 7 is no longer supported.
Closing this issue as outdated.