Problem/Motivation

History

We added support for CRUD operations on content entities via REST without test coverage. Test coverage followed in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method. Translation support never materialized — this issue is the proof of that:

  1. this was created in November 2013 (when REST was still in heavy development).
  2. Then it was only commented on again almost a year later, in Q4 2014! From #3#25, there was a solid discussion about how this should work. But no patches.
  3. One comment in all of 2015: #26, just updating this issue summary.
  4. Then another year later, I commented, in February 2016, in #27, bumping it from Normal (GASP) to Major.
  5. Now we're another year later, and still no work happened.

As such, we have no official, and no documented way to handle translations of entities. Do we return all translations at once or not? How do you modify existing translations? And so on.

@Crell and @dixon_ outlined how they thought it should work in #21 and #22: they tried to adhere to RESTful principles as closely as possible. But alas, per the timeline: no work was done.

So, that brings us to today, where Drupal 8 has been out for more than a year (since November 2015), Drupal 8.0, 8.1 and 8.2 have been released, and 8.3's beta is being tagged in a few days. Drupal 8.3 will ship without this too. That's 4 minor releases without support for this. Time to change that. 8.4 MUST have this fixed.

How to move forward: just add test coverage?

However, that also means that we can't just do anything. We cannot break backwards compatibility. Unfortunately, the #21/#22 proposal would break BC.

As I wrote in #46:

Just had a discussion about this with @damiankloip & @tedbow. (On January 30, but only got around to finish this now.)

There are two key observations that were made:

  1. Doing the multipart approach @dixon_ suggested in #22 may very well be the most correct approach (the ideal solution), but doing so is unquestionably a BC break.
  2. Ted showed in #32 that this already works today. In a way that is natural/intuitive/guessable to a Drupal developer: via a langcode path prefix. Changing that now would also effectively be a BC break, since it's extremely likely that multilingual sites are already suing this.

So doing the ideal isolation would result in a double BC break, hence that approach must be postponed to D9.


The pragmatic solution is therefore to just expand the test coverage in EntityResourceTestBase to test all HTTP methods against all entity types in multiple languages.

Why does the existing code already kinda work? Thanks to \Drupal\Core\ParamConverter\EntityConverter::convert(), which calls getTranslationFromContext().

In other words, the ideal solution is without question off the table. But is adding tests enough? There are several problems/questions to answer:

  1. #49 points out that getTranslationFromContext() supports not just path-based language negotiation, but also domain etc. Do we want to support that?
  2. #51 points out that the HAL normalization and hence the HAL+JSON format in fact does not return a single language, but all translations of every single field. So the HAL normalization is already kind of doing what #22 wanted to do. If a particular language is requested, does that mean that HAL should return only that translation?

Proposed resolution

  1. Add test coverage for serialization-normalizer-based formats (currently only json): test that /de/node/1?_format=json returns the German translation, and also allows modifying that translation
  2. Add test coverage for hal-normalizer-based formats (currently only hal_json): test that /node/1?_format=hal_json returns all translations, and /de/node/1?_format=hal_json returns only the German translation
  3. Language negotiation mechanisms other than the path-based one: TBD

Remaining tasks

TBD

User interface changes

None.

API changes

TBD

Data model changes

None.

Comments

Gábor Hojtsy’s picture

Title: Handling entities with translations » Handling entities with translations in REST
Issue tags: +D8MI, +language-content
Greg Sims’s picture

Gábor Hojtsy’s picture

As #2257293: RESTful Multi-Language Paths says, if you do a REST request to /node/1 on your French domain you expect the French translation, no? Not the Chinese original.

Berdir’s picture

Depends on how it is storing the values. That only works if saving/updating will only ever touch that translation.

REST is often used for content staging and synchronizing data across multiple sites. You don't want to repeat that process in every language you have?

hal currently exposes the language, didn't test it yet with multiple languages, though.

klausi’s picture

Idealy we would use the Accept-language and Content-language HTTP headers to indicate the language. Not sure what we should do if you want to patch multiple languages at once, but I guess multiple requests are fine for core.

dawehner’s picture

