Problem/Motivation

#2786583: Create a base class for DateTimeFieldTest and DateRangeFieldTest and move common code into it inadvertently coupled datetime.module to datetime_range.module

Proposed resolution

Change Item class.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#5 2859013-FAIL.patch629 bytesxjm
#2 2859013-02.patch1.02 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented
Status: Active » Needs review
FileSize
2859013-02.patch1.02 KB
mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented 
+++ b/core/modules/datetime/src/Tests/DateTestBase.php
@@ -5,7 +5,7 @@
-use Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem;
+use Drupal\datetime\Plugin\Field\FieldType\DateTimeItem;
 use Drupal\entity_test\Entity\EntityTest;
 use Drupal\field\Entity\FieldConfig;
 use Drupal\field\Entity\FieldStorageConfig;
@@ -114,7 +114,7 @@ protected function createField() {

@@ -114,7 +114,7 @@ protected function createField() {
       'field_name' => $field_name,
       'entity_type' => 'entity_test',
       'type' => $type,
-      'settings' => ['datetime_type' => DateRangeItem::DATETIME_TYPE_DATE],
+      'settings' => ['datetime_type' => DateTimeItem::DATETIME_TYPE_DATE],
     ]);

The more I look at this, the more confused I am. If datetime_range.module is not enabled, why is the autoloader finding the DateRangeItem class?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs review » Reviewed & tested by the community

If datetime_range.module is not enabled, why is the autoloader finding the DateRangeItem class?

I wonder if the autoloader works differently when tests are running, or something like that?

Regardless, this is a clear bug and a very easy fix.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
FileSize
2859013-FAIL.patch629 bytes

Let's just make sure it actually is working in light of the autoloader/module enabled thing. I wouldn't be surprised if this was different in tests but just to make sure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2859013-FAIL.patch, failed testing.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio at DiD Agency commented

@tim.plunkett helped me understand this. In Drupal\simpletest\TestDiscovery::registerTestNamespaces() we have

      // Add namespace of disabled/uninstalled extensions.
      if (!isset($existing["Drupal\\$name\\"])) {
        $this->classLoader->addPsr4("Drupal\\$name\\", "$base_path/src");
      }

I believe this is so that a class in a test can be resolved properly with the autoloader before the setUp() enables it.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs work » Reviewed & tested by the community

Ah, #7 totally makes sense!

#5 has fewer fails than I was expecting, but does still prove it's actually being used.

  • xjm committed d87d382 on 8.4.x 
    Issue #2859013 by mpdonadio: DateTestBase inadvertently coupled to...

  • xjm committed 9b94bb9 on 8.3.x 
    Issue #2859013 by mpdonadio: DateTestBase inadvertently coupled to...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 8.4.x, thanks! I also cherry-picked it to 8.3.x as a test coverage fix.

Status: Fixed » Closed (fixed)

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