Follow up for #2848914: Move DbLogTest::generateLogEntries() into a Trait

From #9.2

+++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
@@ -0,0 +1,57 @@
+    $logger = $this->container->get('logger.dblog');

This assumes the ->container object will be present. This can make harder to re-use the trait. I think would be better to use Drupal::service instead.

From #19.4

+++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
@@ -0,0 +1,63 @@
+      'user'        => $this->adminUser,
+      'uid'         => $this->adminUser->id(),

I think could be safe to check if $this->adminUser is defined before try to use is. Maybe using a ternary operator and fallback to an anonymous user if empty.

CommentFileSizeAuthor
#9 2985701-9.patch1.27 KBlongwave
#6 2985701.patch1.54 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dagmar created an issue. See original summary.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Gizra commented
Issue summary: View changes
dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Gizra commented
Issue summary: View changes

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Gizra commented
Status: Postponed » Active

#2848914: Move DbLogTest::generateLogEntries() into a Trait was committed. This issue is unblocked now.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented
Status: Active » Needs review
FileSize
2985701.patch1.54 KB

#2848529: Move DbLogTest::verifyCron to a kernel test will suffer from the missing adminUser issue.

This patch makes both the requested changes.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Gizra commented
Status: Needs review » Reviewed & tested by the community

Thanks @longwave. Patch looks good.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review

Under what circumstances would $this->container not be present in a test class using this trait?

The admin user change looks fine though.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave as a volunteer commented
FileSize
2985701-9.patch1.27 KB

Good point, I don't see when it wouldn't be available, and we can always revisit if it turns out to be a problem. Attached patch removes this change and also fixes a whitespace issue in the original patch.

Incidentally, do we have or need any way of documenting trait dependencies like this? I guess it should be obvious in most cases when you try to use them?

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Gizra commented
Status: Needs review » Reviewed & tested by the community

Thanks again @longwave

Under what circumstances would $this->container not be present in a test class using this trait?

Yes. Sorry, I checked the KernelTestBase class and the container is defined there. I didn't realized this when I created the issue.

  • catch committed 7cb2fe8 on 8.7.x 
    Issue #2985701 by longwave, dagmar, catch: Followup: Move DbLogTest::...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

  • catch committed f208a93 on 8.6.x 
    Issue #2985701 by longwave, dagmar, catch: Followup: Move DbLogTest::...

Status: Fixed » Closed (fixed)

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