Idealy we would use the Accept-language and Content-language HTTP headers to indicate the language. Not sure what we should do if you want to patch multiple languages at once, but I guess multiple requests are fine for core.

#2331919: Implement StackNegotiation will certainly allow to get the Accept-Language and we already set the Content-language. So basically the language negotiators
have to take into account the language figured out by the negotiation middleware.

Berdir’s picture

We already have language negotiation, that should also run and work for REST requests, why would you want to add something custom?

Crell’s picture

The Drupal language negotiation process should take into account the request language. The request language can get negotiated by StackNegotiation prior to HttpKernel even firing. It doesn't replace Drupal's custom negotiation (which takes into account things like user preferences), but supports it.

Gábor Hojtsy’s picture

So the Drupal language negotiation currently does not have the "browser language" (AKA Accept-language based) negotiation turned on by default and even if it would be the default ordering makes others take precedence. Eg. if you access fr.example.com with an accept-language of 'de', it would still be French. Looks like you are saying for REST, all other language conditions should be ignored and only the accept-language should be taken? What if that is not provided? (Technically to implement such a setup, we would need a new locked language type for REST requests and use that to negotiate the language for the REST request).

Crell’s picture

Er, no? I think we're talking past each other. I'm saying that the HTTP language negotiation is easy to add, and we can leverage it or not (we should) in whatever way makes sense within the Drupal language negotiation process.

Gábor Hojtsy’s picture

@Crell: HTTP language negotiation is already implemented and supported. It is not enabled by default because without redirection built-in it causes SEO problems at least. Where do you envision it would be "easily added" and "leveraged or not"?

Crell’s picture

#2331919: Implement StackNegotiation makes it trivial to have the HTTP language negotiated and marked on the Request object before HttpKernel is even run. In fact I think the current patch would already do that. We can then remove our own HTTP language negotiation if we have any, or make it just wrap that value. What we do it with it beyond that is not my problem. :-)

But that's really just to explain dawehner's comment in #4, and doesn't necessarily address the original question which is how to represent the data payload.

Gábor Hojtsy’s picture

@Crell: that is *surprising*. #2331919-47: Implement StackNegotiation for an extended answer.

Gábor Hojtsy’s picture

Feeding back info from there, no StackNegotiation will not be useful to negotiate a language yet, the Drupal implementation is much more battle tested.

zippydoug’s picture

I believe the child issue mentioned is related.

YesCT’s picture

Crell’s picture

Getting back to the original discussion, there's two ways this could be done:

1) Say that for REST, you get back all translations of all fields. And then you PUT all translations of all fields. If you only bother reading some of them in the mean time, good for you.

2) Make REST Accept-Language aware for both GET and PUT. That is,

GET /node/1
Accept: application/hal+json
Accept-Language: de

Would return ONLY the German version of a node (give or take whatever rules we already have for non-translated fields and fields with a missing translation), and to write you'd have to do:

PUT /node/1
Content-Type: application/json
Language: de

{ ... }

From a REST perspective, actually, using out-of-band session information (like a user preference) would actually be incorrect; every request SHOULD contain all the information it needs in the request itself without any other server-side state. So it would be appropriate in this case to rely solely on the Accept-Language/Language headers and nothing else.

