Problem/Motivation

This is awkward.

#2758897: Move rest module's "link manager" services to serialization module landed on January 31. It moved the "link manager" services from the REST module to the Serialization module. This was the reasoning, written on June 30, 2016, by @larowlan:

At present serialization of entities with their references requires the link managers in rest module.
This means modules like hal and default_content rely on having rest module enabled, when all they really want to do is deserialize/denormalize entities with references intact.
We shouldn't need rest module enabled to be able to normalize/denormalize to and from hal_json or any other format that supports entity reference relations.

This is entirely sensible. Of course the HAL and default_content modules shouldn't have to rely on the rest module to be able to (de)serialize entities!

This also unblocked #2829746: Clean up \Drupal\jsonapi\LinkManager\LinkManager(Interface): the JSON API contrib module has its own types of links to add. And we were looking forward to let it add new kinds of links using the link manager. Because that's what a link manager is designed for, right? To manage links, to allow new kinds of links depending on the format, etc. And since the link manager used to live in the REST module, it was already generic. It just happens to be a serialization (hence Serialization module) task, not a routing/configuration (hence REST module) task. So we just moved it.

But, as I started to refactor the JSON API link manager to extend the REST/Serialization module link manager, I ran into some unpleasant surprises. See #2829746-12: Clean up \Drupal\jsonapi\LinkManager\LinkManager(Interface). Turns out the REST/Serialization module link manager is… not remotely generic! It's:

  1. 100% tied to the HAL module's needs
  2. absolutely not extensible: adding new kinds of links requires us to break the LinkManagerInterface, hence breaking existing implementations

Conclusion: this is not a generic link manager, this is a HAL link manager! We all assumed that it was, but it was not. Again, for the full painful detail, see #2829746-12: Clean up \Drupal\jsonapi\LinkManager\LinkManager(Interface).

Marked this critical because it should happen before the release of Drupal 8.3.0, otherwise we're stuck with the link manager in the Serialization module. If the Serialization module ever gets a link manager, it ought to be generic.

Proposed resolution

Undo the move of the REST module's link manager to the Serialization module, move it to the HAL module instead.

(With exactly the same BC precautions. The only difference is that there's now again 0 mentions of "link manager" in the Serialization module; all of those have been moved to the HAL module.)

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
57.33 KB

And patch.

Status: Needs review » Needs work

The last submitted patch, 2: 2854830-2.patch, failed testing.

catch’s picture

+++ b/core/modules/hal/hal.install
@@ -12,19 +12,19 @@
-function serialization_update_8301() {

Not sure about this now we're in beta, rather leave it empty.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
52.46 KB

In #2, I moved over slightly too much. And I forgot to roll with --binary, which is necessary when touching those darn fixture files.

Wim Leers’s picture

FileSize
3.52 KB
54.65 KB

Now for the one unfortunate bit: rest module depends on serialization module. So since #2758897: Move rest module's "link manager" services to serialization module moved the link manager from the REST module to the Serialization module, we could let the old services (in REST) redirect to the new services (in Serialization).

Hence #5 fails.

Sadly, we can't do the same anymore with this patch. We are forced to remove the old services in the REST module. Or, rather, we can only conditionally enable them: if the HAL module is also installed.

This should make the tests pass again.

Wim Leers’s picture

FileSize
702 bytes
55.94 KB

#4: done.

Wim Leers’s picture

FileSize
53.55 KB

Same patch as in #7, now rolled with -M5% (git d -M5% HEAD~4 HEAD --binary > 2854830-8.patch).

Hence just a smaller patch for the exact same changes.

Wim Leers’s picture

Made the CR at https://www.drupal.org/node/2830467 point to this issue to.

I'll update the CR as soon as this is committed.

xjm’s picture

Priority: Critical » Major
Issue tags: +rc deadline

This is not a critical issue. There are no release blockers. It is, however, ideally RC deadline.

xjm’s picture

Wim Leers’s picture

The last submitted patch, 5: 2854830-4.patch, failed testing.

The last submitted patch, 6: 2854830-6.patch, failed testing.

The last submitted patch, 7: 2854830-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2854830-8.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
53.64 KB

What was missing, was an update to the fixture. Turns out that protected static $modules = […]; in UpdatePathTestBase tests is totally irrelevant.

Status: Needs review » Needs work

The last submitted patch, 17: 2854830-17.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
53.46 KB
580 bytes

Accidental typo introduced, caused the fail in #17.

Status: Needs review » Needs work

The last submitted patch, 19: 2854830-19.patch, failed testing.

larowlan’s picture

Just chiming in to say this is good by me, default_content can rely on hal (already does) and not rest (temporarily does until 8.3 comes out)

effulgentsia’s picture

Status: Needs work » Needs review

#19's retest passed, so back to "Needs review".

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

This looks really good to me. I like how the BC services for REST module have moved into the service provider so they can be conditionally added. +1 The rest (no pun intended...) is just moving/renaming classes and namespaces.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • catch committed 55a4f98 on 8.4.x
    Issue #2854830 by Wim Leers: Move rest/serialization module's "link...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

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