Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
ViewsHandlerManager::getHandler() catches exceptions and creates a fallback plugin instance.
Proposed resolution
Let's provide this functionality for other modules.
Introduce a fallback plugin ID on the DefaultPluginManager which could be also used by some field module.
Remaining tasks
-
User interface changes
-
API changes
-
Comment | File | Size | Author |
---|---|---|---|
#84 | 1822048-fallback-plugins-84.patch | 5.88 KB | martin107 |
#80 | 1822048-fallback-plugins.patch | 6.81 KB | neclimdul |
#80 | 1822048-fallback-plugins.interdiff.txt | 10.22 KB | neclimdul |
#73 | plugin_fallback-1822048-73.patch | 12.03 KB | EclipseGc |
#70 | interdiff.txt | 4.66 KB | dawehner |
Comments
Comment #1
dawehnerHaving a real error message would really help but i'm a bit worried about something like the following usecase:
In previous versions the actual executed view on the livepage didn't failed but just made these specific things hidden but sure you should really take care about that. Let's assume you have some access checking, so the user could see content which never was supposed to be seen.
One question i have: should a view track internally what modules it needs to run fine?
Comment #2
tim.plunkettThe patch was meant to expose bugs, not a solution.
Comment #3
dawehnerSure, but my comment is still sort of valid if you plan
You will certainly run into the config get's imported even if required modules aren't enabled, issue.
Comment #5
xjmWe should additionally notify the user when a view is being edited what the handlers are, and possibly provide an entry on the admin/reports patch.
Also, in order to report what module the
brokenunavailable handlers were originally from, I think we still need to store the providing module information in the config file. (It should be possible to ship a default view that includes optional handling that only becomes active when, e.g., locale is enabled, and it should be possible to inform the user that locale is needed for that handling.) What gets stored instate()
is the status of which handlers are unavailable in which views.Comment #6
damiankloip CreditAttribution: damiankloip commentedThis is the issue I created with this in mind: #1825896: Add module owner to plugin data on handlers
Comment #7
xjmRelated: #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*
Comment #8
xjmstop-hiding-errors.patch queued for re-testing.
Comment #10
dawehnerOnce we have #1810480: Provide the plugin_id to support views metadata integration and the patch that adds the module name in, we have the required information
to move this issue forward, because then we can show the users which plugin they expect to be there.
Comment #11
tim.plunkettPostponing on #1810480: Provide the plugin_id to support views metadata integration
Comment #12
sunThe HTML formatting of #5 is key here, I think:
I'd think that a view should only be considered as broken, if it attempts to use a plugin that does not exist.
However, if the plugin is merely unavailable, because the providing module is not enabled, then the view should actually not be considered as broken.
So I'd think: broken != unavailable. Only in case of broken, it should blow up. If only unavailable (which can be easily and quickly determined at runtime), then it's not broken, just "limited."
Of course, that only applies to field/filter/argument plugins... If a display or style plugin is missing, then there's no way to omit those, I guess.
Comment #13
tim.plunkett"if it attempts to use a plugin that does not exist"
If you're excluding plugins from disabled modules from that definition, what's left?
Someone hand-writing a view YAML file with a typo?
If a module chooses to remove a plugin, it could update the YAML file with an upgrade hook, so we don't need to support that.
As I see it, unavailable == broken, no matter how you redefine them
If we want to treat filter handlers different from field/sorts, then that's fine, but executing a view with broken/unavailable filter could be a security risk.
Comment #14
dawehnerWell then the question is what do we define as broken, as currently our definition of broken is "unavailable".
"Broken" could be triggered by the plugin itself, even I'm not sure what this gives us.
The question is how broken should it be. One thing we could do is to fallback to the default behavior, which would work at least for the style. For display plugins you end up in a situation in which the actual view is never rendered, because display plugin take care about the place in which a view is rendered.
Comment #15
sunExactly. Something is "broken" when it tries to use something that cannot reasonably exist. If a plugin-providing module forgot to update all affected views — or hey, in case there's no sane facility to do so in the first place ;) and developers just simply did not write the 100k lines for a possibly required module update :P — then that is the definition of "broken." :)
[Oh, and in case a module got uninstalled, but Views did not implement
hook_modules_uninstalled()
to adjust existing views, because the module being uninstalled cannot do anything about that, then that's of course also a definition of "broken"... ;)]But if something is just unavailable right now, and if the view can technically be executed without the thing being available (security is one reason, inability to fall back to sane defaults is another), there's no reason to not execute the view.
The primary reasons for reaching this situation are caused by a modular system. As with everything else, there are things that are able to happily operate further upon losing functionality (e.g., disabling action.module), but there are also things that cannot (e.g., disabling system.module).
Comment #16
dawehnerSo how can we detect the difference between something does not exist, as the views config is not up to date,
and it does not exist because something wasn't installed yet? Maybe i'm thinking to simple but I don't see a difference,
as the plugin simply can't be found in both cases. If a module is not enabled we don't know the plugins of it.
On the actual page I think we should always fallback to a non-broken behaviour, though in the admin UI we should better find a way to distinct between the two ways.
Comment #17
yched CreditAttribution: yched commentedLurking with interest.
The case of "a runtime object configured tu use a plugin that is provided by a missing module" is something that all subsystems had to deal with "somehow" so far (and in the case of Field API, it's a major PITA).
With plugin API, this now appears in slightly more similar terms across subsystems.
There is most probably not one good answer, a missing/unknown plugin means various levels of "broken" depending on the subsytem and plugin type. For missing field formatters, for example, we silently switch back to the 'default_formatter'. For missing field types, OTOH, this means we just cannot do anything with corresponding values (load, edit, save, display them), and getting those fields out of the regular flow of operations, and back in if the field type gets "known again", adds lots of code & concepts complexity.
"Field types as plugins" is not done yet, but I was actually kind of wondering whether having a BrokenFieldType, like views has, might simplify things a bit - so, I'm curious why you guys consider removing the one you have.
And, agreed with the previous comments than "never heard of it / was provided by a module that is disabled right now / was provided by a module whose code has just vanished from the filesystem" are just several flavors of "unknown" that we cannot tell apart - and I'm not sure what the difference would actually be.
Comment #18
catchCross-linking #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed which aims to reduce the 6,000 potential variations of disappearance down at least slightly.
Comment #19
jibranAs per #11
Comment #20
tim.plunkettApparently this debug is now messing with the testbots.
Comment #21
alexpottCommitted #20 7cffb9f and pushed to 8.x. Thanks!
Leaving this issue open as I feel this is but a sticking plaster...
Comment #22
xjmComment #23
tim.plunkettI forgot we had tests for that! My apologies.
Comment #24
alexpottRan tests locally...
Committed #23 5de3941 and pushed to 8.x. Thanks!
Setting back to needs work as per #21,#22...
Comment #25
dawehner/me sighs
Comment #26
tim.plunkettMore targeted issue for optional. Feel free to close as a dupe, but I think we could solve that quickly.
#2016953: Indicate when an optional handler is missing, that it is not "broken"
Comment #27
tim.plunkettWe still have commented out code and tests to prevent SimpleTest from choking, we can't ship like this.
Comment #28
dawehnerThe main reason why we had to comment that out was that the config got somehow imported before the module handler get resetted.
Comment #28.0
dawehnerupdated issue summary
Comment #29
martin107 CreditAttribution: martin107 commentedComment #30
dawehnerJust put some of my ideas out.
Comment #31
yched CreditAttribution: yched commentedWidgets / formatters could use this if the logic was delegated to a getFallbackPluginId() method (they need more complex logic since the fallback widget depends on the field type)
Comment #32
martin107 CreditAttribution: martin107 commentedComment #33
dawehnerSure why not. This also creates a unit test.
Comment #34
drabik CreditAttribution: drabik commentedI could apply the patch, so no reroll is needed.
Comment #35
damiankloip CreditAttribution: damiankloip commentedThis is looking pretty cool.
How about we don't have this as a constructor param and just assign the property definition above to '' or just leave as NULL (prob better). Then extending classes can either override the property, or do something to it in their constructor?
Nitpick alarm: Remove?
Seems like a good idea, would not have thought of that! ++
Should we typehint array here in the method signature too?
@covers
Nice!
Do we need the NULL's? They are just the default anyway.
mmmm apples.
This doesn't get thrown as soon as it is created/stored here?
Should/could we test the configuration on the plugin too?
Comment #36
dawehnerThank you so much for it!
Comment #37
tim.plunkettThis feels strange. Couldn't this just be
if ($fallback_plugin_id = $this->getFallbackPluginId($plugin_id, $configuration)) {
?Comment #38
dawehnerSure! This is a great idea!
Comment #39
dawehnerUpdate the title.
Comment #40
dawehnerLet's be honest here ... seriously a month without any review?
Comment #41
damiankloip CreditAttribution: damiankloip commentedMaybe this should mention 'plugin instance'
'Returns the fallback plugin ID to be used'
We should also mention to return nothing if this method decides no fallback ID should be used for whatever reason.
Comment #42
dawehnerThank you for your review!
Comment #43
damiankloip CreditAttribution: damiankloip commentedI think this is ready.
Comment #44
XanoDo we have use cases for allowing the factory to throw exceptions, or can we just as well check for the existence of the requested plugin ID and use the fallback it the requested ID is not available. It's faster and results in cleaner code.
Comment #45
damiankloip CreditAttribution: damiankloip commentedSpoke to Xano about this on IRC:
- We want to keep it like this so we *can* catch any exception that maybe be thrown when an instance is trying to be created. This will not necessarily only be due to a missing definition.
- This is not the most common case, most plugins will be found, so we don't want to check the definition first every time. So this IMO is the cleaner and faster approach.
Comment #46
alexpottLooks like the issue summary needs an update since
Does not sound like the fallback mechanism implemented by the patch.
Plus it'd be nice to have at least one real world use case covered in the patch. Chatted with @dawehner in IRC and he said he was ok with converting views to use this is this patch.
Comment #47
dawehnerImplemented the change for views and updated the issue summary.
Comment #49
dawehner47: vdc-1822048-47.patch queued for re-testing.
Comment #51
yched CreditAttribution: yched commentedMinor nitpick : could we get the getter and the setter next to each other in the file ? :-)
Does this @todo need an issue link ? (it has "crazy" in it...)
[edit: oops, never mind, that's existing code being moved around]
Also, would be lovely if the custom fallback logic implemented in FormatterPluginManager / WidgetPluginManager's getInstance() could be replaced with an override of getFallbackPluginId(); If not, can also be a followup...
Comment #52
damiankloip CreditAttribution: damiankloip commentedHere is a reroll for now, didn't bother with an interdiff for moving the method order. I'll see how this gets on, then implement #51.2
Comment #54
damiankloip CreditAttribution: damiankloip commentedThe '_exception' property leaves a lot of crap from the exception trace in there, which then subsequently gets serialized, or tries to anyway...
Comment #56
dawehnerFixed the failure.
Comment #57
dawehnerAny feedback?
Comment #58
larowlanLove it, there's some tests in #2249303: Implement fallback plugin for Block plugins we could adapt here too, for when a block content UUID is non-existant.
Comment #59
dawehnerSure, here is hopefully all what is needed.
Comment #61
larowlanDon't we need a fall back plugin ID for this to work?
Comment #62
dawehnerLet's do that in a follow up.
Comment #63
martin107 CreditAttribution: martin107 commentedI wanted to see the changes, just posting what I found for reference.
Comment #66
dawehnerClean 3way reroll.
Anyone up for a review?
Comment #67
dawehner@chx suggested to convert the FilterFormatPluginManager as well.
Comment #68
tim.plunkettI'm definitely +1 on this.
Not needed.
Allow implementations to show the exception message.
Why is this public (outside of needing it in one test)? And if it needs to be, why not put it on an interface? And finally, might as well return $this.
This is awesome.
Looks accidental/unneeded.
Please document why we set the fallback ID back to NULL.
Comment #69
chx CreditAttribution: chx commentedAnd you are listening to him? What an idea.
PHP allows nesting of try-catch statements and I would definitely wrap the fallback
createInstance
in one with an empty catch -- noone really cares that the fallback failed if it fails rather we should log/show the original exception. This should be very rare but it's my duty to think of edge cases :)+ public function setFallbackPluginId($fallback_plugin_id) {
Why is this public? Why does this exist? What is the use case for this? I can see some demented plugin manager having logic in
getFallbackPluginId
and then what will this do? In every single case of the patchsetFallbackPluginId
can be replaced withprotected $fallbackPluginId = '...'
on the relevant plugin manager class. But if we must, can we please make this protected? Or can we just move it to the test manager likesetFactory
?Tests the plugin manager with a disabled module
That will be some test given that the disabled module functionality is removed from D8. I know it's not written in this patch so if it's out of scope please follow up.
Comment #70
dawehnerLet's see whether these changes actually still work.
Well, it is not accidental, but when I am in this file I can also fix the bug in there.
Comment #71
EclipseGc CreditAttribution: EclipseGc commentedOK, this is completely untested, however as an approach I wanted to propose this. dawehner and I discussed this at length in irc.
The crux of this is that we can do this generically for all PluginManager. Essentially we check to see if our manager implements the FallbackPluginManagerInterface. If it does, we know there's a method for handling instantiation of fallback plugins. That method then is 100% responsible for delivering a new instance for the current needs. This has the nice upshot of removing fallback/broken plugins from the discovery pattern. They can still remain in the same directories, but they don't need an annotation. I didn't add an interface for fallback classes, but this might make sense too. In the case of views handlers, there's a broken() method on them. I checked this in my patch. I actually expect this patch to fail, but it's more a "hey does this make sense".
Let me know.
Eclipse
Comment #73
EclipseGc CreditAttribution: EclipseGc commenteddawehner and I are discussing this further. He has issues with my factory method for fallback classes. I'm going to keep pushing forward on the patch for the moment just to make sure it works conceptually so we can weigh all our options.
Eclipse
Comment #74
dawehnerWhat about using that interface but by default use the factory to create that instance?
Comment #75
neclimdulSo, I think the reason EclipseGC hadn't done that is related to this code which I also had thoughts about.
As with the views plugins later, if we don't want it to show up, maybe a "hidden" plugin attribute would be more useful? The line between UI gets complicated here though because you'll need to filter results between the getDefinitions and your UI.
Obviously if we're only instantiating it manually as the patch does we don't need the annotation but if we want to use the factory which seems reasonable since it contains normal logic for this.
Comment #76
EclipseGc CreditAttribution: EclipseGc commentedI REALLY don't want the fallbacks to be discoverable. I think that's likely to cause issue. Likewise, a magic "hidden" key allows all sorts of stuff to be hidden. I'm not actually against that, but it feels like it's a whole OTHER thing. We can't do a magic 'fallback' key cause what if we have two? and on and on. So I went with a factory method to get our fallback. If we want to expand the FactoryInterface to have a createInstance() method that instantiates plugins and a public instantiateClass() method that is JUST the raw factory so that the class can be passed to raw instantiation logic, I'm not against it, but we can't effectively pass to createInstance() here since that could end in a loop (at least with my current logic) and I don't want fallbacks to be annotated.
Is my basic desire unreasonable here? Am I missing anything? I just sort of felt like expanding the FactoryInterface was unnecessary. It seems unlikely to most fallback classes will need anything fancy, and even if they do the manager is registered on the DIC so it's easy to get that stuff to the Manager. These are my basic thoughts at least.
Eclipse
Comment #77
andypostthere was related issue about widgets and and formatters, entity displays has own fragile fallback for this ones
Comment #78
yched CreditAttribution: yched commentedJust a note that fallbacks are not necessarily special, "hidden, do not discover plz" plugin implementation.
FormatterPluginManager::getInstance() falls back to the "defaul formatter" for the field type if the requested formatter is unknown or not applicable to the field type - and same for widgets.
(granted, this relies on a Field API specific construct where each field type declare a default formatter and widget, that are supposed to always exist - i.e they are provided by the same module that provides the field type)
Comment #79
neclimdulRight, I was sort of making the assumption that they might be. "Broken" in views for example wouldn't be something you'd want to show up.
I'll write up a patch for how I think we might do this.
Comment #80
neclimdulMaybe something like this.
Comment #81
dawehnerI really prefer that way, still having a method on the interface but not bypass the factory.
<3
Comment #82
EclipseGc CreditAttribution: EclipseGc commentedI've said this in IRC, I'll say it here again.
a.) I'm not going to fight this. If everyone not-me thinks this is the way forward, so be it... however
b.) By using this approach we are explicitly depending upon the very system we expect to fail under certain circumstances. We all acknowledge that any number of things could cause this sort of fallback behavior, and instead of having a reliable fallback, we're using discovery (a system that just failed if we get into this code) to find a fallback for us. Are the odds of it failing for the fallback low? of course. But someone could (for instance) alter the class that's used for the fallback, or unset it from the list or any number of other things. It just seems like a mistake to me.
yched,
Your specific example can probably be served rather easily by my proposed factory method. I'd be happy to explore that directly if we decide to more seriously consider my approach here.
@all,
I'll reiterate, I won't fight for this position any further if no one else has the same concerns.
Eclipse
Comment #83
dawehnermeh, this needs a newline :(
Comment #84
martin107 CreditAttribution: martin107 commentedPatch no longer applied - needed reroll, no conflicts just auto merging etc
So no interdiff :(
Also added newline.
Comment #86
alexpottCommitted 099e0fa and pushed to 8.0.x. Thanks!
I debated with @dawehener whether or not this should extend PluginManager interface - I was not convinced by the idea that this keeps the interfaces composable but happy to have this debated in a later issue if it comes to that.
Not needed use - same namespace. Removed on commit.
Comment #88
yched CreditAttribution: yched commentedCan we open a followup to make widget anf formatters managers leverage this ? (sorry, not easy from my phone)
Comment #89
larowlan>80 and seems inappropriate
Comment #90
larowlanRe-opened #2249303: Implement fallback plugin for Block plugins
Comment #91
larowlanFixed #89 in #2249303: Implement fallback plugin for Block plugins