Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
views_ui.module
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
30 Jul 2014 at 16:02 UTC
Updated:
13 Mar 2015 at 12:54 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
longwaveBoth those paths only get executed if ViewExecutable::setDisplay() returns FALSE, which seems like a rare, perhaps impossible, occurrence in these cases. The other calls to setDisplay() in \Drupal\views_ui\Form\Ajax\* don't bother checking the return value and just assume it succeeded.
Comment #2
tim.plunkettComment #3
longwaveNot sure how to test this.
Comment #4
dawehnerCan we maybe still throw some exception, or call a drupal_set_message with that old message?
This is not entirely true. Views always tries to assume that a display might not be available, especially in the UI. You still need some basic way to be able to at least remove the broken display.
Comment #5
longwaveIn this case these forms are child panes of the main Views edit UI (aggregation settings and filter reordering respectively) where a valid display will always be present; I don't see a way of triggering these forms from an invalid display. As far as I can tell ViewEditForm will always be displayed first with the same display, and that does check the result of setDisplay() and bails out early if it is invalid. There are five other Ajax form panes that already do not bother checking the result of setDisplay().
Comment #6
olli commentedHere's an alternative that shows a text "Invalid display id foo" but I agree with #5, can't find a way how to trigger this error via UI (except visiting admin/structure/views/nojs/rearrange-filter/content/foo).
Comment #7
longwaveFor consistency if we are to add the message as in #5 we should do it across all the Ajax form classes.
Comment #8
olli commentedHere's #7.
Comment #9
dawehnerWell, this is not the point. The problem is that you should be able to somehow fix in the UI it if it fails to initialize the display.
Still i think it could be testable. ALter the config file manually ... and check whether you can still remove it via the UI.
Comment #10
Jalandhar commentedPatch #8 needs to be updated.
Comment #11
Jalandhar commentedHere is the updated patch.
Comment #12
tim.plunkettThanks! This still needs tests.
Comment #13
imiksuLet's see if I come up with an test.
Comment #14
imiksuI found this very challenging to find a way to test this (as raised in #5 and #6) and not sure where to put this test or should I create new test case completely, but hopefully this takes this issue further.
Attached new patch with test example, interdiff (the test only) and a patch having the patch only without the patch above (to test whether it passes/fails as should.
Comment #15
imiksuComment #17
joelpittetThat looks like a reasonable place for the test but you could also put it in a new test function. Anyways I'll leave this to someone else who knows if it's enough tests.
Comment #18
jhedstromPutting the test in a new method would trigger an entire install since this extends WebTestBase.
I think this is RTBC. I've added a beta phase evaluation to the issue summary.
Comment #19
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed ca35085 and pushed to 8.0.x. Thanks!