DBLogResource is no longer being tested [#3268078]

When the HAL module was moved out of core, the test for DBLogResource was also moved, because it uses the HAL module. This move of the test case was done in #3263654: Move all HAL tests to the module in preparation of removal in D10. The assumption seems to be that because the test used "hal", it was a test OF "hal" - that's not the case. DbLogResourceTest was written to test the DBLogResource, not to test "hal". It happens to use the hal format for the test, but that can easily be changed to json.

DbLogResourceTest should be restored to core D10 and rewritten so it does not depend on the HAL module.

Marking this as "major" because we're supposed to have tests for every feature, and we did have a test for this feature until it was removed.

Comment	File	Size	Author
#13	raw_diff_9.4_-10.0.txt	4.34 KB	Spokje

Issue fork drupal-3268078

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

1 hidden branch

3268078-dblogresource-is-no

changes, plain diff MR !2133

Check out this branch

About issue forks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

7 March 2022 at 03:08

TR created an issue. See original summary.

Comment #2

TR CreditAttribution: TR commented 7 March 2022 at 03:09

Related issues:

+#3268077: Rename DBLogResource

Comment #3

TR CreditAttribution: TR commented 4 April 2022 at 00:49

Comment #4

TR CreditAttribution: TR commented 17 April 2022 at 19:09

Priority:	Major	» Critical
Issue tags:		+Regression

Comment #5

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 18 April 2022 at 07:52

Category:	Bug report	» Task
Issue tags:	-Regression

Missing test coverage is a task.

Comment #6

TR CreditAttribution: TR commented 19 April 2022 at 05:10

Someone mistakenly deleting a test that has been part of Drupal core for 5 years is not a bug? And it's not a regression?

If the bar is lower for deleting things than it is for restoring valuable working code, that makes things easy to break but hard to fix. It only took two weeks to delete all that hal module stuff, including at least two tests that should not have been deleted. Now, SIX weeks after I pointed this out, this test is still missing.

Comment #7

19 April 2022 at 08:07

Spokje made their first commit to this issue’s fork.

Comment #8

19 April 2022 at 08:08

Spokje opened merge request !2133

Comment #9

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 19 April 2022 at 09:08

Status:

Active

» Needs work

Someone mistakenly deleting a test that has been part of Drupal core for 5 years is not a bug?

no.

Comment #10

Spokje

Dutch

CreditAttribution: Spokje commented 19 April 2022 at 09:18

Assigned:

Unassigned

» Spokje

No matter what it is, let's fix/reinstate it :)

Comment #11

Spokje

Dutch

CreditAttribution: Spokje commented 19 April 2022 at 10:45

Assigned:	Spokje	» Unassigned
Status:	Needs work	» Needs review

The MR reintroduces DbLogResourceTest (https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/modules/hal/...) in 10.0.x-dev, this time in the Drupal\Tests\dblog\Functional namespace.

It also reintroduces this core/phpstan-baseline.neon error suppression (https://git.drupalcode.org/project/drupal/-/commit/0cec48668ca71acbe586e81eef6b8b12d1aac2a7?page=10#650015ff26df4f3beec973d0ba4dac1c8a868816_359_359) and adds a new one for DbLogResourceTest::getExpectedUnauthorizedEntityAccessCacheability.

The new addition is inline with the already present ones for FileUploadJsonBasicAuthTest, FileUploadJsonCookieTest and UserRegistrationRestTest.

Comment #12

TR CreditAttribution: TR commented 20 April 2022 at 18:29

+ public function setUp(): void {
This is wrong for Drupal 10. In Drupal 10, this should be protected.

But because ResourceTestBase is one of the only core classes that did not use the correct visibility of setUp() in Drupal 9, if this patch is backported to Drupal 9 then the Drupal 9 version (and ONLY the Drupal 9 version) should use public.

It would be nice to see an interdiff against the deleted DbResourceTest so we can see what changes you made when you restored the deleted test.

Comment #13

Spokje

Dutch

CreditAttribution: Spokje commented 25 April 2022 at 08:42

File	Size
raw_diff_9.4_-10.0.txt	4.34 KB

Thanks @TR.

+ public function setUp(): void {
This is wrong for Drupal 10. In Drupal 10, this should be protected.

You're absolutely right, didn't correct it after the c/p from D9.
Although I do find it weird it isn't picked up on by neither PHPStorm nor any of our PHPCS/PHPStan tooling.
Anyway: Changed it.

[snip] if this patch is backported to Drupal 9 then the Drupal 9 version (and ONLY the Drupal 9 version) should use public.

I don't think this will be backported to D9, since in there the DbLogResource is still tested here.

It would be nice to see an interdiff against the deleted DbResourceTest so we can see what changes you made when you restored the deleted test.

Attached is a diff between the linked D9.4 DbLogResourceTest and the one in the current MR. Should have done that in the first place.

Comment #14

TR CreditAttribution: TR commented 28 April 2022 at 23:15

Status:

Needs review

» Reviewed & tested by the community

Looks perfect. Thanks.

Comment #15

29 April 2022 at 16:58

catch committed f8411d7 on 10.0.x

Issue #3268078 by Spokje, TR: DBLogResource is no longer being tested

Comment #16

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 29 April 2022 at 16:58

Status:

Reviewed & tested by the community

» Fixed

Committed f8411d7 and pushed to 10.0.x. Thanks!

Comment #17

mondrake

🇮🇹

CreditAttribution: mondrake as a volunteer commented 29 April 2022 at 17:30

Confused. Frankly I would not expect to ADD entries to the PHPStan baseline through regular commits. IMHO additions should only be through running the automated baseline generation; patches should REMOVE from the baseline. In the case at hand, both the $entityTypeId static property and the ::getExpectedUnauthorizedEntityAccessCacheability() could be added to the test while maybe something better could be done in the parent and/or the trait that are referencing them without ensuring they exist.

Comment #18

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 29 April 2022 at 17:50

@mondrake this issue is restoring a test that was mistakenly removed (in the issue that removed HAL module), so we did the absolute minimum to restore the test coverage. If we hadn't mistakenly removed the test in the first place, we also wouldn't have removed those entries from the phpstan baseline either, so the net result is no new phpstan entries. Worth fixing in a follow-up, but not to block restoring the test coverage.