The module needs fixes after changes in Drupal 8.5.x core

There are some related issues, for example #2937942: REST views: regression in 8.5.x: view with display using 'csv' format now returns a 406 response, need to add ?_format=csv to URL to make it work

Drupal core 8.5.0 introduced a change (arguably a bug fix) which forces REST requests to specify the format in the query string, for example, _format=json

Here's a change record

I'm not sure what the best solution for Relaxed is. I'm waiting to see how the discussions on the core issues go.

I started to do some work on this but it's still work in Progress https://github.com/relaxedws/drupal-relaxed/pull/97. Maybe will get back to it next week.

Note that #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't would make the break less bad, or even make it disappear altogether. That being said, it looks like relaxed also make the mistake of relying on debug-only behavior (see #2937942: REST views: regression in 8.5.x: view with display using 'csv' format now returns a 406 response, need to add ?_format=csv to URL to make it work's comments of March 14 and later), rather than specifying the ?_format query string as Drupal 8 has always mandated.

@Wim Leers - We can't add a ?_format query string because relaxed needs to be compatible with the CouchDB endpoints, which don't have this query string.

Any update?

Not yet, but patches stemming from the PR in #9 are welcome.

I'm working on this issue #Nashville2018.

Hi.
My idea is to redirect requests to new URL with a query parameter _format=json
I've added new EventSubscriber (please find parch in attached file). When I open URL from a browser or Postman - it works fine (even if there are authentication credentials), but when I try to connect from another Drupal installation I see an error:

GuzzleHttp\Exception\ClientException: Client error: `GET http://admin:***@drupal8b.docksal/relaxed` resulted in a `401 Unauthorized` response: {"message":"No authentication credentials provided."} in GuzzleHttp\Exception\RequestException::create() (line 113 of /var/www/docroot/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php).

It seems that GuzzleHttp doesn't handle a redirect correctly. Now I'm looking for a solution.

Redirects will not work. CouchDB tries to handle redirects, but while redirecting it loses query parameters:
location from the response: http://drupal8.docksal/relaxed/drupal8b?_format=json
will be redirected to http://drupal8.docksal/relaxed/drupal8b
You can see here:
https://github.com/relaxedws/couchdb-client/blob/master/lib/Doctrine/Cou...
The only 'path' without query parameters is used.

I finally have found a solution. But it needs to patch Drupal Core. I've found that CouchDB adds header 'Content-type: application/json' to each request, so I've added one more condition to getContentType method for NegotiationMiddleware class. And now if the header is present, the format will be set to 'json'.
In attached files there are 2 patches:
The first patch adds _format=json to all requests which are done from Relaxed module.
The second one adds a new condition to Drupal Core.

@vg42 Don't forget to change the issue status to "Needs review" when needed.

After the discussion we had with Wim, he suggested to take a look at this issue #2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't it might be what we need to fix the problem we have with the _format parameter.

Yep. I'm currently working on a review/reroll of it, but would be great to get review support from you, to get it to RTBC and committed ASAP.

_{Note I already pointed to it in #11.}

Hi all, coming here from #2854543 and the JSON API team. We've bypassed the _format requirement in the JSON API module using our own middleware:

https://cgit.drupalcode.org/jsonapi/tree/src/StackMiddleware

and that is activated by:

https://cgit.drupalcode.org/jsonapi/tree/src/JsonapiServiceProvider.php

Finally had a chance to work on this. The solution is using a middleware that sets the request format. The idea is taken form JSON API module, as @Wim Leers and @gabesullice proposed in some discussions. With this patch all resources can be accessed and replication works.

Two tests are still failing but those fails are related to some other changes in 8.5.x. A patch with the fix for those will come next.

Results in Travis CI: https://travis-ci.org/relaxedws/drupal-relaxed/builds/376313349

We have now just one failing tests because of this core issue: #2972988: Error when saving a denormalized entity with text fields with "processed" property.

#2854543: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't landed in core. Are you sure this is still necessary?

Also, wrt how this reuses rest.module's functionality: comments welcome on #2822201: Allow contrib modules to deploy REST resources by creating routes, removing the need for config entities in that special case, which is about exactly that, and nearly unblocked!

From #27

Are you sure this is still necessary?

Yes, this is still necessary. Recent core patches don't fix the issues we have in Relaxed.

Now we need #2972988: Error when saving a denormalized entity with text fields with "processed" property to get fixed and commited.

Hm.

Reviewing post-commit because the issue was marked Needs work, not Needs review, which is why I didn't review it.

1. +++ b/config/optional/rest.resource.relaxed.all_dbs.yml
   @@ -1,13 +1,14 @@
   -granularity: method
   +granularity: resource
    configuration:
   -  GET:
   -    supported_formats:
   -      - json
   -    supported_auth:
   -      - cookie
   -      - basic_auth
   +  methods:
   +    - GET
   +  formats:
   +    - json
   +  authentication:
   +    - basic_auth
   +    - cookie
   

   All these default config changes actually are out-of-scope — they are unrelated to the problem at hand. They should've been in a separate commit. That's just optional clean-up that could've happened a long time ago. Perhaps I'm overlooking the reason for making this change now/here?

2. +++ b/src/Tests/ResourceTestBase.php
   @@ -125,7 +125,7 @@ abstract class ResourceTestBase extends RESTTestBase {
   -      $token = $this->drupalGet('rest/session/token');
   +      $token = $this->drupalGet('session/token');
   

   Same here.

It's not at all clear to me which of the changes were absolutely necessary to unbreak the module in 8.5.

Most worryingly … no test coverage at all. If there had been a patch with a failing test on this issue, I'd have chimed in. Because then I could see what exactly was not working and could figure out why. But more importantly, no tests means that a future regression in this area will again mean we only find out AFTER core has shipped a new release. Contrib modules have been able for years now to set up automated testing against the next core minor's branch, and hence be notified many months in advance of potential breakage. So early in fact, that core will instantly roll back the commit that broke it!

@Wim Leers thanks for the review!

1. and 2. I agree that those changes are not relevant to this issue, they were some experiments and should have go in a separate commit or not be done at all.

About the testing: we have more testing that run in Travis CI and it covers the issue we fixed here: https://travis-ci.org/relaxedws/drupal-relaxed/builds/380067540 (now we have 2 tests failing with Drupal 8.5.x because of the core issue).

A build before this fix: https://travis-ci.org/relaxedws/drupal-relaxed/builds/374832476
A build after this fix: https://travis-ci.org/relaxedws/drupal-relaxed/builds/380067540