Follow-up for #2204037: Views allows removal of required relationships and gives a fatal error on save.

Problem/Motivation

ViewExecutable::getHandlers($type, $display_id = NULL) changes ViewExecutable::current_display to $display_id (or 'default' if NULL) and doesn't set it back to the initial value.

Proposed resolution

To store initial value and restore it before return.

CommentFileSizeAuthor
#11 restore-display-id-2408613-11.patch1.99 KBkpv
#7 restore-display-id-2408613-test_only-7.patch843 byteskpv
#5 restore-display-id-2408613-test_only.patch844 byteskpv
#3 restore-display-id-2408613-2.patch1.17 KBkpv
#1 restore-display-id-2408613-1.patch1.12 KBkpv
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kpv’s picture

kpv CreditAttribution: kpv commented
Status: Active » Needs review
FileSize
restore-display-id-2408613-1.patch1.12 KB

The patch restores initial display id or sets it to 'default' if it wasn't set earlier.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +VDC, +Needs tests

The patch looks nice in general.

@kpv
It would be great if you could explain why we can't get just rid of the setDisplay() call. Sure, your fix is safer, just being curios here.
It would be great if you could add a beta evaluation as well as a test

kpv’s picture

kpv CreditAttribution: kpv commented
FileSize
restore-display-id-2408613-2.patch1.17 KB

added check for current and initial display _ids

kpv’s picture

kpv CreditAttribution: kpv commented

@dawehner
setDisplay() is used to initialize $this->displayHandlers, if empty, (via initDisplay()) and to perform some checks like if (!$this->displayHandlers->has($display_id)) debug(...).
We could directly set $this->displayHandlers = new DisplayPluginCollection($this, Views::pluginManager('display')) in getHandlers() and copy required checks there.
In this case we should also make some changes to setDisplay() not to instantiate displayHandlers twice if it was set earlier.

kpv’s picture

kpv CreditAttribution: kpv commented
FileSize
restore-display-id-2408613-test_only.patch844 bytes

this one should fail

Status: Needs review » Needs work

The last submitted patch, 5: restore-display-id-2408613-test_only.patch, failed testing.

kpv’s picture

kpv CreditAttribution: kpv commented
FileSize
restore-display-id-2408613-test_only-7.patch843 bytes

minor test fix. also should fail

kpv’s picture

kpv CreditAttribution: kpv commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: restore-display-id-2408613-test_only-7.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

@kpv
Do you mind merging the two together?

kpv’s picture

kpv CreditAttribution: kpv commented
Status: Needs work » Needs review
FileSize
restore-display-id-2408613-11.patch1.99 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 079b250 and pushed to 8.0.x. Thanks!

  • alexpott committed 079b250 on 8.0.x 
    Issue #2408613 by kpv: ViewExecutable::getHandlers() should restore...

Status: Fixed » Closed (fixed)

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