As reported at #2820096: Altered routes break between 8.1 and 8.2, this module is broken in Drupal 8.2, because of changes to REST module internals.

After having discussed this with the Acquia Content Hub team, I proposed writing a PoC patch to show how this can work.

There are some trade-offs at play when choosing to depend on the REST module or not:
- test coverage is provided by REST vs this module needs to do that
- bug fixes are provided by REST vs this module needs to do that
- bigger exposure to battle-harden vs smaller exposure

Option 1 = stick to current intent, but fix it.

Option 2, 3 4 = duplicate rest module code, plus a spectrum of choices in which URLs to use.

Option 1: only generate routes
This lets \Drupal\rest\Plugin\rest\resource\EntityResource do the bulk of the work, and introduces a decorator for the rest module's route controller that is as thin as possible. Solving #2822201: Allow contrib modules to deploy REST resources by creating routes, removing the need for config entities in that special case would allow this decorator to also be removed, but is not a prerequisite.
Consequences: minimizes changes compared to HEAD, continues to use the same URLs, continues to depend on rest module, once #2822201: Allow contrib modules to deploy REST resources by creating routes, removing the need for config entities in that special case lands 100% of the controller logic is reused from rest module. Which means you can rely on rest module's test coverage. (Which BTW I've been working on expanding, #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method should land soon, and adding test coverage for your use case will be <5 minutes of work, guaranteed.)
Option 2: duplicate EntityResource::get() and much of RequestHandler::handle() into our own controller
Allows this module to no longer depend on the rest module. At the cost of duplicating everything.
Consequences: continues to use the same URLs, removes dependency on rest module, duplicates all logic from rest module. Which means this module will need to manually track changes in rest module and port fixes over, and that you have to introduce your own test suite.
Option 3: option 2 + own URL, but keep generating old URLs/routes with auto-redirects
Consequences: option 2, but adds its own URLs, the old ones continue to work: they redirect.
Option 4: option 3, but without the old URLs
Better than option 3 if you don't need BC. One simple route in *.routing.yml, rather than generating a whole bunch of routes.
Consequences: option 2, except that it does not continue to use the same URLs, it now has its own.

Speaking as the new maintainer of the REST module, I'm confident that if you go with option 1, you will not run into any problems. You'd effectively be generating additional routes on behalf of the rest module, but you'd 100% be reusing all logic from rest module.

If you do want to remove the dependency on the REST module, that's fine & understandable. But do know that it comes with a testing & maintenance burden that is likely bigger than what you gain. I suggest that if you go in this direction, that you go all the way to option 4, which is where code becomes simplest: no more generating of routes in option 4.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
12 KB
12.18 KB
13.58 KB
10.6 KB

And here are the patches corresponding to those options.

Wim Leers’s picture

(I opted out the patches from automated testing, because the automated tests are failing.) I manually tested all patches, they all work.

scor’s picture

Thanks so much Wim for laying down all our options along with their PoC! We will go with option #1 since we'd like to leverage what's available in D8 as much as possible.

scor’s picture

Status: Needs review » Fixed

Thanks again for your help Wim. This was fixed in 8.x-1.0-beta4

Status: Fixed » Closed (fixed)

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