I only have 1 vocabulary activated for TM.
I should find no traces of TM when manipulating other vocabularies, however:
- when I create and add a new vocabulary, (without TM activated on that vocabulary), the message 'The Taxonomy Menu has been removed.' comes up.
- when I check the variables table (e.g., with page devel/variable) This is filled with all vacab's not only the activated.

IMO, this is confusing, and makes me curious if the module processes too many code at other moments, too (CRUD on terms, CRUD on Vocab's).

This happens in function taxonomy_menu_vocab_submit(). It should be sanitized.

IMO, when saving the vocabulary settings,
- if 'Menu location' is and was empty, the function should do nothing;
- if 'Menu location' is unset, the menu items must be rebuilt(deleted) and the variables can be deleted (but can be kept, too.)
- if 'Menu location' is changed, the menu items must be rebuilt and the variables must be set.
- if an option is not set, no variable should be set either.
(When any of the settings was set before, you may set and keep it.)

Files: 
CommentFileSizeAuthor
#29 taxonomy_menu_1896916_29_submit_settings.patch5.82 KBjohnv
PASSED: [[SimpleTest]]: [MySQL] 483 pass(es).
[ View ]
#26 taxonomy_menu_1896916_26_submit_settings.patch5.89 KBjohnv
PASSED: [[SimpleTest]]: [MySQL] 483 pass(es).
[ View ]
#25 taxonomy_menu_1896916_25_submit_settings.patch6.25 KBjohnv
PASSED: [[SimpleTest]]: [MySQL] 483 pass(es).
[ View ]
#16 taxonomy_menu_1896916_16_submit_settings.patch7 KBjohnv
PASSED: [[SimpleTest]]: [MySQL] 487 pass(es).
[ View ]
#11 taxonomy_menu_1896916_11_submit_settings.patch5.66 KBjohnv
FAILED: [[SimpleTest]]: [MySQL] 447 pass(es), 32 fail(s), and 0 exception(s).
[ View ]
#8 taxonomy_menu_1896916_8_submit_settings.patch5.38 KBjohnv
PASSED: [[SimpleTest]]: [MySQL] 487 pass(es).
[ View ]
#2 taxonomy_menu_1896916_submit.patch3.35 KBjohnv
FAILED: [[SimpleTest]]: [MySQL] 101 pass(es), 12 fail(s), and 2 exception(s).
[ View ]

Comments

johnv’s picture

Version:7.x-2.x-dev» 7.x-1.4

Oops, wrong version.

johnv’s picture

Status:Active» Needs review
StatusFileSize
new3.35 KB
FAILED: [[SimpleTest]]: [MySQL] 101 pass(es), 12 fail(s), and 2 exception(s).
[ View ]

See attached patch.

I left an indent, for better readability of the patch.

P.S. we might want to create an update function to remove cleans the variables table.

Status:Needs review» Needs work

The last submitted patch, taxonomy_menu_1896916_submit.patch, failed testing.

hles’s picture

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 ! :)

johnv’s picture

I am happy to create a patch for the version you like.
Indeed, I am confused about the status of version 1 and 2.
Am I correct: Version 1 is now stable, and all developing effort goes to version 2?

hles’s picture

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.

johnv’s picture

I understand. Putting this info on the project page will be helpful. Thanks!

johnv’s picture

Version:7.x-1.4» 7.x-2.x-dev
Status:Needs work» Needs review
StatusFileSize
new5.38 KB
PASSED: [[SimpleTest]]: [MySQL] 487 pass(es).
[ View ]

Attached patch is updated for 2.x , and does the following in the submit callback:
- do not touch vocab's that are not activated for TM.
- do not save the 'rebuild menu' setting. This is a one-time action.
- moving between parents was not captured.

hles’s picture

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 ?

hles’s picture

Issue tags:+Need tests

Tagging accordingly.

johnv’s picture

StatusFileSize
new5.66 KB
FAILED: [[SimpleTest]]: [MySQL] 447 pass(es), 32 fail(s), and 0 exception(s).
[ View ]

Attached patch addresses your remark #9.
It also makes the fist lines of the function a bit more readable.

