New assignment plugins not registered to features bundle

Yeah using that special interface sounds like the right thing to do. It allows you to keep stored configuration and those objects in sync.

So, I agree in theory about using EntityWithPluginCollectionInterface, but that's going to be a large refactor. The bug in this OP is major and prevents new plugins from being used, so I think some sort of workaround is needed while we work on the bigger refactor. I'm not comfortable holding up the beta version to wait for this refactor, but I'm also not comfortable releasing a beta without this issue fixed.

Will look into "quick fixes" for a bit today.

Here's a test that should fail, to illustrate the problem.

Oops, I included the fix. Ha! Let's try again.

Ok, and now with the fix.

Yeah, I punted on that because it's hard to fix. Currently, the only place the settings for each assignment are specified is in its form!

Here's how the patch fares now:

• It's ok not to set dummy weights in setEnabledAssignments(), because AssignmentConfigureForm calls setAssignmentWeights() immediately after.
• Many assignment config forms work even if the assignment has empty settings, but some don't.
• Assignment itself is slightly borked if certain assignments have empty settings.

Ok, trying again. This time assignment methods have a defaultSettings() method. The assigner then gets a method getAssignmentSettings($method_id, $bundle) which merges these defaults into the settings stored by the bundle. Then all the assignments and forms use $assigner->getAssignmentSettings instead of whatever they were using before.

Also added a web test for all of this.

Wow, nice work!

Looking over the patch it all seems pretty good. The only question I had was that in your test you still seem to be calling $this->defaultBundle()->getAssignmentSettings but in the rest of your code it seems you refactored this into $this->assigner->getAssignmentSettings. Did a refactor get missed in the test or is there a reason the test is still calling the bundle function that I'm not getting? Maybe because for the test we don't care about the default settings?

in your test you still seem to be calling $this->defaultBundle()->getAssignmentSettings but in the rest of your code it seems you refactored this into $this->assigner->getAssignmentSettings

It's partly because I want to test what's actually getting saved in the bundle.

But mostly because FeaturesAssigner doesn't cooperate well with web tests. It has a cache of $assigner->bundles, and these bundles never get re-loaded. So when we save the bundle using the web UI, the assigner in the tests never sees the changes. I don't see any way at all to ask it to reset, or to ask the specific bundles to reset. I could just add a reset() method, is there a better way?

While I'm at it, I'm not 100% sure I like putting getAssignmentSettings on the assigner, it feels like conceptually the wrong place for that method. It's only there because nobody else has access to the actual plugin instances, so there's nowhere else obvious for it to go. Any thoughts?

Should we read the default weight from the plugin's annotation?

Great idea!

Yeah, since the UI sets the AssignmentSettings within the Bundle, it still seems like getAssignmentSettings should be part of the bundle, but as you said, then it doesn't have any way to access the plugin instances.

Remember that the Assigner existed before we had bundles and just used a single set of global settings.

I'd have to look more at the code, so maybe this idea is crazy, but now that we have Bundles I wonder if we even need the Assigner or if we just call something like currentBundle->assign() to have it process the plugins. Then the Bundle itself might have a list of plugin instances with their settings.

This might be a big refactor, but something I'm going to contemplate. Nedjo might have more insight since he wrote the Assigner and it's very possible he'll bring up something that only the assigner can do and can't be done by the bundle itself.

Hmm, the bundle is just an entity, a collection of fields/properties. I don't particularly like when entities themselves have complex behaviour (methods). Is there even a sane way to inject access to the assignment manager into a bundle entity?

Before this patch we got the settings from the bundle and that seemed clean. But what you are doing now is trying to retrieve settings for a bundle that doesn't exist yet and try to call the "default settings" for the specific assignment plugin. Is that correct?

If so, maybe we can go back to having the settings just in the bundle and have the default settings be "static" properties of the specific plugin class, rather than using the plugin instance. Defaults shouldn't depend on the instance, just the class.

vasi and I ended up discussing this in the #drupal-features IRC room. vasi is looking into using some static methods or properties for this. The issue really came down to how to allow the Bundle to reference a specific assignment plugin, even if it just needed the class for static default properties. I think we worked it out and I look forward to seeing what vasi comes up with.

Ok, updated.

I really like how putting default_settings in the annotation worked out. I made the assignment plugins inherit from PluginBase, so they have access to their definitions. Now we no longer need self::METHOD_ID, they just know their own IDs.

It's also nice that now $bundle->getAssignmentSettings() just does the right thing, it feels like a good place for that method to live.

I'm less thrilled with the static FeaturesAssigner::getAssignmentMethod() worked out. It feels like a workaround to access getAssignmentMethodInstance(). It also means that FeaturesBundle is less standalone, it has to know about the assigner and the plugins. You can see how much more awkward the unit test became. Think this is good enough for now?

Wow, nice work! I agree with your assessment. I really like how the method ids are handled now like normal plugins.

But you are right about getAssignmentMethod(). Having to still get an instance still doesn't feel right. With the default settings as annotations of the plugin definitions it seems like we'd be able to call a core plugin method to get this data from the class instead of needing the instance.

I think the core Plugin system has a way to get the definitions. Let me play with this.

Here is an updated patch. The main change is that the Bundle can now fetch the default settings directly via code like this:

$manager = \Drupal::service('plugin.manager.features_assignment_method');
$definition = $manager->getDefinition($method_id);

Turns out we were really close. Instead of adding the static method to our assigner, the assignment plugin manager service itself has a getDefinition() function that can be used instead. This makes things pretty clean I think. No dependency of Bundles on the Assigner anymore.

Also, EclipseGc++ on helping me with this.

Looks like the FeaturesBundle PHPUnit test is failing. I probably removed too much from the previous test and definitely need help with this as I'm not really up-to-speed on all the "mock" stuff needed for this test. @vasi maybe back to you?

I'll take a look tomorrow.

Also, shouldn't the default settings that are now stored in the Annotations for each plugin replace the features.bundle.default.yml file? Right now the annotations don't seem to match those default config settings.

Ah right, the default bundle. Forgot about that.

This is related to the config schema #2392561: Add configuration schema. I don't really want to define the schema for each plugin's settings. For now the existing schema passes the schema inspector without defining the plugin settings. So maybe we are fine.

But, to keep this issue on track:

I think #25 is good except for the test needing work. We'll talk in IRC today and decide if we want to push this without the PHPUnit test and then add the test later.

OK, I think I've got the tests working again now. Lets try this patch.

Woot! OK, committed this to d2cbf0c. Now that the annotations for plugins are in I can work on getting the defaults set correctly and handle the config schema in the other issue.