Follow-up to #2374339: FieldConfigBase::calculateDependencies() fatal error is unhelpful

Problem/Motivation

+++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
@@ -248,7 +249,9 @@ public function calculateDependencies() {
+        throw new \LogicException(String::format('Missing bundle entity, entity type %type, entity id %bundle.', array('%type' => $bundle_entity_type_id, '%id' => $this->bundle)));

This should use %bundle in the array.

Proposed resolution

Fix.

CommentFileSizeAuthor
#6 interdiff.txt1.32 KBbenjy
#6 2384665-6.patch3.61 KBbenjy
#4 interdiff.txt2.61 KBbenjy
#4 2384665-4.patch3.63 KBbenjy
#1 2384665.patch1.02 KBpenyaskito

Comments

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Fixed » Needs review
StatusFileSize
new 2384665.patch1.02 KB

Patch attached.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Good catch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Let's test this in FieldConfigEntityUnitTest

benjy’s picture

benjy commented
Status: Needs work » Needs review
StatusFileSize
new 2384665-4.patch3.63 KB
new interdiff.txt2.61 KB

I first went with the following on top of the class:

@expectedException \LogicException
@expectedExceptionMessage Missing bundle entity, entity type

But I decided that catching the exception would allow me to assert the exact error message since we had previously got that wrong as well.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented 
+++ b/core/modules/field/tests/src/Unit/FieldConfigEntityUnitTest.php
@@ -175,6 +175,61 @@ public function testCalculateDependencies() {
+    $this->assertEquals($e->getMessage(), 'Missing bundle entity, entity type <em class="placeholder">bundle_entity_type</em>, entity id <em class="placeholder">test_bundle_not_exists</em>.');

If message is hardcoded, why the previous approach would not work?

benjy’s picture

benjy commented
StatusFileSize
new 2384665-6.patch3.61 KB
new interdiff.txt1.32 KB

Good point, when I wrote that I was using $this->entityTypeId which is calculated at run time but then I updated it last minute. New patch attached.

Thanks for the review.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I was just curious, not a strong preference. Both #4 and #6 look good to me.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/field/tests/src/Unit/FieldConfigEntityUnitTest.php
@@ -175,6 +175,60 @@ public function testCalculateDependencies() {
+    $this->entityManager->expects($this->at(0))
+      ->method('getDefinition')
+      ->with($this->entityTypeId)
+      ->willReturn($this->entityType);
+    $this->entityManager->expects($this->at(1))
+      ->method('getDefinition')
+      ->with($this->entityTypeId)
+      ->willReturn($this->entityType);
+    $this->entityManager->expects($this->at(2))
+      ->method('getDefinition')
+      ->with($this->entityTypeId)
+      ->willReturn($this->entityType);
+    $this->entityManager->expects($this->at(3))
+      ->method('getDefinition')
+      ->with('test_entity_type')
+      ->willReturn($target_entity_type);

Do we really want to ensure that things are called here in that exact order? I think using ->willReturnValueMap is the better approach to reduce changes in the test, when the actual code is refactored.

benjy’s picture

benjy commented

It was an exact copy and paste from the test above it. We could refactor both tests if you think willReturnValueMap() is better, how about a follow-up for that?

dawehner’s picture

dawehner
Primary language German
commented

Okay sure, where is the URL :P

benjy’s picture

benjy commented

Issue created, #2389837: Refactor FieldConfigEntityUnitTest to make it resilient to changes. See you there.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Postponed that one on this one.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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 1ffac67 and pushed to 8.0.x. Thanks!

  • alexpott committed 1ffac67 on 8.0.x 
    Issue #2384665 by benjy, penyaskito: Follow-up: FieldConfigBase::...

Status: Fixed » Closed (fixed)

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