IMO, the value 'vocab_parent' can be used and stored as a variable unchanged. ATM it is normalized and deconstructed into parent and menu, and 2 variables are saved. The use of the second variable 'vocab_menu' seems superfluous.
This might need an update-function, but can also be done with a variable_del() upon saving.
It is not applied in this patch - need your opinion first, and can be addressed in a follow-up issue.

The last submitted patch, taxonomy_menu_1896916_11_submit_settings.patch, failed testing.

johnv’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, taxonomy_menu_1896916_11_submit_settings.patch, failed testing.

johnv’s picture

Status:Needs work» Needs review
Issue tags:+Need tests
johnv’s picture

Title:For a new Vocabulary, taxonomy_menu generates messages ('The Taxonomy Menu has been removed.')» For new vocabularies, taxonomy_menu must not generates messages
StatusFileSize
new7 KB
PASSED: [[SimpleTest]]: [MySQL] 487 pass(es).
[ View ]

Last patch did not save the settings in all cases.

hles’s picture

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.

hles’s picture

Status:Needs review» Needs work
Issue tags:-Need tests

Changing status just for the reroll against latest dev.

johnv’s picture

Status:Needs work» Needs review

Can you give your opinion on my #11 suggestion? IMO it simplifies the code.

hles’s picture

    //  - "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" ?

johnv’s picture

Indeed,
there is no point in separating the values before storing, and re-assemble them before re-use. Just save the variable taxonomy_menu__parent = "main-menu:123" .
For not-active vocabs, it would be taxonomy_menu__parent = "0" IIRC.
But there is no need anymore to normalize, reassemble and destruct: just save, compare the submitted value with the old value, or compare the submitted value with '0'.

hles’s picture

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 ?

johnv’s picture

Title:For new vocabularies, taxonomy_menu must not generate messages» For new vocabularies, taxonomy_menu must not generates messages
Assigned:johnv» Unassigned

I have checked that: It is not used anywhere, but I will check again.(how else would you know where to append the menu items?).
[EDIT] both variables are used in several places. Let's leave it like it is.

johnv’s picture

Title:For new vocabularies, taxonomy_menu must not generates messages» For new vocabularies, taxonomy_menu must not generate messages
Assigned:Unassigned» johnv

Testing the 'assigned' feature.

johnv’s picture

Title:For new vocabularies, taxonomy_menu must not generates messages» For new vocabularies, taxonomy_menu must not generate messages
Assigned:Unassigned» johnv
StatusFileSize
new6.25 KB
PASSED: [[SimpleTest]]: [MySQL] 483 pass(es).
[ View ]

Attached patch moves the 'menu_rebuild_needed', as you stated.
I also discovered that a menu disable/remove is the same as a rebuild, and changed the code accordingly.

johnv’s picture

StatusFileSize
new5.89 KB
PASSED: [[SimpleTest]]: [MySQL] 483 pass(es).
[ View ]

Even shorter and more explicit.

hles’s picture

Status:Needs review» Reviewed & tested by the community

I'll commit this when I have time.

hles’s picture

Status:Reviewed & tested by the community» Needs work

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.

johnv’s picture

Status:Needs work» Needs review
StatusFileSize
new5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 483 pass(es).
[ View ]

In attached patch:
- I removed the mentioned comments - code speaks for itself.
- I moved variable_set('menu_rebuild_needed', TRUE); back to the last line. This was OK in the original code.

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.

THis line needs to be there, otherwise there is no menu update. Since I might have understood you incorrectly, I tried switching taxonomy_menu_menu_links_insert() vs. taxonomy_menu_menu_links_update() . Both work equally fine, where _update() seems a bit faster. I did not change that line of code, since it is not the scope of the patch.
However, it seems both function effectively do the same thing, in a slightly differfent way. IMO one of both can be discarded, but I may miss some fineprint.

I cannot help you with automated tests. I did not encounter a project big enough to invest in that (valuable) feature.

hles’s picture

Status:Needs review» Needs work
Issue tags:+Need 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.

menks’s picture

Status:Needs work» Needs review
Issue tags:-Need tests
menks’s picture

Issue summary:View changes

better explanation.