Move all HAL tests to the module in preparation of removal in D10

Now that's annoying, locally the test passes when ran as a single test:

$ ../vendor/bin/phpunit --stop-on-failure modules/hal/tests/src/Kernel/EntityResource/EntityResourceHalTestCoverageTest.php
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\hal\Kernel\EntityResource\EntityResourceHalTestCoverageTest
.                                                                   1 / 1 (100%)

Time: 01:19.712, Memory: 10.00 MB

OK (1 test, 1 assertion)

I think we're there. All HAL related tests should now be in the hal namespace and /core/modules/hal/tests directory.

To make sure we indeed moved everything:

• I've applied the changes in this MR on D10.0.x-dev
• Then deleted the whole core/modules/hal
• Altered the drupal-9.3.0.filled.standard.php.gz fixture to _not_ have the HAL module installed

Then ran it through TestBot here: https://www.drupal.org/project/drupal/issues/3191612#comment-14408424, that came back green, so I think we indeed caught them all.

Amazing work

Thanks for the updates. I've updated the title and issue summary with typo fixes and minor wording and formatting changes.

Thanks @daffie for the review, all threads resolved.

diff --git a/core/modules/rest/config/optional/rest.resource.entity.node.yml b/core/modules/hal/config/optional/rest.resource.entity.node.yml
similarity index 100%
rename from core/modules/rest/config/optional/rest.resource.entity.node.yml
rename to core/modules/hal/config/optional/rest.resource.entity.node.yml

Are we sure that we want to do this? The config is be used in core/modules/layout_builder/tests/src/Functional/Rest/OverrideSectionsTest.php. Also we are linking core/core.api.php to this to be removed from core config.

Are we sure that we want to do this? The config is be used in core/modules/layout_builder/tests/src/Functional/Rest/OverrideSectionsTest.php. Also we are linking core/core.api.php to this to be removed from core config.

Thus spoketh @daffie in #20

The rationale here is that even when moved as-is to Contrib, HAL needs to work out-of-the-box.. Which means there should be (as before) a working example of a REST endpoint config file. Hence the moving-to-HAL of this config YAML.
The mention in core/core.api.php is altered from a currently already non-existing path to the one in this change.

In the actual removal in D10.0.0 issue for HAL #3049857: Remove HAL module from core and create a contrib project for it we restore the REST endpoint config in /core/modules/rest/config/optional/rest.resource.entity.node.yml but as json instead of hal_json. (As mentioned in the @todo in core/modules/system/tests/src/Functional/System/ResponseGeneratorTest.php). Also we change core/core.api.php once more to link to the config in the REST module.

(And thanks for reviewing these MegaMR)

I get why the config item has been moved.
All changes look good to me.
All my points are resolved.
For me it is RTBC.

Read through the whole patch, the changes almost all look good to me. Added a couple of minor questions but nothing that should block the RTBC.

To make sure we've met "gotta catch them all" for the moved test, I've applied the changes in this MR to the latest 10.0.x-dev, applied the changes from #3049857: Remove HAL module from core and create a contrib project for it on top of it (which is in essence some small changes, a changed 9.3-filled fixture which has hal module _not_ installed, and deletion of the core/modules/hal directory) here: https://www.drupal.org/project/drupal/issues/3191612#comment-14411302

FWIW: That just came back green.

I've looked at the remarks made by @longwave (tips hat for doing the review), they make sense (per usual) but are indeed more nice to haves IMHO.
I'll try to tackle them later today, if $dayjob allows it. (I'm pretty sure by then this issue won't be committed yet.)

Hang on, didn't want a status change...

Thanks @daffie for the review and suggestions. I've tried to incorporate them all.

TestBot returned green, also the "Gotta catch em all" test (Apply the changes in this MR on top of 10.0.x-dev, apply the changes of the removal MR #3049857: Remove HAL module from core and create a contrib project for it on top of that, delete the whole core/modules/hal) came back green here: https://www.drupal.org/project/drupal/issues/3191612#comment-14412677

Since code changed I think another round of review is needed => Back to NR

All open threads are (in my mind at least) resolved, but since I didn't create the MR I can't actually resolve them. @bbrala will come along at some point and do so.

All code changes look good to me.
All threads on the MR are addressed.
For me it is RTBC.

OK this is a huge diff but it's only really doing a couple of things:

1. 1-1 moves for Hal tests
2. Stops using HAL as a random format in a couple of tests, switching to something else.
3. Duplicating/splitting some generic REST tests that cover both json and hal into hal module to only test hal.

All of this seems fine. However, the MR diff only applies to 9.4.x, and I think (?) this should go into 10.0.x first and be backported (then contrib hal can be a subtree split following this change), so we need a separate 10.x patch/MR to commit there first.

Thanks @catch for speaking words of wisdom (let it be, let it be, let it be-e, let it be...), aiming for 10.0.x first is indeed inline with the normal proceedings now 10.0.0-alpha-something is out, I'll create a new MR for that.

• MR!1802 is against 9.4.x-dev
• MR!1848 is against 10.0.x-dev

00:02:55.711 Running PHPStan on *all* files.

00:04:07.005  ------ -------------------------------------------------------------------- 
00:04:07.005   Line   core/lib/Drupal/Core/Http/RequestStack.php                          
00:04:07.005  ------ -------------------------------------------------------------------- 
00:04:07.005   26     Call to an undefined static method                                  
00:04:07.005          Symfony\Component\HttpFoundation\RequestStack::getMasterRequest().  
00:04:07.005  ------ -------------------------------------------------------------------- 
00:04:07.005 
00:04:07.005 
00:04:07.005  [ERROR] Found 1 error                                                          
00:04:07.005 
00:04:07.046 
00:04:07.046 PHPStan: failed

Another victim of #3265356: Deprecate Symfony 4 RequestStack BC shim for removal in D11, postponing on that issue.

Unpostponing after #3265356: Deprecate Symfony 4 RequestStack BC shim for removal in D11 landed.

Give this is just a re-roll since the last RTBC, moving it back there.

Even the tiny help_topic snippet for the hal module should probably also be moved.

Also attached a raw diff between both MRs.

Back to RTBC

Committed/pushed the respective patches to 10.0.x and 9.4.x respectively, thanks!

Committed/pushed the respective patches to 10.0.x and 9.4.x respectively, thanks!

@catch Looks like the 9.4.x part of the above sentence never made it into Git, forgotten push perhaps?

At least one of the tests in this issue should not have been removed from core. See #3268078: DBLogResource is no longer being tested

Opened an issue for this one too:
#3268105: Bring back RestRegisterUserTest into user module (without HAL)

Tagging for posterity.