Fix entity serialization in edit form

Ugh. I guess maybe we need to put the serialization back in?

OK... I don't think we're intentionally trying to serialize, is this a Core bug? I think running into this was why I put serialization into the class in the first place? Anyway, if you know how to fix it, great!

OK. let's step back a second...

Before #2920841: Fix serialization of help topics, we had a test that instantiated, then serialized/unserialized a HelpTopic entity. And the HelpTopic entity implemented Serializable, and had serialize/unserialize methods.

In working on that issue, I removed the interface and methods. The test failed with an error very similar to the above, but we decided on that other issue that we didn't need HelpTopic to be serializable, so I removed the test and we committed that patch.

The original reason for adding serialization was because somewhere in the Form API, during the Ajax "Add item" stuff, the entity got serialized and it didn't work; it failed with an error very similar to the above. I thought that the need for serialization had gone away because even after #2920841: Fix serialization of help topics the Add Item button works now.

But maybe we need to reverse the patch from #2920841: Fix serialization of help topics? HelpTopic entities definitely don't serialize as they are now.

Thoughts?

See also #2688730: Add new item button is not working due to serialization problem, which was the original issue when the Add Item button stopped working due to some change in Drupal Core.

I have the same PHP notice showing up in my dblog, when I hit the "Add new" button on the form.

When I look at the stack trace... it seems to come from FormCache::setCache(), which calls DatabaseStorageExpirable::setWithExpire(), passing in $form, and that ends up calling serialize($form).

So it looks like if you have a config entity form that uses Ajax, it is going to get run through the form cache, and the form will get serialized, including the config entity. So, I think the HelpTopic config entity does actually need to be serializable, and we should just reverse what we did on #2920841: Fix serialization of help topics and make it serializable again.

Thoughts?

Here is a patch that puts the serialization stuff back in HelpTopic, plus the serialization test. this also includes the patch in #6, which seems sensible to do in any case. I didn't make an interdiff, as all but the few lines in #6 is new patch.

When applied locally, the PHP notices about serialize/sleep are no longer getting into my dblog. I'm running the tests locally... any thoughts?

Serialize test failed. I forgot a use statement at the top of HelpTopicTest.

use Drupal\config_help\Entity\HelpTopic;

That's the interdiff. Tests all pass locally with this patch.

I'll take a look tomorrow at this :)

So the problem here was our usage of plugin collections in \Drupal\config_help\Entity\HelpTopic. The point of EntityWithPluginCollectionInterface is to be used when a config entity type actually stores some plugin configuration in one of its properties, but our entity type does not store any configuration for the \Drupal\config_help\Plugin\HelpSection\ConfigHelpSection plugin, so we simply need to remove the dead code from \Drupal\config_help\Entity\HelpTopic.

With this patch I don't see serialization notices in watchdog when clicking the "Add new" button anymore ;)

Edit: and all tests pass locally.

Um... The plugin collection that is referenced is for TextSection plugins, not HelpPageSection plugins.

The reason we have the plugin collection is that the body field on HelpTopic is a collection of TextSections. So I think we do actually need the HelpTopic to be an EntityWithPluginCollectionInterface, and I don't think it is correct to remove it. Having the interface there should take care of the proper cache tags that are needed on the HelpTopic entity because of its dependence on TextSection plugins, right?

This is very similar to what other Core config entities that implement EntityWithPluginCollectionInterfacee do. Here's a list of classes that implement the interface:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Examples:
ImageStyle's effects field is a collection of Effects
FilterFormat's filters field is a collection of Filters
Block's settings and visibility fields are both plugin collections
etc.

---
Regarding #6, OK we can make a separate issue for that.

Um... The plugin collection that is referenced is for TextSection plugins, not HelpPageSection plugins.

Ahh, that's right. Not sure why I got confused with the other plugin provided by this module.

In that case, this patch should do it :)

Ah, I see that is what those other EntityWithPluginCollection classes do... but we'll need to do more than that. The Add More button doesn't work at all with that patch. I get this in the dblog when the add button fails:

Drupal\Component\Plugin\Exception\PluginNotFoundException: Plugin ID '0' was not found. in Drupal\Core\Plugin\DefaultLazyPluginCollection->initializePlugin() (line 79 of  (my directory)/core/lib/Drupal/Core/Plugin/DefaultLazyPluginCollection.php).

So. Really, the body is only currently sort of a collection of TextPlugin items... it needs to actually become a real collection probably... The TextSectionPluginManager may also need some work...

Yes, some more work is likely needed, I posted the patch from #17 without any kind of testing.

But it really just shows why we had that notice in the dblog, and it was because the return value of \Drupal\Core\Plugin\ObjectWithPluginCollectionInterface::getPluginCollections() needs to be keyed by a string which represents a property of the config entity.

OK, I think this works!

What it does:
a) Changes the body schema for the config entity to use 'id' instead of 'type' for the TextSection plugin ID
b) That allows us to use the default plugin collection constructor for the TextSectionPluginCollection

This makes everything work in the UI without errors in DB log. You have to uninstall/reinstall the module though.

I am running the tests locally and they have failures so this isn't final patch yet but you can see where it is headed.

Ah, I forgot to update the configs in the test module for the new config schema. I think this may work... running tests locally again...

Incidentally this takes care of #6 too, because we don't pass the HelpTopic into the TextSectionPluginCollection constructor at all now, just the body array. Which will make that plugin collection more generic as well.

#21 looks great! If it passes all the tests, consider it RTBC from my POV :)

Also filed #2923139: Move some functionality into the TextSectionPluginCollection... not quite but somewhat related to this issue.

Anyway, the tests pass locally, so @amateescu says this is RTBC, and I went ahead and committed it. Will update the Core patch shortly as well, and make sure amateescu gets credit there if not previously credited.

Thanks for the suggestion, review, help, etc.!