Problem/Motivation

Alot of the fields in \Drupal\eck\Entity\EckEntity::baseFieldDefinitions could be dropped by calling out to the parent method.

Proposed resolution

Let's try that out.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

legolasbo’s picture

Status: Active » Needs review
FileSize
17.19 KB

Attached patch adds tests for the baseFieldDefinitions and calls out to the parent method for the default entity fields.

dawehner’s picture

+++ b/src/Entity/EckEntity.php
@@ -80,39 +80,16 @@ class EckEntity extends ContentEntityBase implements EckEntityInterface {
+  public static function baseFieldDefinitions(EntityTypeInterface $entityType) {
+    $fields = parent::baseFieldDefinitions($entityType);

That's a change which is unrelated IMHO. Feel free to do so though. I'm a huge fan of optimizing for reviewablity, aka. change one thing at a time.

Status: Needs review » Needs work

The last submitted patch, 2: refactor-2797789-2.patch, failed testing.

The last submitted patch, 2: refactor-2797789-2.patch, failed testing.

legolasbo’s picture

Status: Needs work » Needs review
FileSize
17.19 KB

Rerolled the patch, lets see what happens.

Status: Needs review » Needs work

The last submitted patch, 6: refactor-2797789-6.patch, failed testing.

The last submitted patch, 6: refactor-2797789-6.patch, failed testing.

legolasbo’s picture

Status: Needs work » Needs review
FileSize
17.21 KB
473 bytes

Added the missing @group annotation.

legolasbo’s picture

Performed the refactoring in a separate issue ([#2825144)] as suggested in #3 and rerolled the patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/tests/src/Unit/UnitTestBase.php
@@ -1,135 +1,155 @@
+namespace Drupal\eck\Entity {
+  if (!function_exists('t')) {
+    function t($string) {
+      return $string;
+    }
+  }
 }

note: on the longrun you can use TranslatableMarkup::create(), out of scope of this issue of course.

  • legolasbo committed e13312d on 8.x-1.x
    Issue #2797789 by legolasbo, dawehner: Refactor \Drupal\eck\Entity\...
legolasbo’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x-1.x. Thanks for reviewing dawehner.

Status: Fixed » Closed (fixed)

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