Problem

If we ever want to make menu items fieldable (per menu), moving them between menus is going to be a huge pain. Read more at http://drupal.org/node/1966298#comment-7458390

Proposal

Remove this feature as per suggestion from @effulgentsia and @catch.

Dependencies

Followup to #1966298: Introduce menu link bundles per menus.

Comments

Gábor Hojtsy’s picture

Status:Active» Postponed
Berdir’s picture

Not sure that's viable.

Especially the automatically created menu links from modules are sometimes not re-creatable manually. E.g. in Private message, I have a menu link that displays the number of unread messages using a title callback. That link is by default in the user menu, which makes a lot of sense for the default theme but most themes don't really use that, so users frequently move that to other menus.

Preventing this would break that. If we internally delete and re-create menu links when you do that, possibly with a confirmation that says you will loose (some?) of your field content, then that would be different, but completely removing this option seems like a feature regression?

Gábor Hojtsy’s picture

I think if we clone the menu item and move it around, that also may have issues for other related data on the menu item. I don't know. Now they are entities, there might as well be entity relations to them somewhere. If we clone it, then it will have a new ID and break any of those relations.

andypost’s picture

Status:Postponed» Active

Bundles are in so back to active.
Also now we can solve #1882218: Remove static menu link creation for menus in menu_enable() and elsewhere

catch’s picture

I'm not sure I suggested this in the other issue... Default menu items from modules makes this more or less unworkable as berdir points out.

Gábor Hojtsy’s picture

All right, then. What do we do? The idea was if we do this step, we can safely allow fields on menu bundles later. Looks like if we cannot make menu move-around impossible ever, then any kinds of bundles will be in danger if they ever want to support fields in contrib(?).

effulgentsia’s picture

I think we should remove this from core despite #2. There can be a contrib module that provides this functionality. And there can be a contrib module that makes menu links fieldable. And then a site can choose which of these contrib modules to use. Or, people can work out a solution in contrib that allows for both to coexist cleanly.

But, if we retain this functionality in core, then we're effectively requiring a contrib module that wants to make menu links fieldable to deal with the (difficult) bundle change problem, rather than allowing sites to choose which feature they want more (moving menu links between menus or having them be fieldable).

effulgentsia’s picture

Especially the automatically created menu links from modules are sometimes not re-creatable manually. E.g. in Private message, I have a menu link that displays the number of unread messages using a title callback.

The presence of 'title callback' does not make the menu link impossible to recreate manually. 'title callback' is stored in {menu_router}, not in {menu_links}. There is code in _menu_item_localize() to call the title callback whenever the static link title matches the static route title. So, you can delete (or not delete) the original menu link, and then create a new one in the menu you want, and if you give it the same static title as the original one, then when the menu is rendered, the title callback will be used. I just confirmed this locally by enabling menu_test.module, and adding a link to my footer menu with the title "Example title" and the path "menu-title-test/case3". In the footer menu, it got rendered as "Example title - Case 3". If I give it a custom title though, (e.g., "Foo"), then that gets used instead of the title callback.

amateescu’s picture

... deal with the (difficult) bundle change problem ...

The implementation is actually really easy (or at least was in D7), the hard part is deciding what to do with stale field data :)

das-peter’s picture

the hard part is deciding what to do with stale field data :)

We don't want or better don't have the time to write the code, but the scenarios are already in our heads because that was definitely an important topic during the discussion about menu bundles.
Maybe we should document the problems with the stale data and provide theoretical solution(s).
Like a guideline "What to do with stale data if my module changes entity bundles" :D

amateescu’s picture

We don't want or better don't have the time to write the code ...

Yes we do! Didn't you hear that Dev Days is just around the corner with a full week sprint? :P

So, the question is simple: when an entity is saved and its bundle was changed, what to do with field data that is (potentially) not relevant for the new bundle? The answer: we don't (and can't) know for sure, so we ask the user :)

I imagine something like this:
- we add a new key to the entity info definition, 'allow_bundle_switching', defaulting to FALSE
- based on that key, we provide an extra field in the manage fields page
- after the Field UI cleanup from #2014821: Introduce form modes UI and their configuration entity, we can probably add settings form and settings summary capabilites to extra fields, so we present the question above as a field setting

Did I miss anything?

Gábor Hojtsy’s picture

@amateescu: that does not seem to answer what happens with the stale data for those who support bundle switching?

amateescu’s picture

I don't see what you mean. We ask that question to the user and we act based on its answer: either keep the stale field data or delete it.

Gábor Hojtsy’s picture

Oh, it did not occur to me you are proposing we keep unused (disconnected) field data around in the field tables if the user wishes so.

amateescu’s picture

Yep, that's what I propose :) The user might change his mind later and return an entity to its initial bundle, so the option to have the data still available seems like a valid use case.

Edit: moar grammar..

catch’s picture

Hmm thinking about that, what about cloning and discarding any data, then unpublishing the original entity instead of deleting.

So you end up with two copies, if you really want to get rid of the old one, you can just delete it. That way you end up with 'stale' data but only conceptually, nothing actually gets orphaned.

amateescu’s picture

That means the entity type has to be revisionable or we need to create a reference between them. Not sure about either option..

Gábor Hojtsy’s picture

Also the entity type needs a status to be "unpublised", which taxonomy terms don't have for example.

amateescu’s picture

I think I have another alternative for cleaning up field data.. it's probably too much for core at this point, but we can definitely create a contrib module that finds all this stale data and offer the option to remove it (in a batch operation, of course). And it can alter the settings form/summary for that "bundle swtich" extra field to add a link to its UI.

das-peter’s picture

But what happens if you decide to keep the data when switching the bundle,
then you create a new revision of the entity, later you decide to switch back to the original bundle?
Is the revisioning an issue, what happens when you access a revision that had another bundle?

Further what happens with the stale data if you delete the entity? Will the delete handler recognize the stale data and remove them too? I don't think the current delete handler checks every possible field of the entity type for stale data to delete.

We should collect all this scenarios and try to find conceptual answers, document them and then see what we can / need to realize in core.

andypost’s picture

As I mentioned in #4 about #1882218: Remove static menu link creation for menus in menu_enable() and elsewhere we should drop parent menu items (auto created for each menu) first
then we can have clean menu_item entities with MenuLink::moveTo($menu_name, MenuLink $parent = NULL)

catch’s picture

@Gabor, we could only offer the 'retain' option for entities with a status. I don't think that's an unreasonable thing to add to taxonomy terms fwiw.

@das-peter keeping an unpublished entity around side-steps this a bit. The two entities are going to be immediately detached, but it gives a return option at least and doesn't require any additional data handling.

effulgentsia’s picture

I still think we should remove the UI for moving menu items across menus from core. We don't have a UI for changing the type of a node. We don't have a UI for moving a taxonomy term to a different vocabulary. #8 answers #2. At this point in the D8 cycle, I think the ideas discussed from #10 on are more likely to be successful in contrib than in core. I won't obstruct it going in if it's what people really want to do, but I guess I just want to raise a warning that I think it's risky.

amateescu’s picture

The thing is that contrib already has it :) https://drupal.org/project/bundleswitcher

What I proposed on top of that is just the UI to let users (actually, site builders) choose what happens with their field data.