Updated: Comment #N
Problem/Motivation
Foo module provides a plugin type, FooPlugin.
MyModule provides one FooPlugin.
During the install of MyModule, the FooPlugin is instantiated.
Expected:
The plugin is instantiated.
Actual:
Uncaught PHP Exception Drupal\Component\Plugin\Exception\PluginException: "The plugin (foo_plugin) did not specify an instance class." at /www/d8/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php
There are 3 known workarounds in core:
- Entity types have entity_info_cache_clear() called directly in ModuleHandler::install()
- Field types are cleared by forum_module_preinstall()
- Views has code just commented out in ViewsHandlerManager::getHandler()
It is also holding up the ActionManager conversion and new search config entity patch, see related issues.
Proposed resolution
Provide a new service tag that plugin managers can use. Any manager tagged will have "clearCachedDefintions()" called on it during module install.
This could also prove useful for a drush command or other places anyone wants to clear plugin manager caches.
Remaining tasks
Agree on proposal
Write code
Fix the 3 workarounds listed above.
User interface changes
N/A
API changes
No changes, but a new service tag is added.
Comment | File | Size | Author |
---|---|---|---|
#59 | interdiff.txt | 1.9 KB | tim.plunkett |
#59 | plugin-manager-2155635-59.patch | 10.42 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettClarified existing workarounds.
Comment #2
dawehnerWhat about something like this?
Comment #4
tim.plunkettBerdir suggested that this be added to the parent: default_plugin_manager definition.
We need a better tag name, but clear is better than reset.
We should add a call to \Drupal::service('plugin.cache_clearer')->clearCachedDefinitions(); :)
Comment #5
tim.plunkettTurns out the tags don't work with abstract/parents. Or at least it wouldn't install locally.
I've renamed the tag, and tried moving some more stuff around. Uploading 2 patches to see what I can move out of ModuleHandler::install().
Comment #8
tim.plunkettYay for finding bugs in core!
InstallUninstallTest and ModuleApiTest both blindly call module_uninstall on comment, which used to be fine, but is not fine now that comment provides fields.
Now because we're clearing the views plugin cache a bit more, field_views_data tries to look up the storage controller for comment_field right after comment is uninstalled. Whoops!
Adding field_module_preuninstall for now, but we'll need to fix that exception.
Also found some weirdness with language_list(), and had to remove the views fix for now.
Comment #10
dawehner@tim
Should we try to keep the changes in ModuleHandler at least for now as small as possible and iterate in follow ups?
Comment #11
tim.plunkettYes, we can move that call back. It won't help with the bug though...
I plan to work on this tonight, but if someone wants to jump in before then, feel free.
Comment #12
tim.plunkettI don't think forum module is removing its comment field properly. It tries to in forum_uninstall, but doesn't seem to take. Here are some hacks to show how this works.
I don't know why this is only a problem with comment...
I also removed the change to ModuleHandler, that didn't make the interdiff.
Comment #14
aspilicious CreditAttribution: aspilicious commentedSmells like crappy copy paste dx. Isn't there a way to have this on by default?
Comment #15
pwolanin CreditAttribution: pwolanin commentedSince all plugins are provided by modules, and hence may have new plugins available when a module installed, I guess I also don't see how to make this simple unless each module needs to declare somehow which plugins it provides?
Comment #16
tim.plunkettPlugins can also be provided by Drupal\Core and Drupal\Component.
Comment #17
sunInstead of throwing an exception here, can't we move the CachedDiscoveryInterface condition into the clearCachedDefinitions() loop?
Can't we simply grep all defined services for the prefix "plugin.manager." in this compiler pass, instead of manually tagging them?
Combined with aforementioned suggestion, that would simplify things and improve DX.
Looks like a ::getDefinitionsByProvider() method would be handy?
IIRC, there is an existing separate issue that intends to make destruction of all module data part of the module uninstallation process (through the existing confirmation form), which should eliminate the need for having to manually delete these comment body field instances in tests?
I've the impression that this code should rather be part of the ModuleHandler::install()?
The reason for why a hook_modules_uninstalled() implementation is not required here is that ModuleHandler::uninstall() calls into drupal_flush_all_caches() already (which is undesired/unwanted on its own).
Both the install() and uninstall() operations should clear cached plugin manager definitions automatically at the right point in time.
(FWIW, I did not understand the @todo, as it talks about priming a storage, whereas the code here is about pruning/flushing a primed storage → why is the condition necessary?)
Comment #18
Berdir1. No because that method is part of that interface, if the plugin manager doesn't implement it, so we can't call it. Not sure if the exception is necessary or if we want to silently ignore it.
2. The entity manager is just entity_manager now, without the prefix.
5. The objectify the language system issue might fix this, as the core language manager is going to be very simple and return a fixed set of languages (english + special languages like not specified).
Comment #19
tim.plunkettI don't have really strong opinions on *how* we solve this, just that we need to. So unassigning for myself.
Comment #20
dawehnerI am quite a fan of telling people when they are doing something wrong, which would here happen directly in the container rebuild and not on runtime as the clearCachedDefinitions loop and yeah
also berdir's point is quite good.
Sadly we really cannot load all the services and check there interface directly. We could auto tag every service with "plugin.manager" it front and require it for the other ones?
Not sure whether there are really many usecase for this kind of functionality on a generic plugin manager, though at least for entities this is kind of helpful.
I kind of have the feeling that the module handler should know the least possible amount of information, like not care about the other subsystems like fields/entities/plugins/whatever.
Comment #22
BerdirWhile I do see your point, I don't really see how it's more OK if it's system.module. If the plugin component would be able to re-act to a module handler change yes, but this just seems to hide something important in a rather hidden location?
Comment #23
sunWhile that is a noble goal, fact (and sad truth) is that
ModuleHandler::install()
(and implicitly also::uninstall()
) calls intosystem_list_reset()
.That function clears the majority of all extension related caches. The remainder is handled by
$kernel->updateModules()
andentity_info_cache_clear()
, whereas the latter is currently guilty for flushing all entity info + field info caches, which are all plugins today.From a conceptual perspective, we introduced
hook_rebuild()
for this purpose (indrupal_flush_all_caches()
) two years ago. To make a clear differentiation between cache flushing vs. data rebuilding.Unfortunately, the introduction of
hook_rebuild()
in D8 went mostly unnoticed and wasn't advanced on yet. A whole bunch ofhook_cache_flush()
implementations (in particular, relating to entity/field data) should actually behook_rebuild()
implementations.Semantically, the "cached" plugin info/definitions are much rather state than cache, because they're statically defined in code and persistently cached (forever) until they need to be refreshed.
That differentiation applies to almost all of the former _info hooks, and even more so today, as we've started to take the differentiation much more seriously in D8, by disallowing context/request specific values in all declarations that were _info hooks previously.
Long story short:
drupal_flush_all_caches()
. (Unlike module_uninstall(), which still calls it, for whatever reason.)drupal_flush_all_caches()
also happened to rebuild all data structures. But in D7+, we started to bake a "base system knowledge" intomodule_install()
— i.e., it knows that entity info caches need to be rebuilt as well.entity_get_info()
& Co are defined in/includes/entity.inc
; i.e., the service is not provided by a module, and thus, there is no originating module that is responsible for performing the operation. (Gentle reminder: We want to get rid of System module ASAP.)Thoughts?
Comment #24
EclipseGc CreditAttribution: EclipseGc commentedIs there a reason we want to check the names instead of the tag? Caching has some implicit developer knowledge in order to get it working and while true, plugins have attempted to side step that as much as possible, I'd much rather just tag a service as needing to be part of the cache invalidation paradigm. This is one of the "hard" programming problems, I don't think asking people to put a tag on a service is really all that large of a DX request/regression.
Eclipse
Comment #25
dawehnerWell, there are cases of plugins which are certainly dynamic it the sense that they have to be recreated based on user actions. The block plugins provided by views is one example.
This also just worked on some cases, which wasn't cached. The plugin system in ctools for example (obviously) had a similar behavior, so I wonder whether that system ever had dynamic usecases.
I am glad that someone suggested that, but I wonder how we deal with the problem of having both events and hooks. We would have to decide an order for that or just get rid of hook_modules_preinstall
Comment #26
BerdirStraight untested re-roll, will check afterwards if we can improve things now.
Comment #29
BerdirOk, updated the code and calling the clearer directly for now, replacing the entity_info_clear_cache() call, otherwise we do that twice.
Comment #31
sunIt looks like most of the failures are because the
field.info
service defined by field.module is no longer cleared?Can we make the
FieldInfo
class implement theCachedDiscoveryInterface
, simply rename theflush()
method intoclearCachedDefinitions()
, and tag the service?PS: Closely related, partially blocked on this: #2201785: Remove drupal_flush_all_caches() from installer
Comment #32
BerdirOk, forum needs to delete the comment body field too. This is a bit weird :(
Comment #33
sunCreated #2202477: Make FieldInfo implement a new CachedStateInterface
Comment #35
sunSeriously?
And… some people are actively shooting for a beta...?
(Disregard the example, it's just a testament of how large the real distance between HEAD and a hypothetical beta actually is.)
Comment #36
tim.plunkettI added that as a placeholder because I don't know what we should do in that case. This patch has never passed tests, and was in no danger of being committed to core.
If you would like to help work toward a beta and resolve this issue, please feel free.
Comment #37
Berdir@sun: This is not related to FieldInfo, those caches are only about actual existing fields and the caches are transparently cleared when there are changes. This is about field types and fields that reference field types that no longer exist. Which is no longer supporter with the removal of the active flag.
This should fix those test fails.
- Rewritten FieldInfoTest::testInstanceDisabledEntityType() to use entity_test, I'm a bit confused why this is even supported, maybe #2198429: Make deleted fields work with config synch will remove it if it will happen.
- Re-added the manual deletion to CommentFieldsTest but I think what should happen here is that comment removes it's bundles in uninstall which will also remove all fields. We could even standardize this now.
- The installer is an ugly one. clearCachedDefinitions() calls out to the language manager. In case of locale installation, because rebuild the container before we install the module/schema call preinstall (why?!), the locale translation service already exists which then tries to translate the default language labels but the schema has not yet been installed. The order of calls in install() looks very random to me, I think we should d something like this: call preinstall, install schema, clear caches, call enable, rebuild container? As a quickfix, clearing the plugin caches after installing the schema makes the test pass.
Comment #39
BerdirPatch passed, testbot was just too lazy to update the issue status.
Comment #40
BerdirInstead of having to manually clean up the fields, can we do something similar to entity_module_preuninstall() and drop all fields that belong to a field type of the module that is being uninstalled?
Comment #41
tim.plunkettThis is a nice enough addition, but it's not used and I don't think its in scope...
I think this name is fine, but considering the interface we check and the method we call,
CachedDiscoveryClearer
might be a better name. And then we don't have need for the PluginManagerInterface typehint...I used this to find tests that didn't clean up fields properly, so this whole hook can be remove AFAICS?
Do we want to tag all of these even though they match the 'plugin.manager.' pattern? It doesn't seem like other services were updated
Comment #42
Berdir1. Yeah, removed.
2. Renamed, let's see if I got all references right. I did not rename the compiler pass as that still has plugin manager specific assumptions. I also didn't rename the service name, I think that one still works, it's still a part of the plugin system.
3. Yes, removed. Maybe a follow-up to discuss to automatically remove such fields? We already automatically delete bundles, which results in deleting fields attached to these bundles. So doing it for field types should complete the picture there. And there might be an issue with comment, as it should probably re-act on field deletion and delete it's own corresponding bundles? If you look at comment_field_instance_config_delete(), it deletes the content (this should probably happen as part of field data deletion, as this does not scale), but not bundles and once the field instance is gone, the bundles silently vanish and are not deleted. I think that's why most fails that we had here are related to the comment body field. Or we do it here, then we can drop the manual deletions.
4. Yeah, I think that depends on whether we want to do the magic name thing or not, I don't think there was an agreement there. If we do, then we can remove it.
Comment #43
BerdirOh, I found the problem, there was a delete call in comment_field_config_delete(), but it used the wrong name for the bundle. This fixes the CommentFieldsTest without the need to manually delete the field.
Comment #44
tim.plunkettI think this is enforced by PHP :)
Nice find with that last field deletion.
Comment #45
BerdirYeah, my brain does not work reliably before 9am :)
Comment #46
larowlanGood catch, berdir++
Comment #47
Berdir45: plugin-managers-2155635-45.patch queued for re-testing.
Comment #49
BerdirRe-roll.
Comment #50
Berdir49: plugin-managers-2155635-49.patch queued for re-testing.
Comment #52
BerdirRe-roll, lots of conflicts in views.services.inc, so here's a patch without the explicit tags there for now.
Comment #53
dawehnerGiven how many potential plugin managers we have I would vote to make them loaded lazy here, which means that we store the service names and just load the ones which are already instanciated.
Comment #54
BerdirThat won't help with clearing persistent caches?
I was thinking about a flag to avoid clearing persistent caches more than once unless they have been rebuilt again, but we rebuild the container anyway, so that will drop those objects?
Comment #55
sunAside from the remark below, the latest patch looks good to me - would be great to move forward here.
Won't that delete the entire comment body field wherever it is used...?
When uninstalling Forum module, comments may still be enabled on my blog and elsewhere?
Comment #56
BerdirYes, that is wrong. Might not even be needed anymore now, with the field instance deletion above, worth a try.
Comment #57
BerdirLet's see if that is now handled through the bundle deletion...
Comment #58
sunYay, thanks!
This is a blocker for #2201785: Remove drupal_flush_all_caches() from installer, which will be a major performance improvement for the installer + core test suite, so let's get this in ASAP :-)
Comment #59
tim.plunkettAwesome work @Berdir, thanks for finishing this off!
I went to write a draft change record for the API addition, and noticed some weird docblocks. No functional changes, still RTBC.
Draft change record coming soon, will appear in the sidebar.
Comment #60
tim.plunkettHmm, I just noticed that if a plugin manager service ID starts with
plugin.manager.
, it is assumed to implement CachedDiscoveryInterface.There is no way to use that service ID pattern without implementing that interface, because of CachedDiscoveryClearer::addCachedDiscovery().
Now, both PluginManagerBase and DefaultPluginManager implement this, which covers every manager in core. Just thought I'd point out the assumption.
Added https://drupal.org/node/2218621
Comment #61
catchHmm #60 is interesting but I think it can be a follow-up, patch looks like a very nice improvement otherwise.
Committed/pushed to 8.x, thanks!
Comment #63
andypostFiled follow-up to remove @todo that left #2324945: clean-up @todo after 2155635