Move the {shortcut_set_users} table to a proper field on the user entity

Oops, thanks!

Also assigning to me for now.

Here we go. This was ridiculously easy (but for the test failures...) because we're at a point where Entity API Just Works© (yay!).

This should be green already. Note that I had quite some fails at first and even found an separate core bug (will open issue in a minute) so that does not mean that test coverage is insufficient.

I snuck in a few changes that could be left out, but make sense to do as part of this:
- Add AccountInterface typehinting to some ShortcutStorageInterface methods. This is not an API change it's only documenting what happens anyway. What we really want is UserInterface but that would be an API change, which is why I'm doing that magic loading dance.
- Fix some test assertions to be more debuggable.

Next step is ripping out the custom form stuff for assigning a shortcut set to a user. Current plan is to introduce a new form mode for that, we'll see how that goes.

Hehe, good thing we have the testbot. I only ran the shortcut tests locally, and then obviously my two favorite tests in the world get jealous and want to spend some alone time with me. Fun!

Let's see if this is green.

ModuleInstallUninstallTest revealed that the schema updates were not being applied on install and uninstall of the module. I thought that was a weirdness of the test, which is why I added it into some of the tests above, but it was in fact not working. In my local testing I just used Standard profile which does that manually in standard_install() (which is super weird, by the way!).

This fixes that, so could be green.

It reveals another problem, however: With this patch shortcut module cannot be uninstalled as soon as any user has ever submitted the switch shortcut sets form. That's kind of a good thing, as that means the entity API correctly detects that it's going to delete unrecoverable data and it refuses to do that (which is nice), but it's not really an acceptable situation. Not sure what the fix for that is exactly, but we somehow need to handle the default shortcut set differently, I think.

Thanks!

One thing I just don't get, when does someone uses a base field, and when does someone uses a configurable field? Do you always uses base fields if you are in code?

Yes, basically that is it. As soon as there is business logic (i.e. code) that requires that field it makes sense to use a base field.

Also: isn't it just way more fancy that writing/exporting a bunch of YAML files? ;-)

This is much nicer!

I agree. :-) I think with this ShortcutSetStorageInterface is basically obsolete, I did not add @todos yet to remove the relevant methods (in 9.x). At the very least assignUser, unassignUser and getAssignedToUser are rather silly with the patch.

Thanks for the change record @waringnick! I added some example code to it.

I think the patch is in fact ready to go. Attached patch just adds some @todos to the methods mentioned in #10.

I spent a lot of time thinking about and hacking around the "problem" described in #8 until I realized that I am an incredible buffoon and there is absolutely no problem at all. Shortcut module cannot be uninstalled if there are shortcut entities (with and without the patch), this has nothing at all to do with the user->shortcut_set assignments.

I also had a stab at making the switch shortcut set form a proper (user) entity form that uses widgets for the shortcut set reference fields. Even though that would be the correct implementation in theory there are several nitty gritty details that made me revert all those nice local commits in the end:

1. The nice setDisplayOptions() only apply to the default form mode. Any additional form modes have to specified as an entity form display entity in YAML which inevitably makes them appear in the UI, which might not be what we want.
2. It is possible to use the "check boxes/radio buttons" widget for entity references (even though strangely only with Entity reference module installed, even though all the classes are in Drupal\Core) but that does not have the auto_create feature that the current form has, where you can create a new shortcut set on the fly. So that would have to be added somehow
3. There is a subtle difference between no shortcut set assignment at all for a user and an assignment to the Default shortcut set, i.e. there is a slight difference between selecting Default and N/A. The current UI does not ofer the latter choice (for UX reasons) which is not someting that the "check boxes/radio buttons" widget allows
4. It is not easily possible to create a shortcut set or even entity specific widget which subclasses OptionsWidgetBase because that hardcodes the plugin IDs of its subclasses in getOptions(). While that is a legitimate bug in its own right, it would be out of scope to fix here.

In summary, in order to make the form an entity form we would have to code around too many shortcomings of our APIs to even remotely justify the benefit.

Played around with it again and everything seems to work.

Hmm... apparently I got our standards for documenting @deprecated code wrong. This one should be better.

Sorry for the noise.

Interdiff was correct, here's the patch I should have posted in #14.

Re #18: That hasn't been a criterion before (e.g. user_roles and other things), which is why I pursued this in the first place. You are of course correct, though.

Re #19: Will test again. I think I verified that this worked, but will re-verify. Setting to "needs review" for that.

This does in fact not work correctly, thanks @Berdir. Will post updates later.

So at the time hook_uninstall() gets called the respective module (Shortcut in this case) is still registered as installed and still takes part in hooks, etc. That's why the call to EntityDefinitionUpdateManager::applyUpdate() doesn't work: Because at the time of calling the field storage definition for the shortcut_set field still exists.

What does work, though, is removing the field storage definition "manually" á la:

  $entity_manager = \Drupal::service('entity.manager');
  $storage_definition = $entity_manager->getFieldStorageDefinitions('user')['shortcut_set'];
  $entity_manager->onFieldStorageDefinitionDelete($storage_definition);

There is no field purging involved. The uninstallation is only possible if no data is stored in the field in the first place and the onFieldStorageDefinitionDelete() simply drops the column alltogether, which is exactly what we want.

The problem that crops up then, is that when Shortcut module is uninstalled the default shortcut set along with any other shortcut sets, which aren't used anymore is deleted. So ShortcutSetStorage::deleteAssignedUsers() is called which queries the shortcut_set field on users, which doesn't exist anymore at that point.

I haven't found a solution to that yet, so no new patch.

Thanks again @Berdir, for pointing this out!!!

Not necessarily advocating this as a good idea, but let's see if this is green at least.

(Note that I need to use getLastInstalledFieldStorageDefinition() because the storage definition does not exist at that point anymore. Seems awkward at first, but I think (?) it's kind of correct.)

As @Berdir pointed out, this points to a more general problem of uninstalling modules which provide field storage definitions for other entity types. Trying to uninstall Content Translation will be equally borked, if not more.

Actually, writing #23 I realized that makes EntityDefinitionUpdateManager::applyUpdates() which does make me slightly less uneasy about this approach.

Maybe it was right there all along. If this is green, I want a cookie.

I meant to revert that post_uninstall stuff before, which this does. Also took a stab at fixing KernelTestBase. I have a strange batch error locally, but am hoping that's not reproducible.

Sorry

(interdiff is correct...)

Ok, if this still doesn't apply, I seriously have to consider just quitting IT and becoming a construction worker or something...

Awesome, so that seems to have worked.

I think these latest changes revealed on a major bug I would title "Modules that provide dynamic field storages cannot be uninstalled reliably" that should be split and fixed separately, but @Berdir rightly pointed out that @plach should have a look at the patch so I wanted to wait for that before doing any issue queue foo.

Assigning for that purpose and will try to reach out in IRC.

We can save some characters after #2449709: ContentEntityBase::set() does not respect its interface went in.

Hmm... random?

If it's just about removing lingering base field schema, we have #2282119: Make the Entity Field API handle field purging for that.

Yes, that was the reason I started all this in the first place. I will move the relevant code as proposal to the issue(s) you linked and re-focus the patch here. Should be a lot less controversial then. Thanks!