The DefaultFactory plugin factory class will throw exceptions when a. a plugin id doesn't exist in the defintions or b. the class cannot be found but the defintion is there.
Currently, if for any reason, the views display plugin is either invalid or unfound, we get an uncaught exception. So let's fail in a slightly nicer way (like field plugins etc... do in views) and catch the plugin exceptions instead.
Here is a patch and some simple tests for it.
Comment | File | Size | Author |
---|---|---|---|
#69 | 1879774-68.patch | 14.59 KB | effulgentsia |
#68 | 1879774-68.patch | 14.59 KB | damiankloip |
#63 | 1879774-63.patch | 14.58 KB | damiankloip |
#61 | Screen Shot 2013-05-21 at 16.48.22.png | 65.68 KB | damiankloip |
#58 | 1879774-58.patch | 14.57 KB | damiankloip |
Comments
Comment #1
dawehnerWhat about using debug()?
Afaik we use t() for assertText()
Comment #2
damiankloip CreditAttribution: damiankloip commentedThanks!
I changed the set message for a debug. We do use debug everywhere else in views for these types of things.
I am not sure about the assertText, currently a mixture of with and without t() is used. But I guess we might as well, then both are run through t() and it's consistent.
Comment #3
dawehnerAfaik the rule is easy: No-t for assertion messages, t() for everything else.
Comment #4
damiankloip CreditAttribution: damiankloip commentedSmall change, just added a variable to store the result of $e->getMessage().
Comment #5
damiankloip CreditAttribution: damiankloip commentedAnd a version with drupal_set_message, just in case. Someone else can decide which they want.
Comment #6
xjmDo we want to wait on this a bit while we discuss #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)?
Comment #7
dawehnerOn the other hand we do have
already in ViewExecutable::setDisplay()
Comment #8
xjmYeah, which I think is not the right way to handle it. ;)
Comment #9
damiankloip CreditAttribution: damiankloip commentedThat code in setDisplay would not really help anymore, because the plugin exception would already be thrown. So I don't think that code can actually handle what we're trying to do here.
If we don't want displays to break, we have to implement this here really. So the only question is do we go with the debug, or the drupal_set_message version? I think I vote for the latter.
Comment #10
xjmWell, part of the discussion I raised in #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies) is about doing the same thing consistently in the same circumstances in different modules, and about who should see what when. E.g., in this issue, what does "We don't want displays to break" mean? The goal is going to be different if we are loading the admin form versus rendering the view to an end user. I also think we should consider only displaying a message to users who have permission to administer Views.
Edit: I'd also rather add a report entry to the reports page, rather than logging messages to watchdog. But let's discuss in #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies).
Comment #11
dawehnerBut just looking at the current code of development: Having an uncatched exception laying around is wrong, at least from my perspective.
Even small progress is progress.
Rerolled against the usual changes of views.
Comment #12
dawehnerForgot the yml file.
Comment #13
tim.plunkettI think this should go in to stop the bleeding, and we can improve it holistically in the linked meta.
Comment #15
dawehnerMhhh the message is not shown, as I guess something before takes care about it.
Comment #16
damiankloip CreditAttribution: damiankloip commentedYeah, I think maybe the default display plugin is being used or something. Maybe this changed in commmit
d0492226
, but not sure?Comment #17
dawehnerFrom my perspective the problem is something different:
If you save a view, views_invalidate_cache() is called that automatically forces a menu rebuild. Now the actual hook_menu entry does not exists at all anymore
and you don't have an invalid display. We might should test the code directly.
Comment #19
damiankloip CreditAttribution: damiankloip commentedYeah, that sounds totally right actually. So I think before we had troubles with menu rebuilds (in tests). I.e. now we dont have to call menu_router_rebuild() in tests. I think we need to test this in the context of a page callback. To me that makes sense.
How about we circumvent the entity API to save the view, and do this directly in config, then the rebuild wont be called? Although, I think the fact that we now use the new routing system may give us different issues instead?
Comment #21
damiankloip CreditAttribution: damiankloip commentedSo this works as expected if you test this normally, but in the test environment it doesn't.
Comment #22
damiankloip CreditAttribution: damiankloip commentedSo we could make these changes? I'm not sure why but the reason the tests are failing is because a database exception is being caught but when it's decoded it fatals. (decodeException()).
Comment #23
dawehnerWhat is the advantage of using node here? It always feels wrong to use node in tests.
Comment #24
damiankloip CreditAttribution: damiankloip commentedThat's why. So it works with node, but not with out views_test_data table.
Comment #25
dawehnerRerolled the patch, though I still get the exception.
Comment #27
dawehnerYeah let's go with the route of damian and use node. These at least don't fail completely.
Comment #28
damiankloip CreditAttribution: damiankloip commented#22: 1879774-22.patch queued for re-testing.
Comment #29
damiankloip CreditAttribution: damiankloip commentedrerolled.
Comment #30
dawehnerThat's ready to go now: We have test coverage, so let's get this in.
Comment #31
xjm#29: 1879774-29.patch queued for re-testing.
Comment #33
tim.plunkett#29: 1879774-29.patch queued for re-testing.
Comment #34
damiankloip CreditAttribution: damiankloip commentedThis was already RTBC.
Comment #35
xjm@effulgentsia and I discussed this patch today, and then I talked about it with @tim.plunkett.
There are two things that happen when a display plugin exception is thrown:
#2 is really, really bad--your site is broken and you can't fix it. #1, however, could be the "correct" behavior in some cases, DX-wise. Let's hardcode this catch, message display, and watchdog behavior only for Views UI. For the rendered View, for now, let's let that exception through (or re-throw it if needed), without the error message or watchdog logging.
Eventually, what I think we should do is discuss a complete solution in #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies). Ideally, what would happen in Views is that we'd collect any and all exceptions together (not just displays'), and then at the end of execution, decide what to do with them based on either the user's configuration or module input. This could range anything from failing silently to hiding the view entirely and displaying an error message.
Edit: But for now, for purposes of development, we want to see these exceptions on render.
Comment #36
dawehnerRemoved the watchdog entry, just because it add a lot of entries.
I think we should make it clear why accessing the page still works, and test, that the display is not accessible, when the plugin fails here.
Comment #38
dawehnerNote: If you you aren't sure about 403/404 choose the one you guessed first.
Comment #39
xjmI'm much more comfortable with #38, especially with the test that ensures the invalid display is not accessible. Do we also want to add a similar tests for a block display to ensure that the block is not displayed on a page? Where in the render process does an invalid display plugin result in the view not being rendered normally?
Comment #40
damiankloip CreditAttribution: damiankloip commentedThere isn't much chance of anything getting rendered, as there would be no display plugin available to render it. So I think we are good. With pages being registered in hook menu, when the router is rebuilt, that display (executeHookMenu) is never called again, as it can;t find it. So really there is no way a view would be accessible, I don't think. This happens way early, when a new display instance is trying to be instantiated.
I would say this is good to go. Let's get this puppy in :) I think I'm ok to rtbc this....
Comment #41
xjmAgain, what about block displays?
Comment #42
xjmComment #43
damiankloip CreditAttribution: damiankloip commentedI'm not sure what you're expecting to happen with the invalid displays? The tests originally were just to point out that a message is displayed when we get an invalid type. If a display plugin is not found it can't register much or do anything because no display plugin will be loaded.
Comment #44
dawehnerWell, I think this sorts of make sense to test the behavior as well. We want to be sure that the behavior never changed?
Comment #45
xjmAgree with #44. If a block has an invalid display plugin, what do we want to happen?
If we want it to do the equivalent of what the page display is now doing, that would be: not return any block, and throw or not throw an exception. So a test should probably test that there is no block wrapper on the page for a block display that uses a broken display plugin, and that the exception is
rendered on the pagethrown or not depending on our expectation.Comment #46
damiankloip CreditAttribution: damiankloip commentedBut if I the plugin id is not valid, it doesn't really know it's even meant to be a block.
Comment #47
xjm@damiankloip, see the existing test for what I mean. Am I completely misunderstanding?
Comment #48
damiankloip CreditAttribution: damiankloip commentedYep, I originally wrote that test :)
To me, this issue was just about catching that exception. But it seems that ship has sailed, ha.
OK, let's fix this and add the block tests. If we really want to protect people, we will have to change a few other bits of code in the code path for executing a display I think..
I'll post a new patch shortly.
Comment #49
damiankloip CreditAttribution: damiankloip commentedok, here goes...
We have to make a few changes, So let's see how this fairs up.
Comment #50
dawehnerWon't that break the UI at some point? I would be super conservative here.
I'm wondering whether we should put that into the DisplayBlockTest and the code above into the DisplayPageTest and just reference from here via an @see ?
Yeah that's cool!
Don't hack core ;)
Comment #51
damiankloip CreditAttribution: damiankloip commentedJust a reroll for now.
The trouble is it's really difficult to know when rendering a block , for example that things have failed if it always returns the default display. It get tricky to know that that is not the display instance we wanted.
I will see how the reroll goes; then moving the block tests sounds like a good plan.
Comment #52
dawehnerThis seems to be better then having a failing UI on which nothing can be fixed at all :(
Comment #53
dawehnerSo what about somehow adding a flag whether we are in the UI or on the actual site?
Comment #54
damiankloip CreditAttribution: damiankloip commentedWe could add a property to the actual default display? like isFallback or something? Don;t take that name too seriously.
Comment #55
dawehnerWhatever doesn't break the UI is great.
Comment #56
damiankloip CreditAttribution: damiankloip commentedOK, let's make sure we can get an instance of a display before rendering the tab too. So any invalid display plugin types will not get a tab. Nice :)
Comment #58
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #59
dawehnerIt feels wrong to not use list() here.
I have seen it working on damians laptop, so a broken views config does not break the views UI, but maybe a screenshot of the behavior would be cool.
Comment #60
dawehnerForgot the rtbc
Comment #61
damiankloip CreditAttribution: damiankloip commentedGOod plan, here is a screen grab when (for the sake of argument, the page plugin name was changed to 'pag'). You can see the message from the exception being set but also the page display tab is not available. So for instance, this would work just the same for disabled moduled. E.g. if you created a rest export display on a view but then disabled the rest module.
Comment #62
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #63
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #64
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #65
longwavehttp://drupalcode.org/project/drupal.git/commitdiff/db2df2d contains the patch from #2002506: Rearranging fields and sorts causes existing items to be removed, not this one :)
Comment #66
Dries CreditAttribution: Dries commentedI rerolled the other patch. Unfortunatly, the patch in #63 no longer applies. Needs a re-roll.
Comment #67
xjm#63: 1879774-63.patch queued for re-testing.
Comment #68
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedBot seems stuck on #68. Reuploading to see if that helps.
Comment #70
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #71
oadaeh CreditAttribution: oadaeh commentedI'm not 100% sure this is related to this issue, but I think it is, so I'm posting it here. Feel free to direct me elsewhere, if I'm wrong.
I did a brand new install with D8 code downloaded less than an hour ago (which means the committed changes from this issue are in it) into a new database, and then performed the following steps:
I get a WSOD. (When I did this last night, I was also getting an error message, but I'm not getting it now, so I can't post it).
I get this from Apache:
PHP Fatal error: Call to a member function access() on a non-object in /.../core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php on line 56
If I delete the associated .yml file, everything is fine again.
Comment #72
damiankloip CreditAttribution: damiankloip commentedHi, Thanks for reporting! It's good that people are actually testing this stuff :)
I guess this is related, but this issue was specifically for dealing with displays that were not available. What you have just pointed out is definitely a problem though. If you also disable a view, you should see some problems too.
Let's create a new issue for that.