Problem/Motivation

At the moment EntityReferenceItem does key = $target_entity_type instanceof ConfigEntityType ? 'config' : 'content'; to determine where to store a dependency on the default value. This is a mistake since it means plugins have to know too much about entity types. Views also has plugins that can depend on both content or config dependencies - see #2341357: Views entity area config is not deployable and missing dependencies.

Proposed resolution

Add a method to EntityTypeInterface to return this.

Remaining tasks

  • Review
  • Commit

User interface changes

None

API changes

Additional method getConfigDependencyKey

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Prioritized changes This change reduces fragility because it moves responsibility for determining the location for configuration entity dependencies to the entity type interface which means that plugins which do not distinguish between config and content do not have to have custom logic. Also content dependencies are very new to Drupal 8 and this is followup to #2282519: Add content dependency information to configuration entities.
Disruption This is not disruptive.
Files: 
CommentFileSizeAuthor
#6 2390615.6.patch3.73 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2390615.6.patch. Unable to apply patch. See the log in the details link for more information. View
#6 1-6-interdiff.txt617 bytesalexpott
#1 2390615.1.patch3.51 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2390615.1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
3.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2390615.1.patch. Unable to apply patch. See the log in the details link for more information. View

Patch attached

Berdir’s picture

Makes sense to me.

The related question is how to properly identify if an entity type is config or content, we have different approaches for that, the old code here or the isSubclassOf() method, but that's not related to this (anymore).

xjm’s picture

Wim Leers’s picture

Looks good :)

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -680,4 +680,11 @@ public function getListCacheTags() {
+  /**
+   * {@inheritdoc}
+   */
+  public function getConfigDependencyKey() {
+    return 'content';
+  }
+

OH, are we sure that this is right here? ... so do we really support referencing a basic entity? Are we sure that the behaviour is exactly like for content entities? In case we do, we could also skip the implementation in the ContentEntityType

alexpott’s picture

FileSize
617 bytes
3.73 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2390615.6.patch. Unable to apply patch. See the log in the details link for more information. View

@dawehner I thought I added a comment to address that. Patch now contains a comment.

xjm’s picture

Issue tags: +Needs tests

Let's add a unit test and/or check or document the implicit test coverage?

alexpott’s picture

Issue tags: -Needs tests

There is explicit test coverage of where entity reference in \Drupal\entity_reference\Tests\EntityReferenceIntegrationTest. I don't think we should be adding a test method to EntityTypeTest to check that getConfigDependencyKey() returns a string.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me as well, and I agree about the existing test coverage. Assuming the testbot will come back green at some point.. RTBC :)

alexpott’s picture

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This definitely looks a lot cleaner. Entity Reference definitely won't be the last module to try and differentiate between entity types.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 9eb4efb on 8.0.x
    Issue #2390615 by alexpott: Add method to determine config dependency...
alexpott’s picture

Drupal\config\Tests\ConfigDependencyTest contains complete coverage of the difference between content and config dependences.

The last submitted patch, 1: 2390615.1.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 6: 2390615.6.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

This was commit before testing was complete. HEAD is green post commit - so all good here.

Status: Fixed » Closed (fixed)

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