After disabling this module it doesn't remove classes already added

Confirm.

I have same problem.

Can you confirm they are not removed after you've flushed the cache?

Well after review it seems that we are hijacking the menu_links table's options column, which is great for performance reasons. Though there is no clean-up script to remove them once the module is disabled or uninstalled.

It would be nice if I could just remove the attributes from that column's serialized object but that would be hasty because I know there are other things in core that include attributes and potentially other modules hijacking that column too...

So here's the only proposal I could think of:

• We create a new table to keep track of menu_attributes attributes.
• It has no purpose other than collecting values provided by menu_attributes for eventual cleanup script to compare against.
• On disable it could remove them from menu_links but keep the menu_attributes table.
• On uninstall it could remove them and delete that table.

This may be overkill, but thoughts on the matter would be appreciated.

I agree, this feature would require the approach that @joelpittet hint in #4, I neither think about other approach without possibly harming the actions of others on the menu options column.

... and I also agree that it would be a quite overkill solution right now, it should have been covered at the beginning stages of the module's development.

There is an annoying workaround to get rid of the attributes: just manually clear the entries in the form for each menu item before disabling/uninstalling the module.

May be a notice about this in a (missing ;)) README.txt would be enough for now...

@interdruper That sounds like a good solution for now, want to propose a README.txt change patch. Gladdly review and commit to -dev.

There is an almost identical issue in the queue for the menu_target module (which performs a very similar task as this, albiet with a much simpler feature set)

#2320615: Target remains applied even when module is uninstalled

I figured out a potential solution to this problem using hook_menu_link() to modify the link and insert various attributes at render-time versus being saved into the menu_links options field as it is being performed now.

A similar solution could apply here, and could even be used toward requests such as #1862048: Class attribute for entire menu and #1994942: request: Can attributes be added to tags other than <a> if implemented correctly.

Basically, a distinct table will contain any additional attributes per link-item, and can be detected and modified when that link item is subsequently rendered by Drupal. The benefit of using hook_menu_link() is that it would allow you to modify both the <a> tag as well as its container <li> tags with various attributes.

Since the menu_target module is much more simple, I can attempt to create a patch toward that code base and then apply the same logic to a patch here, if you'd like?

@oranges13 please do, and keep in mind performance as menu_links are already quite a heavy sql usually and cached due to volume per page.

tl;dr; I have the theory for this fix all worked out and code to go with it, I just need some implementation guidance!

@joelpittet So I have a working solution to the basic issue that I believe I can apply to this module as well. Here's some more information, and I do have some questions about how to go about implementing it as well:

The menu_link_form_submit functions are unaware of the mlid for newly created menu items, so that makes it tricky to insert these values into a custom table as the module currently inserts them into the options column. However, hook_menu_link_insert works perfectly for this use case. Since we're injecting the custom attributes to the menu form array, they come through to this function as well.

Similarly, hook_menu_link_delete can remove a row from our custom table if a menu link is removed (either through the node edit form or the menu admin interface).

We still need to maintain the custom submit hook, however, because even though there is a hook_menu_link_update, it will only fire if the default menu attributes are changed. So if someone modified our custom attributes but did not change any of the core menu link attributes, the update hook would not actually occur. We must instead continue to perform this transaction in the submit hook.

Ok, so that's the suggested modifications so that we can save the custom attributes in a custom table instead of the options array within menu_links. But, how do we get the data back out?

When a menu item is rendered, we have the mlid available and can use that to query for and inject any custom attributes the user has set.
hook_preprocess_menu_link can be used to insert the attributes into the link render array (and in the future could be used to add attributes to the containing <li> element as well.)

Ok, that's the fundamental problem of adding the attributes to the menu_links table fixed.

Now my quandry is that I can write a patch that includes an update hook to remove any extant custom attributes that may be present within the menu_links table. However, I know that Drupal requires the version # to increment in order for an update to run effectively, and I also know that the version numbers are dynamically added by the versioning system and can't be incremented by a patch. So how do you propose I integrate the removal of custom attributes currently in the menu_links table into a patch which will work for everyone? Nevermind, updates run fine as long as the schema version makes sense.

That is long, not sure I understand much of of it completely, mind just throwing together some code in the direction you are looking to take this and we can review/massage it? Like I said before getting data is a performance issue and I'm not aware of a good solution. If you can somehow(haven't look to see if it's possible) alter the menu_links select statement to join on the other table, you may be able to mitigate risk a bit on the performance.

I have a patch going on for the same sort of issue here which is where I developed this logic.
#2320615: Target remains applied even when module is uninstalled

Perhaps instead of using hook_preprocess_menu_link() we could use something like is described here, just not for translation: http://drupal.stackexchange.com/a/59029

The problem still stands, however, because hooks like hook_menu_link_alter simply replicate what we're already doing by saving directly into the menu_links table, which is what we're trying to avoid. There's plenty of hooks to change what goes into that table, and almost none that change what comes out of it. :(

@oranges13 how practical would it be to keep a second table to keep track of attributes added by menu_attributes but also add them to the options column of menu_links. Then on uninstall it would just array_diff() remove them and delete that capture table?

It would, foreseeably, make enabling and disabling the module extremely expensive.

Disable would have to go through all the menu items and remove the options set by this module, and enable would have to go through and put them back. Uninstall would drop the custom table.

I'm looking but I've never encountered a module that utilizes a batch process within hook_disable or hook_enable, and I'm not sure what would happen if we did.

Yeah you're right, for the sake of performance:

1. 10,000 Menu links
2. menu_attributes left joined against menu_links table.
3. On each record:
4. unserialize attributes in both options columns.
5. array_diff()
6. save menu_links record.

And the reverse would:
array_merge() them back on.

Assuming that was how it was done.

I have an idea. But I didn't do any verification to see if the core would allow it.

Could the variables or something similar be used to store the menu attributes things outside the menu table? Or maybe, use the schema API to add an extra column to the menu_links tables. That way, a query_rewriting can inject the extra column for menu items... and it will be possible to drop that column upon uninstall. Also, the query_rewrite will not occur if the module is disabled.

If this is still a current issue ( I no longer use this module so I'm not sure ) I fixed a similar issue on a different module. I can look at applying the same solution to this module as well

I'd be interested in creating a 2.x branch for 7 and 8 to implement a separate table with schema API as long as we can show a performance that is not horrible with lots of menu links. I think it's worth a try though, thoughts?

Well ideally, any of the batch process could be completed in an upgrade hook once and then enabling or disabling could just ignore the custom table.

I can take another look today.

Good ideas @oranges13 and @joelpitter. I am in. However, we will need to have a chat with the current maintainer(s).

@asiby, you may not have noticed, but I'm a current maintainer...

Oh. Shame on me. I didn't notice. That's excellent then.

So whenever you are ready to start. I am available to help at any level.

Hey all, I have quite a bit of this as a WIP hopefully will have a patch up for testing and feedback today.

Good stuff

This is still an issue with even the latest release. Any update on the patch ?

My bad. The issue was with another custom js code in place.

Yes still an issue, not likely fixed in 7.x. Needs a way to keep track and merge which is a joined table.