Skip to content

Conversation

@Auspicus
Copy link
Contributor

@Auspicus Auspicus commented Aug 31, 2020

Description

We're currently using gatsby-source-drupal to pull content from our D8 site and we got stuck trying to pull content from another language because it gets overwritten by the last source plugin in the list. eg. gatsby-config.js

...
{
  resolve: `gatsby-source-drupal`,
  options: {
    baseUrl: `http://example.com`,
  },
},
{
  resolve: `gatsby-source-drupal`,
  options: {
    baseUrl: `http://example.com/es`,
  },
},
...

Proposed Solution

  • Enable translation support by a languageConfig object to the options.

eg. new gatsby-config.js

resolve: `gatsby-source-drupal`,
options: {
  baseUrl: `http://www.example.com`,
  languageConfig: {
    defaultLanguage: `en`,
    enabledLanguages: [`en`, `fil`],
    translatableEntities: [`node--article`],
  },
},

Related Issues

Addresses #10020


Replaces a #26642 PR which did this in a more rudimentary way.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 31, 2020
Copy link
Contributor Author

@Auspicus Auspicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments on areas for improvement. Also open to using lodash instead of native JS APIs for cleanliness and consistency. Thoughts @smthomas? I tested this with fastbuilds and it's looking positive. It worked in default language and my secondary language (ES in my case).

} else {
const baseUrlWithoutTrailingSlash = baseUrl.replace(/\/$/, ``)
const urlPath = url.href.replace(`${baseUrlWithoutTrailingSlash}/${apiBase}/`, ``)
dataForLanguage = await getNext(`${baseUrlWithoutTrailingSlash}/${currentLanguage}/${apiBase}/${urlPath}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to handling this in a different way or using a library for the URL parsing. I don't like having to manually edit the URL and it's error prone.

@Auspicus
Copy link
Contributor Author

Closing #26642 in favor of this.

@madalynrose madalynrose requested a review from smthomas September 1, 2020 18:39
@madalynrose madalynrose added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 1, 2020
store.getState().status.plugins?.[`gatsby-source-drupal`]?.languageConfig
|| await fetchLanguageConfig({ translation, baseUrl, apiBase, basicAuth, headers, params })
)
setPluginStatus({ languageConfig })

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further testing seems to prove that this won't affect other status values.

.data
.data
.map(entity => ({
type: entity.attributes.target_entity_type_id,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need "type" or "bundle" since we are using "id". Might be better to just create a list of type strings preformatted eg. node--article

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just notice: in Drupal we can alter that, we do not need to use type--bundle naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmajzlik Do you mind explaining that a bit more? I'm just proposing removing the extra properties here and changing the returned value from an object to a string matching the existing "id" property.

@KenBoone23
Copy link

Hi @Auspicus, we are looking forward on this fix, since we need it in one of our new projects. Any idea when this could be merged?

.map(language => language.attributes.drupal_internal__id)
)

const defaultLanguage = enabledLanguages[0]
Copy link

@Ayrm4x Ayrm4x Sep 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default language is not always the first in the array.
I think the site default language value is not exposed via jsonapi. We might have to set it in the gatsby-config.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll bring this up in the #contenta slack channel tomorrow to see if anyone has ideas on how to get the default langauge via the JSON:API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my understanding that the lowest weight language will be default so perhaps the request just needs to be sorted by weight.

Copy link

@Ayrm4x Ayrm4x Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the request url to /configurable_language/configurable_language?sort=weight and it seems to work. I am testing the package now. It's looking good so far. Good job! Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update this patch to include your changes.

@Auspicus
Copy link
Contributor Author

Auspicus commented Sep 15, 2020

Hi @Auspicus, we are looking forward on this fix, since we need it in one of our new projects. Any idea when this could be merged?

I'm working with the Gatsby team to get this in. @KenBoone23 in the meantime you can use this as a patched version of the original node module if you'd like. You can download it from Github via your npm install.

@Ayrm4x
Copy link

Ayrm4x commented Sep 15, 2020

I found a problem with the fastbuilds feature.
The delete action can't be performed anymore because in line 170 the nodeid is not created with the langcode pattern.

actions.deleteNode({ node: getNode(createNodeId(nodeSyncData.id)) })

Even if we would change the line to
actions.deleteNode({ node: getNode(createNodeId(`${nodeSyncData.langcode}${nodeSyncData.id}`)) })
it doesn't work because the 'delete' response does not contain a langcode.

I created a feature request: https://www.drupal.org/project/gatsby/issues/3171054

With the changed line and the patch from my issue at drupal.org the feature is working.

The result of the response would look like this:

        {
            "id": "429335f5-d636-4235-bd62-88fe4905922e",
            "action": "delete",
            "langcode": "en"
        }

@Auspicus
Copy link
Contributor Author

Thanks @Ayrm4x for that patch to the Gatsby module! Good catch! 🎉 Seems to make sense. Would be interesting to hear @smthomas's take on this.

@Ayrm4x
Copy link

Ayrm4x commented Sep 16, 2020

By digging deeper I figured out that we might have to update all language versions of the node on gatsby side at fastbuilds because language versions might sync fields and stuff.

@Ayrm4x
Copy link

Ayrm4x commented Sep 16, 2020

I updated the patch for the gatsby_fastbuild module: https://www.drupal.org/project/gatsby/issues/3171054#comment-13825325

With the id creation changes in the gatsby fastbuilds feature and the patched "gatsby_fastbuild" module, basic update and delete actions should work.

If we ignore syncs between translations on drupal side we should be fine.

@hvermeire
Copy link

We have a project where we use gatsby-source-drupal with Drupal paragraphs.
When I linked this PR in the project, we lost access to the paragraphs. Here's a simple example:

query MyQuery {
  allNodeArticle {
    nodes {
      id
      relationships {
        field_paragraphs {
          __typename
        }
      }
    }
  }
}

Some articles have Paragraphs attached to them. With this PR linked, I get this graphql error: Cannot query field "relationships" on type "node__article".. When I unlink it and return to the current (3.5.33) version of the plugin, the query works as expected.

@Auspicus
Copy link
Contributor Author

@hvermeire Interesting find. What's the language configuration for that paragraphs field? eg. Is it set to be translatable?

@hvermeire
Copy link

hvermeire commented Sep 16, 2020

I tried both translatable true and false.

EDIT: Sorry, I used the setting from the initial post:

    {
      resolve: `gatsby-source-drupal`,
      options: {
        preview: true,
        baseUrl: `<snip>`,
        translation: false,
      },
    },

Should I use translatable instead of translation @Auspicus?

@Auspicus
Copy link
Contributor Author

Auspicus commented Sep 16, 2020

The field for the gatsby-config file should be translation: true. You can see here: https://github.com/gatsbyjs/gatsby/pull/26720/files#diff-35492dcbc33980cf1dc8f072ec186c62R52

As for the field settings in Drupal that will depend on whether or not you provide translations for each language you have enabled or if the field is not translatable then it should show up the same in both languages. Either way it should work but I haven't tested Paragraphs yet so you might have stumbled across some bug with the patch. We might need Paragraphs in our Drupal installation so when I get around to adding that I'll take a deeper look.

In the meantime you're welcome to propose any changes here you think might help. Cheers! 🎉

Copy link
Contributor

@smthomas smthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking awesome! I left a few comments on things that still need to get it working. It seems like it works when multilingual is turned on for entities, but there may be a few instances that it stops working when entities do not have language selection enabled.

We will also need to fix some minor linting issues to get the tests to pass. In the Gatsby repo you can run yarn run lint to see the errors and then fix them for any that show up inside gatsby-source-drupal.

headers,
params,
}),
axios.get(`${baseUrl}/${apiBase}/language_content_settings/language_content_settings?filter[language_alterable]=true`, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor note is that JSONAPI will only return 50 results. In the off chance the site has more than 50 languages enabled or more than 50 entity types with language selection enabled, this will miss any that are not on the first page.

I think this is an edge case, but might be worth accounting for now. We would need to check that data.links.next does not have a URL. If it does then use axios.get and combine the results.

const referenceLanguagePrefix = isTranslatableReferencedNodeType
? rootNodeLanguage
: languageConfig.defaultLanguage
const referencedNodeId = createNodeId(`${referenceLanguagePrefix}${data.id}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why we can't just use createNodeId(${rootNodeLanguag}${data.id}) here and drop lines 81 through 88.

If the entity in Drupal is translatable it uses the rootNodeLanguage value anyway. However, if the entity is not translatable it uses the defaultLanguage which would be und. The problem with this approach is that in normalize.js it uses the langcode to construct the node id. This means that these references don't look like they will work properly for nodes that are not translateable.

For instance, let's say I have an article content type that does NOT have languages enabled. Assume an article that is language en with UUID 1234-1234 and it references a file with language en and UUID abcd-abcd.

The nodes will be first created as en1234-1234 and enabcd-abcd in normalize.js. Now when it is trying to handle the references the article will try to create a reference using languageConfig.defaultLanguage (which is und) so it looks for undabcd-abcd which isn't going to correctly reference the file.

If we just set the variable up above to const rootNodeLanguage = node.langcode ? node.langcode : und`` and get rid of this extra logic I think it works for both scenarios (translations on and translations off).

I could be missing something here though, so jump in if it needs further clarification. Either way, I can confirm this seems to break on entities without language selection turned on that reference other entities without language selection turned on.

const referenceLanguagePrefix = isTranslatableReferencedNodeType
? rootNodeLanguage
: languageConfig.defaultLanguage
const referencedNodeId = createNodeId(`${referenceLanguagePrefix}${v.data.id}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See same note as above.

typeof _attributes_id !== `undefined` ? { _attributes_id } : {}
return {
id: createNodeId(datum.id),
id: createNodeId(`${datum.attributes.langcode}${datum.id}`),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance, it seems like all entities have a attributes.langcode attribute. However, I have found that for some reason JSONAPI does not always return this langcode value if you are browsing as an anonymous user. For an example try going to /jsonapi/date_format/date_format as an authenticated and an anonymous user. For me I see the langcode when logged in but not when anonymous. This breaks builds if the user is not using authentication.

The simple workaround here is to just check if the langcode exists and if it doesn't, default to und

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is related to fact that date_format is config entity in Drupal. So it may be related to enabling config_translation module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a few entities that seem to have no langcode, mostly internal entities. I'll make sure to review this during my next pass.

Copy link

@kmajzlik kmajzlik Oct 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically there are 2 types of entities in Drupal - content(node, user...) and config(node type, language...).
We have 2 modules: content_translation and config_translation. So langcode property means language of that entity.
For example we can have language entity.
id = cs, langcode = en, label = Czech
id = cs, langcode = cs, label = Česky
id = en, langcode = en, label = English
id = en, langcode = cs, label = Anglicky
this is with config_translation enabled.

id = cs, label = Czech
id = en, label = English
and this is without config_translation enabled.

From my point of view: i expose only content entities. I do not see reason to expose config in general. Being developer i prefer langConfig in gatsby-config.js. Why to store this in Gatsby index?

If i wanna build page with News i expose only node--article resource in jsonapi(i use jsonapi_extras module in Drupal). I use included (either in Gatsby or in Drupal with jsonapi_defaults) for adding related data like author and images into News.
(i prefer this way compared to disallowedLinkTypes of Gatsby.

Copy link
Contributor Author

@Auspicus Auspicus Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a valid point, especially for larger sites you'll be pulling a lot of unnecessary entities that might not be shown on your front end. I think that'd be an idea for another ticket. Perhaps an "allowedLinkTypes" option which, when set, whitelists particular entity types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

  • Check if langcode exists and if it doesn't use und

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still relevant.

@Ayrm4x
Copy link

Ayrm4x commented Sep 22, 2020

I updated the patch for the gatsby_fastbuild module: https://www.drupal.org/project/gatsby/issues/3171054#comment-13825325

With the id creation changes in the gatsby fastbuilds feature and the patched "gatsby_fastbuild" module, basic update and delete actions should work.

If we ignore syncs between translations on drupal side we should be fine.

The Patch has been merged and will be in the next release.

@kmajzlik
Copy link

Hello, i am Drupal architect/developer and starting with GatsbyJS. And for first time to start i need to build site with multilanguage. After several tries with copy/paste from code directly from this PR to my node_modules i found Lerna etc. At the end i was able to apply your changes and test. I found multiple problems:

  1. i would like to have possibility to set languageConfig manually in gatsby-config.js (i do not want expose Drupal's config into jsonapi, i want to have different default language for then backend CMS)
  2. utils.js in const fetchLanguageConfig is failing for me - trying to access empty data property from axios response - it seems like some async magic (so i made hardcoded languageConfig in gatsby-node.js and skipped checking to be successful
  3. multilanguge data fetching is dropping custom filters from configuration. Filters are applied only if url is object. But at the end first iteration it is changed into string at beginning of const getNext().
  4. fetching of data fails on second language, need to fix url.href.replace like this

const urlPath = url.href.replace(${baseUrlWithoutTrailingSlash}/${languageConfig.defaultLanguage}/${apiBase}/, ``);
dataForLanguage = await getNext(${baseUrlWithoutTrailingSlash}/${currentLanguage}/${apiBase}/${urlPath});

Now it works for me except of 3) but i can live with that (can add filters directly to Drupal with jsonapi_extras / jsonapi_defaults module).

@Auspicus
Copy link
Contributor Author

Auspicus commented Oct 1, 2020

@kmajzlik Thanks for taking the time to review this PR! Would you be able to submit a PR of your own with your proposed changes?

v.data.map(data => {
const referencedNodeId = createNodeId(data.id)
const referencedNodeId = createNodeId(
`${rootNodeLanguage}${data.id}`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work for most cases. However when a entity doesn't support translation it seems that it's langcode is always the default language of the site. In my case this meant that file entities (which don't support translation) would be lose their relationships in languages other than the default. I was able to fix this in my fork of this patch but I will need to backport here.

@hvermeire
Copy link

Hi @Auspicus.

We're using Gatsby cloud with it's incremental build feature. We notice that when using gatsby-source-drupal with this PR merged, the incremental builds do not get triggered.

This is the output we get when I change an item in Drupal with this PR merged:
image

This is the output we get when I change an item in Drupal with a vanilla gatsby-source-drupal:
image

Do you perhaps have an idea why this would be?

Thanks in advance

@Auspicus
Copy link
Contributor Author

@hvermeire I would have to look deeper into this to diagnose. I haven't seen that on our end. I am running a modified version of this patch to allow for drafts and fields on updated nodes. An amalgamation of 3 different PRs at the moment. If you want to ping me on the Drupal slack I am under @EdiSch and I can try and help you debug a bit.

@hvermeire
Copy link

Thanks for the reply @Auspicus. I found that apparently, the plugin has to be called "gatsby-source-drupal" to make incremental builds working (it was named custom-source-drupal before).

Another thing though: Removing the languageConfig from handleReferences, and using the rootNodeLanguage instead of the referenceLanguagePrefix broke having multilingual paragraphs. I think this is for paragraphs that should be available in both languages, but I'm not sure how it works in Drupal.

@KyleAMathews
Copy link
Contributor

@Auspicus and others — I've been updating this to work with master + test and it seems like it's in pretty good shape now. My test plan was:

  • compare nodes created with and without this PR
  • compare nodes created with or without translation enabled
  • translate an article and make sure I can query both

If you could do some testing soon to ensure everything is still working from your perspective, we can get this merged and published.

@KyleAMathews KyleAMathews changed the title WIP: feat(gatsby-source-drupal): Add toggleable multilingual support by prefixing nodes with their langcode feat(gatsby-source-drupal): Add toggleable multilingual support by prefixing nodes with their langcode May 17, 2021
@Auspicus
Copy link
Contributor Author

Auspicus commented May 17, 2021

@KyleAMathews I'll pull this change down locally and see how it goes.

We have another PR (yet to be pushed) that will make the sourcing of nodes run in parallel and with error correction because we were noticing that on larger sites (1k+ requests for nodes) the Drupal side was bottlenecking and the source plugin would fail if a single request dropped. We're running on Pantheon (Extra Large plan and still getting Drupal side issues) and someone at their team had a patch that queued the requests, used ETags for request caching and had automatic retries for failed requests. I'll also ping them on the PR once I get it up.

@KyleAMathews
Copy link
Contributor

that will make the sourcing of nodes run in parallel and with error correction because we were noticing that on larger sites (1k+ requests for nodes) the Drupal side was bottlenecking and the source plugin would fail if a single request dropped.

💯 a research project of mine I'm intending to do is to create a http library with

  • parallelization support and automatic concurrency control so if the server is overwhelmed it backs off but if the server can take it, it keeps upping the concurrency
  • full cache control support (etag & cache-control)
  • automatic retries on errors

This would be a separate library so we can use it across all the source plugins.

Instead set the config in the plugin options e.g.:

```js
options: {
  translation: true,
  languageConfig: {
    defaultLanguage: `en`,
    enabledLanguages: [`en`, `fil`],
    translatableEntities: [`node--article`],
  }
}
```
smthomas
smthomas previously approved these changes May 18, 2021
Copy link
Contributor

@smthomas smthomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@KyleAMathews KyleAMathews merged commit 408ba7b into gatsbyjs:master May 18, 2021
@KyleAMathews
Copy link
Contributor

Published to gatsby-source-drupal@next — this will be published to a formal release next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: needs core review Currently awaiting review from Core team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants