Document the extent of JSON:API's multilingual support.

CS fix.

1. +++ b/jsonapi.api.php
   @@ -121,6 +120,14 @@ use Drupal\Core\Access\AccessResult;
   + * @section translations Translations
   

   Nit: Resource translations, because we also wrote Resource versioning. Fixed.

2. --- /dev/null
   +++ b/jsonapi.install
   …
   --- a/jsonapi.module
   +++ b/jsonapi.module
   …
   

   These changes look 👍 I think we can port these 1:1 to rest.module. Do you agree, @xjm?

Thanks for this! Two small points of feedback:

1. +++ b/jsonapi.install
   @@ -0,0 +1,34 @@
   +      $requirements['multilingual_support'] = [
   

   Let's call the machine key jsonapi_multilingual_support. I think we've had bugs before with too-generic requirements keys overwriting each other.

2. +++ b/jsonapi.install
   @@ -0,0 +1,34 @@
   +        'title' => 'JSON:API multilingual support',
   +        'value' => t('JSON:API multilingual support'),
   

   We should probably explicitly specify the severity as I always forget what the default is.

   Which leads me to the question of whether this should be info or warning, or maybe info at runtime but warning at install time. We have to be careful with warnings at runtime especially if they're something the site owner can't meaningfully address. OTOH the possibility of "unexpected results" for update operations or language negotiation changes does seem to rise to something that we should warn people about.

   Thoughts?

I pinged @gaborhojtsy and @plach for feedback on this as well.

1. ✅
2. Hm … good question! 🤔I don't feel strongly. In any case, it's indeed not going to be actionable. And it's been the reality for REST for many years, and this is just the same for JSON:API. Which makes me think "info" is appropriate. It is purely informational. The NODE ACCESS PERMISSIONS status report entry is somewhat similar, and it also is marked as "info". Went with that for now. (And "info" is the default FWIW.)

Thanks for the screenshot; that's helpful. It makes me notice that the title and description are redundant. (We've had UX issues about such before; I'd just forgotten.) Maybe the short description should be something like "Limited"? Or, borrowing from https://www.drupal.org/docs/8/modules/jsonapi/translations --something along the lines of "Simple functionality only"?

+++ b/jsonapi.install
@@ -0,0 +1,35 @@
+      $requirements['jsonapi_git multilingual_support'] = [

That's a weird key structure with both spaces and underscores. Is that intentional, with the git in there as well?

Most of the hook_requirements() keys I've seen are underscore-delimited although it might work with spaces too for all I know.

OK, turns out info does nothing at install phase, which I also often forget. So we should maybe make it a warning at install time.

#10: ✅ Agreed.
#11: ✅LOL that's the wrong window having focus and me having started to type a git command in there. I guess that happens around 1 AM 😅
#12: ✅

I wanted to add updated screenshots for #10, but especially for #12. Apparently REQUIREMENT_WARNING only has an effect when installing Drupal (or a Drupal distribution) from scratch, it doesn't trigger a visible warning at all, or a confirm form. I was under the impression that this is what you were expecting/wanting.

I just stepped through the module installation code with a debugger to figure it out: the module installation process calls drupal_check_module(), which does:

function drupal_check_module($module) {
  module_load_install($module);
  // Check requirements
  $requirements = \Drupal::moduleHandler()->invoke($module, 'requirements', ['install']);
  if (is_array($requirements) && drupal_requirements_severity($requirements) == REQUIREMENT_ERROR) {
    // Print any error messages
    foreach ($requirements as $requirement) {
      if (isset($requirement['severity']) && $requirement['severity'] == REQUIREMENT_ERROR) {
        $message = $requirement['description'];
        if (isset($requirement['value']) && $requirement['value']) {
          $message = t('@requirements_message (Currently using @item version @version)', ['@requirements_message' => $requirement['description'], '@item' => $requirement['title'], '@version' => $requirement['value']]);
        }
        \Drupal::messenger()->addError($message);
      }
    }
    return FALSE;
  }
  return TRUE;
}

As you can see, it only generates a message for REQUIREMENT_ERROR.

Also created a sibling issue for rest.module: #3037912: Document the extent of REST's multilingual support.. Marked it postponed on this issue, because once we have something we agree on here, we can just port that 1:1.

Testbot's PHPCS is drunk:

wim.leers at Acquia in ~/Work/d8 on 8.7.x*
$ composer phpcs modules/jsonapi
> phpcs --standard=core/phpcs.xml.dist --runtime-set installed_paths $($COMPOSER_BINARY config vendor-dir)/drupal/coder/coder_sniffer -- 'modules/jsonapi'
wim.leers at Acquia in ~/Work/d8 on 8.7.x*

It's telling me to remove leading spaces that need to be there 🙃I'll just put it all on one line then.

Two nits from a self-review:

1. +++ b/jsonapi.install
   @@ -0,0 +1,35 @@
   +  if (in_array($phase, ['install', 'runtime'])) {
   

   Should use strict check.

2. +++ b/jsonapi.install
   @@ -0,0 +1,35 @@
   +        'title' => 'JSON:API multilingual support',
   

   Should be wrapped in t().

Hey thanks! Still waiting from feedback from @gaborhojtsy or @plach as maintainers for the affected features, to make sure this approach is okay given that it affects REST as well as JSON:API.

Here's the updated status report warning. I confirmed it only appears when a multilingual module is also enabled.

I still can't get any message to show up when I install JSON:API either first or second. How do other modules do this? Is it only at Drupal install time rather than module install time or is something broken?

content_translation is using the messenger service in content_translation_install() to post a warning message. media_requirements() has a bunch of logic in install phase to prevent incompatible modules from being installed, but they are straight up errors.

The hook_requirements() documentation documents this:

Other severity levels have no effect on the installation. Module dependencies do not belong to these installation requirements, but should be defined in the module's .info file.

So, I guess it has to be a DSM. It's easy to add that in hook_install() for when JSON:API is installed after the modules, but what about the case where JSON:API is installed first?

FWIW @plach gave this issue a 👍and @Gábor Hojtsy said he thinks it's fine, so I think we can go ahead once the above are addressed.

I still can't get any message to show up when I install JSON:API either first or second. How do other modules do this? Is it only at Drupal install time rather than module install time or is something broken?

I don't think it ever worked in the way you thought — see #14. I knew you would ask about it, so I already researched it.

So, I guess it has to be a DSM.

DSM won't work when installed using drush or composer AFAIK? Anyway, done. It looks like this:

It's easy to add that in hook_install() for when JSON:API is installed after the modules, but what about the case where JSON:API is installed first?

🤷‍♂️ No idea. I don't think that needs to block this though? The status report still informs them.

Oops, sorry, missed #14.

Regarding the hook_install() for the other modules, I think it's important in both cases, regardless of which you turn on first. I think we have a way in core to react to other modules being enabled; I just can't remember what it is. I'll ask.

Ah, what we need is hook_modules_installed().

Oh RE: DSMs and drush, yeah, we run into this all the time. =/ Ideally we would have some sort of confirm form but I guess modules use hook_install() and friends for that instead of hook_requirements() (even though it seems like something hook_requirements() should do). But for now the DSM is a good first step.

#27: 👍OF COURSE! 😀 I completely forgot that even existed!

IS updated with screenshot for the Upon installation of language, content_translation or config_translation when JSON:API is a already installed case!

+++ b/jsonapi.install
@@ -0,0 +1,55 @@
+  if (in_array($phase, ['install', 'runtime'], TRUE)) {
...
+        'severity' => ($phase === 'runtime') ? REQUIREMENT_INFO : REQUIREMENT_WARNING,

We can simplify the code and get rid of the install phase, since we know now it does nothing.

Looks great other than that!

Things get really messed up with current multi-lang handling of JSON:API.

I have a multi-lang site with en as default/primary lang, zh-hant as translation.

With below query

/jsonapi/node/news?
filter[langcode][condition][operator]==
&filter[langcode][condition][path]=langcode
&filter[langcode][condition][value]=zh-hant
&filter[default_langcode][condition][operator]==
&filter[default_langcode][condition][path]=default_langcode
&filter[default_langcode][condition][value]=false

I am expecting an empty data array in response, becuase by implicit language neogotiation, the result should be list only en nodes, but as I am filtering with zh-hant and want translation nodes, so there should be none given.

But it gives me tons of nodes which the langcode is en and default_langcode is true, why?

Then, I think by the current logic, it make sense.

The filtering translate to sql query with results of all nodes that have translations, then the lang neogotiation kicks in, returns all these nodes in their eng state.

And this is really confusing.

It has no effect, but it does get executed during installation. drupal_check_module() just chooses to not act on it. That might change in the future (hopefully it will). But: WFM :) Fixed on commit.

#32: can you please create a new issue for that? Thanks! 🙏

_{P.S.: That sounds like an Entity Query bug, I saw a similar support request in the GraphQL Slack channel a week or two ago.}

This line still has the dead half of the ternary:
+ 'severity' => ($phase === 'runtime') ? REQUIREMENT_INFO : REQUIREMENT_WARNING,

#36: fixed in #37.