Views does not properly check config integrity, causing a fatal error in PHP 8 when keys are missing

[richardlampitt]

Description of the bug

Original title: Views fatal error in PHP 8 caused by init_display not properly checking value before attempting to create an object.

If a view display config file is missing some keys, Backdrop will crash disgracefully with a fatal error in PHP 8. In PHP 7 a, warning is thrown instead.

In my case, for some reason the Ubercart module did not import some of its views correctly when the module was activated, and the name key was missing in the config file. Henceforth any time the full list of views was accessed, such as on cache clear or attempting access the list of views, the site fell over and died.

It was particularly particularly bothersome because the name of the view was missing, provided notices and messages were useless: User notice: 'Skipping broken view ' in views_get_applicable_views() (line 1311 of C:\wamp64\www\_backdrop\sr-backdrop.local\httpdocs\core\modules\views\views.module). I had to resort to manually checking each view on an insecure version of PHP (7.4), and trawling through the config visually, to find the problem.

Even if my issue is a bit of a random edgecase, I still believe that it would be extremely prudent to check the data views is valid, or at the very least exists, before attempting to do anything with it.

This will henceforth protect malformed config from both automated changes (such as my case) and developer edits.

Steps To Reproduce

• Have a view with some manner of missing key in the config.
• Attempt to create a node, clear caches, access the views overview admin page, or adjust a view.

Actual behavior

In PHP 7.4, this causes a warning to get issued:

Warning: Creating default object from empty value in view->init_display() (line 514 of C:\wamp64\www\_backdrop\sr-backdrop.local\httpdocs\core\modules\views\includes\view.inc).

In PHP 8 this causes a fatal error.

Error: Attempt to modify property "handler" on null in view->init_display() (line 514 of C:\wamp64\www\_backdrop\sr-backdrop.local\httpdocs\core\modules\views\includes\view.inc).

Expected behavior

No errors or warnings are returned during the attempted operation.

Suggested fixes / changes

1. Assuming that gracefully maintaining site uptime is the priority:
   • Check that any $views object keys exist before use, and ideally check that the data is valid to some degree,
   • This could be done with a try { /*…*/ } catch () { /* Log the error gracefully and stop */ }, or an if ( isset($view->thing) && $view->thing ) { /*…*/ } else { /* Log the missing data gracefully and stop */ }
2. Ensure that any notices / errors / watchdog / logs don't rely on data that could be missing. Even just the name of the accessed config would have been extremely useful in finding what was happening.

Additional information

Add any other information that could help, such as:

• Backdrop CMS version: 1.31
• Web server and its version: apache 2.4.54
• PHP version: 8.0+ (tested with 8.0, 8.1, 8.2 and 8.3)
• Database sever (MySQL or MariaDB?) and its version: MariaDB Version: 10.11.11
• Operating System and its version: Windows NT 10.0 build 19045 (Windows 10) AMD64

The issue is in the init_display function:

  /**
   * Set the display for this view and initialize the display handler.
   */
  function init_display($reset = FALSE) {
    // The default display is always the first one in the list.
    if (isset($this->current_display)) {
      return TRUE;
    }

    // Instantiate all displays
    foreach (array_keys($this->display) as $id) {
      // Correct for shallow cloning
      // Often we'll have a cloned view so we don't mess up each other's
      // displays, but the clone is pretty shallow and doesn't necessarily
      // clone the displays. We can tell this by looking to see if a handler
      // has already been set; if it has, but $this->current_display is not
      // set, then something is dreadfully wrong.
      if (!empty($this->display[$id]->handler)) {
        $this->display[$id] = clone $this->display[$id];
        unset($this->display[$id]->handler);
      }
      $this->display[$id]->handler = views_get_plugin('display', $this->display[$id]->display_plugin);
      if (!empty($this->display[$id]->handler)) {
        $this->display[$id]->handler->localization_keys = array($id);
        // Initialize the new display handler with data.
        $this->display[$id]->handler->init($this, $this->display[$id]);
        // If this is NOT the default display handler, let it know which is
        // since it may well utilize some data from the default.
        // This assumes that the 'default' handler is always first. It always
        // is. Make sure of it.
        if ($id != 'default') {
          $this->display[$id]->handler->default_display = &$this->display['default']->handler;
        }
      }
    }

    $this->current_display = 'default';
    $this->display_handler = &$this->display['default']->handler;

    return TRUE;
  }

There is nothing to check that $this->display['default']->handler exists or has a value before attempting to use it to assign the display handler.

[richardlampitt]

Updated the issue after finding and fixing the cause.

[richardlampitt]

As an aside, I think it would be extremely wise to ensure that all provided watchdog log entries in core don't exclusively rely on data that could be missing or malformed. I wasted hours of time on this problem over the last couple of days and it's eaten up all of this project's contingency budget.

This is of course beyond the scope of this bug report, but I need to vent. Is it worth putting in a feature request (or something else) for this?