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

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because code is referencing non-existent functions
Unfrozen changes Unfrozen because it only changes code to reduce fragility
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave’s picture

Both 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.

tim.plunkett’s picture

Version: 8.x-dev » 8.0.x-dev
longwave’s picture

Status: Active » Needs review
FileSize
1.64 KB

Not sure how to test this.

dawehner’s picture

+++ b/core/modules/views_ui/src/Form/Ajax/RearrangeFilter.php
@@ -41,9 +41,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-    if (!$executable->setDisplay($display_id)) {
-      views_ajax_render($this->t('Invalid display id @display', array('@display' => $display_id)));
-    }
+    $executable->setDisplay($display_id);

Can we maybe still throw some exception, or call a drupal_set_message with that old message?

ViewExecutable::setDisplay() returns FALSE, which seems like a rare, perhaps impossible, occurrence in these cases

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.

longwave’s picture

In 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().

olli’s picture

FileSize
1.64 KB

Here'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).

longwave’s picture

For consistency if we are to add the message as in #5 we should do it across all the Ajax form classes.

olli’s picture

FileSize
5.49 KB

Here's #7.

dawehner’s picture

Here'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).

Well, 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.

Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch #8 needs to be updated.

Jalandhar’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.5 KB

Here is the updated patch.

tim.plunkett’s picture

Status: Needs review » Needs work

Thanks! This still needs tests.

iMiksu’s picture

Assigned: Unassigned » iMiksu

Let's see if I come up with an test.

iMiksu’s picture

Assigned: iMiksu » Unassigned
FileSize
6.37 KB
885 bytes
885 bytes

I 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.

iMiksu’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: 2312647-14-FAIL.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

That 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.

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Putting 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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed ca35085 on 8.0.x
    Issue #2312647 by iMiksu, olli, longwave, Jalandhar: views_ajax_render...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.