Problem/Motivation

When enabling Views and a module (for eg node) has views, Views installation fails if the entities aren't stored in SQL.

Proposed resolution

  1. Patch EntityViewsData to get the generic entity storage interface instead of the SQL specific one and bail out early if it's not the SQL one. This deals with one fatal.
  2. Add ConfigEntityBase::isInstallable() by default returning TRUE and View will override it to check the base table existing.

Remaining tasks

User interface changes

API changes

A new method on ConfigEntityBase that can be overridden but it'll be seldom necessary. It's an addition.

Files: 
CommentFileSizeAuthor
#16 2419059_16.patch5.04 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,235 pass(es). View
#13 2419059_13.patch4.59 KBchx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,174 pass(es), 1 fail(s), and 0 exception(s). View
#8 2419059_8.patch3.06 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,208 pass(es). View
#6 2419059_6.patch2.54 KBchx
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Comments

chx’s picture

Issue summary: View changes
Berdir’s picture

That's the calculate dependencies code, that has hardcoded assumptions that everything must be there when default views are imported, and it is not when the views data is not.

I'm not sure how to fix this myself, maybe we need some sort of dependency to skip those default views if the views data they need is not available?

dawehner’s picture

I'm not sure how to fix this myself, maybe we need some sort of dependency to skip those default views if the views data they need is not available?

Yeah, I guess we want to check for the base table in views data + the existance of the sql query plugin maybe?

alexpott’s picture

Discussed with @chx and @dawehner in IRC. The outcome of which is that we're going to add a new method to ConfigEntityBase called isInstallable() that will run before config entities are created by the installer. By default this returns TRUE but views will be able to determine if the base table exists and go from there.

chx’s picture

Issue summary: View changes

Issue summary updated

chx’s picture

Status: Active » Needs review
FileSize
2.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Let's see if this blows up. Meanwhile I will look for a good place to add an assert.

Status: Needs review » Needs work

The last submitted patch, 6: 2419059_6.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,208 pass(es). View
chx’s picture

I think to test this we need an entirely new test module. Do we want to do that?

alexpott’s picture

@chx just use the ConfigTest entity make it return false for isInstallable depending on state and then add to one of the existing config entity tests.

alexpott’s picture

Issue tags: +Needs tests
dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -192,4 +192,12 @@ public function onDependencyRemoval(array $dependencies);
    +  /**
    +   * Checks whether this entity is installable.
    +   *
    

    I always like if we explain a bit more why a config entity might not be installable, so for example has dependencies on more than just config_dependencies.

  2. +++ b/core/modules/views/src/Entity/View.php
    @@ -441,4 +441,12 @@ public function mergeDefaultDisplaysOptions() {
    +    return \Drupal::service('views.views_data')->get($this->base_table);
    

    Let's directly cast it to bool here,

  3. +++ b/core/modules/views_ui/src/ViewUI.php
    @@ -1241,4 +1241,12 @@ public function addDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
    +  ¶
    

    Nitpick: whitespace.

chx’s picture

FileSize
4.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,174 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 13: 2419059_13.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Not sure what to do.

Pass      Other      ConfigImporterTes  538 Drupal\config\Tests\ConfigImporterT
    Value true is TRUE.

this passes just fine here.

chx’s picture

FileSize
5.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,235 pass(es). View
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for addressing my concerns!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Good find. I don't see another way around this.

Committed/pushed to 8.0.x, thanks!

  • catch committed 69e817c on 8.0.x
    Issue #2419059 by chx: Impossible to enable views if entities are not in...

Status: Fixed » Closed (fixed)

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

chx’s picture