Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
As part of #2847428: [Meta] Modernize dblog module the plan is to use Kernel Tests to test the dblog module without require an UI.
The candidates for a Kernel Test are:
- #2848529: Move DbLogTest::verifyCron to a kernel test
- #2458191: Provide a storage backend for dblog module
Which requires generate log entries.
It would be nice to abstract the DbLogTest::generateLogEntries()
method into a trait to reuse it in both Kernel and Web based tests.
Proposed resolution
- Create a new Trait to generate log entries.
- Use this trait in
Drupal\dblog\Tests
. and in Kernel Tests
Remaining tasks
User interface changes
None
API changes
A new Trait.
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff-2848914-20-25.txt | 1.95 KB | ApacheEx |
#25 | 2848914-25.patch | 5.22 KB | ApacheEx |
Comments
Comment #2
dagmarComment #3
dagmarThis needs a re-roll after #2882525: Move suspicious string in DblogTest into a KernelTest
Comment #4
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled. Check once.
Comment #6
pk188 CreditAttribution: pk188 at OpenSense Labs commentedComment #7
dagmarThanks @pk188. If you take a look to #2882525: Move suspicious string in DblogTest into a KernelTest you will realize that this string was removed from the test precisely to make this easier to review.
Comment #8
pk188 CreditAttribution: pk188 at OpenSense Labs commentedFixed #7.
Comment #9
dagmarThanks @pk188. Here are some comments:
In Drupal 8.4.x we have a replacement for REQUEST_TIME. We can use it here.
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.
Missing newline.
Comment #10
pk188 CreditAttribution: pk188 at OpenSense Labs commentedThanks! for reviewing.
Updated according to #9.
Comment #12
pk188 CreditAttribution: pk188 at OpenSense Labs commentedUpdated last patch. I think this should work.
Comment #13
dagmarThanks @pk188
Please take a look to the Change record
Comment #14
pk188 CreditAttribution: pk188 at OpenSense Labs commentedI updated the patch.
Comment #15
dawehner-1 for changing anything in there. IMHO just moving the code is the right scope for this particular issue.
Comment #17
dagmarBased on @dawehner comments, let's back to #9 comments, without the part of TIME_REQUEST.
Comment #18
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Drupal Ukraine Community commentedHere is updated patch, based on #9.
Comment #19
dagmarWe can replace
watchdog
with justlog
We can use [] syntax.
We can use new [] syntax.
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.Comment #20
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Drupal Ukraine Community commentedThanks for your review. Yeah, I agree with fixes, here is updated patch.
Comment #21
dagmarLooks good to me. Thanks @ApacheEx
Comment #22
xjmAs @dawehner said in #9 it's not a good idea to mix moving code and improving it in the same issue. Let's make this issue just about moving the code, and improve it in followups. Thanks!
Comment #23
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Drupal Ukraine Community commented@xjm,
Perhaps #18 is what we need, is not it?
Comment #24
dagmarI think @xjm refers to #19.4.
Ok, so let's do this, just move the code, inside a trait. Forget all the enhancements, I will create the follow ups. Also, please consider that the [] syntax is already in place.
Comment #25
ApacheEx CreditAttribution: ApacheEx as a volunteer and at Internetdevels, Drupal Ukraine Community commentedThis is just about moving a code and nothing else. All other things like in #20 will be done in follow ups.
p.s Hopefully @dagmar will create a needed follow up (s).
Comment #27
dagmarNow that this is just moving code around as requested by @dawehner and @xjm I think is good to go.
Comment #28
xjmAh thanks @dagmar for getting this moving again.
Do you want to create a followup issue for your earlier review points? There are a couple things I will add to it if so. :) The issue can just be RTBC again once the followup is filed.
Thanks!
Comment #29
dagmarThanks @xjm
We can use [] syntax.
syntax was already completed long time ago.Here is the follow up: #2985701: Followup: Move DbLogTest::generateLogEntries() into a Trait. I'm not sure if we should make the trait dependent of ContainerAwareTrait or just use Drupal::service to address item 1. Could you provide some feedback on that?
Comment #33
catchThis is purely internal test re-organisation so fine during the rc. Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!