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.
views_ajax_render() is called from 2 places in the code but has never existed (as far as I can tell) in Drupal core. These calls are in commit that created the 7.x-3.x views branch a626abb24faa51ac140f73779a89e1ad7d5ae716 so this bug is also present in that version of views.
The calls are in Drupal\views_ui\Form\Ajax\ConfigHandlerGroup and Drupal\views_ui\Form\Ajax\RearrangeFilter.
This might be dead code since there are no issues in the views queue either.
Beta phase evaluation
Issue category | Bug because code is referencing non-existent functions |
---|---|
Unfrozen changes | Unfrozen because it only changes code to reduce fragility |
Comment | File | Size | Author |
---|---|---|---|
#14 | interdiff.txt | 885 bytes | iMiksu |
#14 | 2312647-14-FAIL.patch | 885 bytes | iMiksu |
#14 | 2312647-14.patch | 6.37 KB | iMiksu |
#11 | 2312647-11.patch | 5.5 KB | Jalandhar |
#8 | 2312647-7.patch | 5.49 KB | olli |
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 CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: Jalandhar commentedPatch #8 needs to be updated.
Comment #11
Jalandhar CreditAttribution: 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!