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:
- 100% tied to the HAL module's needs
- 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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 2854830-19.patch | 53.46 KB | Wim Leers |
Comments
Comment #2
Wim LeersAnd patch.
Comment #4
catchNot sure about this now we're in beta, rather leave it empty.
Comment #5
Wim LeersIn #2, I moved over slightly too much. And I forgot to roll with
--binary
, which is necessary when touching those darn fixture files.Comment #6
Wim LeersNow for the one unfortunate bit:
rest
module depends onserialization
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.
Comment #7
Wim Leers#4: done.
Comment #8
Wim LeersSame 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.
Comment #9
Wim LeersMade 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.
Comment #10
xjmThis is not a critical issue. There are no release blockers. It is, however, ideally RC deadline.
Comment #11
xjmComment #12
Wim LeersComment #17
Wim LeersWhat was missing, was an update to the fixture. Turns out that
protected static $modules = […];
inUpdatePathTestBase
tests is totally irrelevant.Comment #19
Wim LeersAccidental typo introduced, caused the fail in #17.
Comment #21
larowlanJust 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)
Comment #22
effulgentsia CreditAttribution: effulgentsia at Acquia commented#19's retest passed, so back to "Needs review".
Comment #23
damiankloip CreditAttribution: damiankloip at Acquia commentedThis 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.
Comment #24
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Comment #26
Wim LeersThanks! CR now also updated: https://www.drupal.org/node/2830467/revisions/view/10355881/10363796