Option 2 is, I think, more RESTfully correct but probably also more work. :-(

Greg Boggs’s picture

Assuming you go with option 2, if you ask for zh-hans/node/1 in JSON without specifying a language, what language would the results be? Would there even be results at all?

Crell’s picture

Option 2 would say that results in either a 404 or a 406 error, I believe.

Gábor Hojtsy’s picture

Spaces and domain access will change some things I guess, if node/1 is only accessible on domain de.example.com and not en.example.com then no common REST endpoint will exist (as with path prefixes), but domain access is used anyway to make sites appear they are different sites, so different entry points for REST are not surprising.

As for option 1 or 2 I can imagine both having its uses. Is there a way we can satisfy both? For example if there was no Accept-language, we work with the whole entity and not some hardwired fallback language?

Crell’s picture

From a REST perspective, different domains => different resources. The amount of unholy things we do to support browsers is truly mind-boggling. :-(

I *think* the following logic would be REST-acceptable:

if (Accept-Language/Language header) {
use that and only that, ignoring all other configuration
}
else {
send/receive the whole object with all languages
}

We should probably verify that. That said, I am concerned about someone sending an object in just one language but without a Language header. That would probably get interpreted as "delete all languages but this one", which presumably we do not want.

Unless it were somehow a serialization error, because the language keys on fields are missing?

dixon_’s picture

From a REST perspective, different domains => different resources.

Completely agree. We should not go ways to support languages in the domain/path in any way. That's going against restful principles.

if (Accept-Language/Language header) {
use that and only that, ignoring all other configuration
}
else {
send/receive the whole object with all languages
}

The problem I see with this approach is that the structure of the entity fields would have to change to support send/receive all languages in one whole object. Although it might be restful it would complicate things for clients and consumers of the API having to handle different object structures.

So my take on this would be:

If Accept-Language/Language headers are available, send/receive the entity object as is.

If Accept-Language/Language headers are NOT available, send/receive multipart/related (see MIME#Related on Wikipedia) with Transfer-Encoding: chunked.



A simplified GET request would be:

GET /node/1
Accept: multipart/related



Response:

Content-Type: multipart/related; boundary="12345delimiter"
Transfer-Encoding: chunked

--12345delimiter
Content-Type: application/json
Language: en

{
  id: 1,
  title: [{value: "english title"}],
  body: [{value: "english text"}]
}
--12345delimiter
Content-Type: application/json
Language: se

{
  id: 1,
  title: [{value: "svensk titel"}],
  body: [{value: "svensk text"}]
}
--12345delimiter--



A simplified PUT request would be:

PUT /node/1
Accept: application/json
Content-Type: multipart/related; boundary="12345delimiter"

--12345delimiter
Content-Type: application/json
Accept-Language: en

{
  id: 1,
  title: [{value: "english title"}],
  body: [{value: "english text"}]
}
--12345delimiter
Content-Type: application/json
Accept-Language: se

{
  id: 1,
  title: [{value: "svensk titel"}],
  body: [{value: "svensk text"}]
}
--12345delimiter--



It may seem a bit complicated, but it's properly restful and most robust HTTP clients should support this type of responses nicely. Each "chunk" is simply a normal entity object translated in the language specified by the headers.

Gábor Hojtsy’s picture

@dixon_: I think that makes total sense :)

dixon_’s picture

I noticed that neither Symfony HttpFoundation or Guzzle had support for multipart/* requests and responses (except multipart/form-data which is not what we're looking for).

So I quickly created a set of components to deal with that, see:

This could serve as a demonstration of how we could implement support for multipart requests and responses in our REST implementation.

Gábor Hojtsy’s picture

That's unfortunate if we could not output or test those in core without more libraries :/ Sounds like this would be blocked for adding more libraries if we want to go that route.

clemens.tolboom’s picture

Wim Leers’s picture

Title: Handling entities with translations in REST » Supported entities with translations in REST
Priority: Normal » Major

I think this is at least major. AFAICT without this issue, Drupal 8 does not support working with translated entities via REST. Even though that's one of Drupal's strengths overall. That makes this a very important missing link.

vasi’s picture

The steps to reproduce match those for the issue I'm working on now: https://www.drupal.org/node/2664880 . I'm not sure about the other parts of this issue, so maybe 2664880 should be a sub-issue?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Supported entities with translations in REST » EntityResource: translations support
andypost’s picture

Version: 8.1.x-dev » 8.2.x-dev
tedbow’s picture

Just wanted to comment here that I am not getting the same results as the issue summary.

This is the behaviour I am seeing for:

  1. 2 languages English and Spanish
  2. 1 node with original language English and is translated into Spanish

If create REST export for the front page view
curl -H 'Accept: application/json' --request GET http://d8/es/node
this does return the spanish results.

for the single node GET also
curl -H 'Accept: application/json' --request GET http://d8/es/node/1
Does return the Spanish results.
Where as
curl -H 'Accept: application/json' --request GET http://d8/node/1
Returns the English results

Furthermore if I login, send cookies along with the request and set the "account's preferred language" then the corresponding language of the node and view is returned.

From the comments in this issue it sounds like this is not the desired behavior.

But if we decide to fix it and implement another solution do we consider that sites might be currently depending on the current behavior. Or since the current behavior is not documented anywhere is it ok to break/fix it.

Also hal_json gives me a different behavior. For every node response in the object it gives both english and spanish field values:

"body": [
    {
      "value": "<p>english</p>\r\n\r\n<p>&nbsp;</p>\r\n",
      "format": "basic_html",
      "summary": "",
      "lang": "en"
    },
    {
      "value": "<p>spanish body</p>\r\n",
      "format": "basic_html",
      "summary": "",
      "lang": "es"
    }
  ],

I have attached the full response. So in this case
http://www.octo2.dev/d8_rest_lang/node/1?_format=hal_json and http://www.octo2.dev/d8_rest_lang/es/node/1?_format=hal_json

give the same response

tedbow’s picture

FileSize
5.57 KB

Forgot to attach file with full hal_json response. Unlike json the response is the same regardless of language provide in path or user account.

Gábor Hojtsy’s picture

Yeah I don't know what the correct/desired solution is, but it is comforting that at least the translations are somehow accessible now. The HAL response looks very confusing though :/

tedbow’s picture

@Gábor Hojtsy yes I found it weird to but maybe this expected behavior.

Looking at \Drupal\hal\Normalizer\FieldNormalizer it seems merging the field values from all languages is intentional

foreach ($entity->getTranslationLanguages() as $language) {
        $context['langcode'] = $language->getId();
        $translation = $entity->getTranslation($language->getId());
        $translated_field = $translation->get($field_name);
        $normalized_field_items = array_merge($normalized_field_items, $this->normalizeFieldItems($translated_field, $format, $context));
      }
dawehner’s picture

Right, well, in a rest world, the resource, for example /node/1 should have ideally one representation. Yes, this one representation might be quite verbose, but at least its one. In this logic the verbose HAL_JSON output, is not the worst.

Gábor Hojtsy’s picture

All right, should we consider the HAL JSON implementation as reference? In that case, sounds like we do need https://packagist.org/packages/dickolsson/http-multipart or an equivalent so return multiple variants at the same time and we need to expect clients to deal with that fine?

Gábor Hojtsy’s picture

for the single node GET also

curl -H 'Accept: application/json' --request GET http://d8/es/node/1

Does return the Spanish results.
Where as

curl -H 'Accept: application/json' --request GET http://d8/node/1

Returns the English results

Furthermore if I login, send cookies along with the request and set the "account's preferred language" then the corresponding language of the node and view is returned.

From the comments in this issue it sounds like this is not the desired behavior.

So do we want REST to require authentication to be able to specify language in some other way than the regular language negotiation? And even then, to me it looks like it works for you because you configured your language negotiation in a specific way, no? I think more details on your setup would be nice to see what you have / what you observed.

dawehner’s picture

In that case, sounds like we do need https://packagist.org/packages/dickolsson/http-multipart or an equivalent so return multiple variants at the same time and we need to expect clients to deal with that fine?

In case we want that, we certainly would want it to be an opt in. Supporting for example a language prefix support IMHO is kinda what people expect.

Gábor Hojtsy’s picture

@dawehner: but what's a thing though? I mean if you update an entity and it has untranslated fields, it will have side effects on other translations. Is that what is expected in REST to have such side effects or are you supposed to deal with the whole entity as one? Would dealing with the whole entity as opposed to a translation be an opt-in thing?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: EntityResource: translations support » [PP-1] EntityResource: translations support
Status: Active » Postponed
Related issues: +#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method

Considering the number of (broken) edge cases I'm finding while working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, I think this issue needs to be postponed on that one. Because this would introduce the wiggle room for even more edge cases. When we add this, we need firm, reliable, comprehensive test coverage. We can't achieve that without #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method.

Wim Leers’s picture

Title: [PP-1] EntityResource: translations support » EntityResource: translations support
Status: Postponed » Active
Wim Leers’s picture

Note that JSON API committed a solution for this: #2794431: [FEATURE] Provide GET support for translated entities. However, it violates the concerns that dixon_ raised at #22. It violates this:

We should not go ways to support languages in the domain/path in any way. That's going against restful principles.

But for JSON API, this may be acceptable — see #2794431-35: [FEATURE] Provide GET support for translated entities.

We should look into how we can move forward here. It's really a huge gap that our REST API doesn't support reading and modifying translated entities. Thanks to #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, no matter what direction we choose, we will be able to have solid test coverage.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Just had a discussion about this with @damiankloip & @tedbow. (On January 30, but only got around to finish this now.)

There are two key observations that were made:

  1. Doing the multipart approach @dixon_ suggested in #22 may very well be the most correct approach (the ideal solution), but doing so is unquestionably a BC break.
  2. Ted showed in #32 that this already works today. In a way that is natural/intuitive/guessable to a Drupal developer: via a langcode path prefix. Changing that now would also effectively be a BC break, since it's extremely likely that multilingual sites are already suing this.

So doing the ideal isolation would result in a double BC break, hence that approach must be postponed to D9.


The pragmatic solution is therefore to just expand the test coverage in EntityResourceTestBase to test all HTTP methods against all entity types in multiple languages.

dawehner’s picture

The pragmatic solution is therefore to just expand the test coverage in EntityResourceTestBase to test all HTTP methods against all entity types in multiple languages.

+1

Note: This relies on our language negotiation service, so its actually configurable.

Here is a question: could we have a "translation" link relation type which exposes all the other translations?

Gábor Hojtsy’s picture

Right, if we want to be backwards compatible and consider the language based URLs as a valid way to access certain representations of an entity, then unifying under single entry points is not an option anymore.

Wim Leers’s picture

Note: This relies on our language negotiation service, so its actually configurable.

Indeed. So it is possible that a site is using fr.example.com and cn.example.com, and hence allows fr.example.com/node/42 to be read in Chinese at cn.example.com/node/42. Is that really a problem? Is that still a problem if the necessary link relationships are present?

Here is a question: could we have a "translation" link relation type which exposes all the other translations?

+1. I looked through https://www.iana.org/assignments/link-relations/link-relations.xhtml, but there's no translation metadata there.

#48: thanks for confirming!

Berdir’s picture

I think it shold be possible to define a custom format that would include all translations, then that could be done in contrib or even core? That would allow to implement and test that already, which would make getting it into core easier.

Keep in mind that fetching a certain translation for an entity is the easy part, Creating and updating is already more challenging, not sure if we already have code that deals with that. And deleting a translation is something that we most certainly don't support yet, we'll have to do define something like DELETE on a different translation than the default will only delete translation, or something like that. Technically deleting a translation is removing it from the entity and then normally saving it.

My point is that I'm 99% sure that it is *not* just adding test coverage :)

Wim Leers’s picture

custom format

Sure, you can do that. But another format to maintain? That means even more normalizers to maintain, even though the existing ones are already flawed (see the "serialization gap" issues at the top of #2794263: REST: top priorities for Drupal 8.3.x. Who's going to do that? I'm certainly not going to.

This reminds me of something though: @damiankloip pointed out during that same meeting that I reported back on in #46 that the HAL+JSON format actually already includes all translations! However, it does this at the field level. Every field has a 'lang' => 'en' right now, and our test coverage asserts that.

The code also shows this very clearly — quoting \Drupal\hal\Normalizer\FieldNormalizer::normalize():

    // If this field is not translatable, it can simply be normalized without
    // separating it into different translations.
    if (!$field_definition->isTranslatable()) {
      $normalized_field_items = $this->normalizeFieldItems($field, $format, $context);
    }
    // Otherwise, the languages have to be extracted from the entity and passed
    // in to the field item normalizer in the context. The langcode is appended
    // to the field item values.
    else {
      foreach ($entity->getTranslationLanguages() as $language) {
        $context['langcode'] = $language->getId();
        $translation = $entity->getTranslation($language->getId());
        $translated_field = $translation->get($field_name);
        $normalized_field_items = array_merge($normalized_field_items, $this->normalizeFieldItems($translated_field, $format, $context));
      }
    }

My point is that I'm 99% sure that it is *not* just adding test coverage :)

Point taken! But at least for GETting translations, it is 99% adding test coverage, since it is already working. Will we discover edge cases that we need to worry about? I have no doubt about that. But certainly the first step on this issue should be adding test coverage for reading translations, even if only to ensure that we don't regress there while we work on supporting PATCH/POST/DELETE.

And as I've basically just shown, for HAL+JSON, it's already going to be more complicated than "just adding test coverage".

Berdir’s picture

Sure, you can do that. But another format to maintain? That means even more normalizers to maintain, even though the existing ones are
already flawed (see the "serialization gap" issues at the top of #2794263: REST: top priorities for Drupal 8.3.x. Who's going to do that? I'm
certainly not going to.

Yeah, just thinking out loud. We're currently supporting someone who is starting a multilingual project and they will likely need a service that returns all translations together for custom entity types. So it is either that or completely custom controllers, which might actually be easier in the end for them.

Wim Leers’s picture

IS updated.

Wim Leers’s picture

Issue summary: View changes

My proposal:

  1. Add test coverage for serialization-normalizer-based formats (currently only json): test that /de/node/1?_format=json returns the German translation, and also allows modifying that translation
  2. Add test coverage for hal-normalizer-based formats (currently only hal_json): test that /node/1?_format=hal_json returns all translations, and /de/node/1?_format=hal_json returns only the German translation
  3. Language negotiation mechanisms other than the path-based one: TBD
Berdir’s picture

2) Add test coverage for hal-normalizer-based formats (currently only
hal_json): test that /node/1?_format=hal_json returns all translations,
and /de/node/1?_format=hal_json returns only the German translation

That's not how language negotation works. There is always *one* current content language. If you don't have a prefix, then this only means that your default language doens't have a prefix, which is the default but very uncommon setting. Either we always return all languages or we always only one.

3. Language negotiation mechanisms other than the path-based one: TBD

As Gabor mentioned, this is a non-issue. Language negotation just works. All we need to do is respect the current language. If we actually want to.

I will start with hal because that's what we're going to need.

Berdir’s picture

Status: Active » Needs review
FileSize
5.06 KB

Here's a start with a node that has a translation for hal+json. Had some troubles with the Link header and content_translation links, looks like we forgot to define them properly?

testGet() is passing with a second translation, not quite sure yet what to do about testPost() and testPatch().

Berdir’s picture

Started with a POST test, basically trying to create an entity with two translations, not expecting that to actually work.

But I'm running into some sort of problem right now, with the permissions no longer being there.. in fact, the *roles* no longer being there.. Hoping that @Wim has some clues..

Status: Needs review » Needs work

The last submitted patch, 57: rest-translations-hal-2135829-57.patch, failed testing.

Wim Leers’s picture

#55

  1. Right!
  2. Well, people were very concerned that we'd be supporting subdomain-based language negotiation. Whether we want to exclude certain language negotiation mechanisms like subdomains, is something that is TBD. Although perhaps that ship has sailed too then.

#56: Regarding content_translation link relation types: answer below. testGet() is passing: YAY. testPost() and testPatch(): we should be able to modify a subset of translations or all translations? So we should test both. Or should we not be able to modify all translations? Should HAL just allow you to read all translations?

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonCookieTranslationsTest.php
    @@ -0,0 +1,122 @@
    +  protected static $patchProtectedFieldNames = [
    +    'created',
    +    'changed',
    +    'promote',
    +    'sticky',
    +    'revision_timestamp',
    +    'revision_uid',
    +  ];
    

    You can omit this, since it matches what is inherited from NodeHalJsonAnonTest::$patchProtectedFieldNames()

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonCookieTranslationsTest.php
    @@ -0,0 +1,122 @@
    +  protected function getExpectedCacheContexts() {
    +    return [
    +      0 => 'languages:language_content',
    +      1 => 'languages:language_interface',
    +      2 => 'url.site',
    +      3 => 'user.permissions',
    +    ];
    +  }
    

    Let's change this to:

    return Cache::mergeContexts(
      parent::getExpectedCacheContexts(),
      ['languages:language_content', 'languages:language_interface']
    ); 
    
  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -435,12 +435,16 @@ public function testGet() {
    +      // @todo What to do about content_translation link relationships?
    +      if (strpos($rel, 'drupal:content-translation') !== FALSE) {
    +        return FALSE;
    +      }
    

    Add them to core.link_relation_types.yml. Or actually, add them to content_translation.link_relation_types.yml :)


#57:
This is failing with the following error message: "{"message":"Access denied on creating field \u0027langcode\u0027."}". AFAICT this is caused by language_entity_field_access() only allowing access if specific conditions are met.

Gábor Hojtsy’s picture

@Wim: even when you update a translation, there is good chance there are non-translatable fields, which are the same for all translations, so you have a bit of a side effect for your post/patch request.

Wim Leers’s picture

Right. All the more reason to have good test coverage showing that. Or am I not understanding what you mean by that?

Berdir’s picture

Ah, langcode makes sense, will look into that. It is behavior extremely weird for me locally, somehow connecting to a wrong database or something, which could be caused by debugging... so apparently the error is different whether I debug it or not. Fun...

Or should we not be able to modify all translations? Should HAL just allow you to /read/ all translations?

Modify is *exactly* what I need, Get was just the first step. And changing values should be relatively easy. Add will definitely not work yet,

Now I got the access problems sorted out and it's now failing as I expected: With a "500 Internal Server Error" (Since we now have nice 403 responses, wouldn't it be nice to have nicer 500 responses too? :)). Will look more into that a bit later.

Wim Leers’s picture

somehow connecting to a wrong database or something

WTF?! :O

Since we now have nice 403 responses, wouldn't it be nice to have nicer 500 responses too? :)

Yes, but … the point is that it's such a severe error that all we can do is send a 500 response. So there's a pretty strong contradiction in there. Most of the time, we can't/don't want to expose stack traces!
Add this before the currently failing assertion:

var_dump((string)$response->getBody());

Status: Needs review » Needs work

The last submitted patch, 62: rest-translations-hal-2135829-60.patch, failed testing.

Berdir’s picture

I did exactly that var dump, but that just contains "Internal Server Error" :)

Most of the time, we can't/don't want to expose stack traces!

Actually, I think that's exactly what we want: To respect the same setting as we have for HTML: None, All, Verbose (include stack trace)?

Wim Leers’s picture

I think that's related to \Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware. It very easily and very frequently gets in the way.

Berdir’s picture

See the change in EntityResource. Drupal is bad with nested/previous exception handling right now, removing that suddently results in a helpful and easy to fix exception. I haven't tested what is shown outside of tests/with disabled error logging, but as far as I see, this is a weird hack that should be solved elswhere if it doesn't work already, because other exceptions would show up there. We can move that into a separate issue to discuss further, but it helped me a lot in debugging this, so keeping it for now.

And POST works now that I removed the identifer fields. So far so go...

Also noticed that we don't seem to be doing any assertions on whether post/patch *actually* works? In my POST test, I'm checking if the title is there in the translation, the generic methods all only assert on the response code, location header and so on. As far as I see, we could simply not save at all on PATCH and the existing test would happily pass?

Started with a PATCH test as well, and here we seem to have our first actual test fail.. Looks like the german translation is not added/created on PATCH, but it does work on POST. Which, to repeat my point from the previous paragraph, we only see because I'm actuall asserting that something happened, not just that we didn't get negative error code back?

Berdir’s picture

Had a look at what it takes to support it, not trivial obviously. Here's a first step, but it is failing badly on non-translation PATCH requests and validation on translations also explodes, apparently on fields having more than one value, not sure what this is yet.

The last submitted patch, 67: rest-translations-hal-2135829-67.patch, failed testing.

Wim Leers’s picture

As far as I see, we could simply not save at all on PATCH and the existing test would happily pass?

You're right. The test coverage does assume that status codes don't lie. So far, that's been good. But with the need to support translations, we obviously need to make the tests more strict.

Berdir’s picture

You can't assume anything in a test, I thought that's the point of writing tests? :) I think there should be assertPatchedEntity()/assertPostedEntity() and similar methods that subclasses can override and verify that things are working as expected. That's not specific to translations at all.

Status: Needs review » Needs work

The last submitted patch, 68: rest-translations-hal-2135829-68.patch, failed testing.

Wim Leers’s picture

You can't assume anything in a test, I thought that's the point of writing tests? :) I think there should be assertPatchedEntity()/assertPostedEntity() and similar methods that subclasses can override and verify that things are working as expected. That's not specific to translations at all.

Eh… I'm sorry that not every possible edge case was tested in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, which was a mere 190 KB patch that took months to get to RTBC and uncovered >20 unreported bugs. I spent dozens and dozens of hours on that test coverage patch, which should have happened years ago, i.e. when REST landed.

To be clear: I'm not mad at you, but that statement was very unfair. Things are the way they are. We make leaps and baby steps in the direction of more stability.


To help get you started…

We already have code that does this in testGet():

    // Comparing the exact serialization is pointless, because the order of
    // fields does not matter (at least not yet). That's why we only compare the
    // normalized entity with the decoded response: it's comparing PHP arrays
    // instead of strings.
    $this->assertEquals($this->getExpectedNormalizedEntity(), $this->serializer->decode((string) $response->getBody(), static::$format));

So you'll want to add assertions like that after successful POST and PATCH requests. And you'll want to add assertions that load the entity object and also assert the field values from there.

Berdir’s picture

Copy & Paste from IRC, in case you didn't see that:

sorry, my comment wasn't meant to be an attack on you or something like that.

If anything, I was just a bit surprised to not see any asserts on created/updated entities after all those invalid/negative tests/assertions.

We clearly have a different process, because I started with a successful test with explicit success assertions, while you started from nothing and then step-by step enable things and test that things do not work the way we expect them to not work.

And yes, something like that would make sense. Doesn't need to happen here though.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
20.08 KB

We clearly have a different process, because I started with a successful test with explicit success assertions, while you started from nothing and then step-by step enable things and test that things do not work the way we expect them to not work.

I also generally start with successful tests, but in this case, I was turning thin & spotty test coverage into something more reliable. The process I have there was mostly: think about all the possible mistakes a developer writing a client talking to the D8 REST API might make, and ensure that we have error responses that guide them towards successful requests. Because that was the #1 complaint about REST API: endless failing requests, with no way to figure out how to get them to be successful, other than firing up Xdebug and stepping through all the code.

Anyway, enough about that. Let's focus on this issue again.


+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -180,27 +182,22 @@ public function post(EntityInterface $entity = NULL) {
-    try {
...
-    }
-    catch (EntityStorageException $e) {
-      throw new HttpException(500, 'Internal Server Error', $e);
-    }

Because this try/catch is removed, one assertion in \Drupal\Tests\rest\Functional\EntityResource\Comment\CommentResourceTestBase::testPostDxWithoutCriticalBaseFields() must be updated.

That fixes most failures.

Wim Leers’s picture

Status: Needs review » Needs work

The actual failing test is failing with this error:

"{"message":"Unprocessable Entity: validation failed.\nlangcode: Language: this field cannot hold more than 1 values.\nstatus: Publishing status: this field cannot hold more than 1 values.\nuid: Authored by: this field cannot hold more than 1 values.\ncreated: Authored on: this field cannot hold more than 1 values.\npromote: Promoted to front page: this field cannot hold more than 1 values.\nsticky: Sticky at top of lists: this field cannot hold more than 1 values.\n"}"
dawehner’s picture

I think its wrong from a BC point of view to limit our support to content entities, but meh, we would need to rewrite that stuff for config entities anyway, I guess.

I'm trying to look into the remaining test failure.

dawehner’s picture

One problem seems to be that \Drupal\serialization\Normalizer\FieldNormalizer::denormalize appends additional items, so in that case somehow we end up with two values per language.

I'll continue exploring.

Berdir’s picture

#77: I was just changing that because it was annoying to not have proper type hints while I worked on this. We can definitely make it optional. That said the code that is there right now on patch() clearly needs at least FieldableEntityInterface, we're lying to ourself right now :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
20.52 KB

Rebased. That was a tough rebase.

Status: Needs review » Needs work

The last submitted patch, 80: rest-translations-hal-2135829-80.patch, failed testing.