Remove remaining (textual) references to the HAL module

First great issue summary

The changes to me look good. Can you update the MR to 10.1 though please. Then I don't mind marking RTBC.

Thanks!

Changes look good to me.

These are the references I find with the MR applied:

[ayrton:maintainer | Fri 19:09:08] $ grep -rEi "\bhal\b" * | grep -v "vendor" | grep -v "node_modules"
core/lib/Drupal/Component/Transliteration/data/xd5.php:  0x60 => 'hal', 'halg', 'halm', 'halb', 'hals', 'halt', 'halp', 'halh', 'ham', 'hab', 'habs', 'has', 'hass', 'hang', 'haj', 'hach',
core/modules/serialization/serialization.services.yml:      # Set the priority lower than the hal entity reference field item
core/modules/serialization/serialization.services.yml:      # Priority must be lower than serializer.normalizer.field_item.hal and any
core/modules/serialization/serialization.services.yml:      # Priority must be lower than serializer.normalizer.field.hal.
core/modules/serialization/serialization.services.yml:      # than hal field normalizer.
core/modules/serialization/serialization.services.yml:      # than hal normalizers.
core/modules/system/system.install:  'hal' => 'HAL',
Binary file core/modules/system/tests/fixtures/update/drupal-9.4.0.filled.standard.php.gz matches
core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:      // entity (for example '_links' and '_embedded' in the HAL normalization).

The system.install one needs to stay for sure, as does the upgrade path fixture. The transliteration match is clearly unrelated.

However, I'm not sure that I agree with @Spokje that the references in serialization.services.yml should be kept, and I don't know what to do with the comment in EntityResourceTestBase.

Let's get a subsystem maintainer review from one of the API-First maintainers on what to do with those two.

(Aside: 🍪 for you for "HAL of the Mountain King".)

The comment in the test could just be trimmed imo.

      // Some top-level keys in the normalization may not be fields on the
      // entity.

Regarding the services, i understand you'd probably rather have those removed. This would be my first reaction also.

But I'm trying to imagine what will happen in a while if we remove those. Wouldn't it be rather confusing why those priorities exist? I might end up sending someone into a rabbithole. Also those comments are a pointer to an issue regarding a better way to select normalizers. (#2575761: Discuss a better system for discovering and selecting normalizers).

When examining the 'blame' it does seem to point in the right direction though, so someone who might need to find out, can. Pretty clean commit which actually added the comments also. I've added a link to the above issue in #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods, so it is still discoverable though some digging.

My conclusion, the information is clear enough, even without those comments. And it does feel weird to have hal live on through that. So I'd remove them.

In the test I think.

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

Appears that changes requested have been addressed.

My concern with the references in serialization.services.yml isn't that we mention HAL, it's that we're setting priorities relative to non-core services. I don't think we should just remove the comments; I think we should remove the code and the contrib module should be responsible for altering the priority of the services in the way that it needs. That should be possible, right?

Assigning to @bbrala for feedback. :)

I've been thinking about this. And although I competly agree core should not serve contrib a change in how priorities work could mean sites will break in weird ways where custom serializers might not be called anymore as before. If there are sites that have implementere their own and rely on the order/priority currently output by core wouldn't those break?

So, I'm not against removing them BUT this really really does feal like something that is a BC break which would mean we would only be able to do that in 11. I'm just very weary of randomly breaking sites, which I know noone wants.

That's my 2ct. I might be overly defensive, but I do believe we will get weird breakage in a subset of sites.

Could we deprecate the service?

Get this MR in, discuss the references to HAL in core/modules/serialization/serialization.services.yml, in a follow-up

+1 to this let's split it off since it's not just a textual reference but actual logic.

Opened #3345059: Discuss reference to HAL in serialization.service.yml

I have reviewed this and I think it is ready to commit. I will wait for other committers to comment. Otherwise, I will commit in a day or two.

A few days turned in a week.

Committed 837957e and pushed to 10.1.x. Thanks!

Work can continue in #3345059: Discuss reference to HAL in serialization.service.yml.