🇫🇷France @guignonv

@colan feel free to merge when you thinks it is ok.

Looks good to me. But it means we have to add the (core) token module as a dependency. I fixed tests accordingly. They pass on my environment but not on GitLab pipeline, probably because of the new dependency? Weird.

The fix is quite simple. Thanks! Merged.

I hope it is not a high priority for you nor urgent as I don't have time currently to review this feature request... :s
I'm not against the idea of splitting Rest client functions into pieces though. I would need time to read carefully what you put and understand what is your final goal and then I could have an opinion.

Merged.

Note: the second query should be corrected as it uses "de" instead of "fr". ;)
https://www.dummy.com/fr/jsonapi/node/publication?filter[id]=d4133e27-92a8-40c9-9b57-122494e444c5

I'm not sure as I am not currently dealing with multiple languages so I can' tell (it's far in my stack but it's there).
I'll let @das-peter answer if he passes by, but to give more info in case he does not, this patch allows to aggregate multiple language version of a content into one external entity instance.

So, let's say you use a REST client that gather content from a website and that content site provides 2 endpoints, one for English and one for French. Each content item has the same id on both endpoint but its content is in different language for each endpoint. You use the vertical aggregator with 2 REST clients, one for each endpoint, and you aggregate them using the new option "as a translation" for the second client. Then you are supposed to have a drop-down letting you choose the language corresponding to the client endpoint and it will add the translated content as a sub-field of the external entity instance. Your xntt array would look something like that, more a less:

[
  'id' =>  1234,
  'title' => 'Some title',
  'content' => 'Some content in English.',
  '__external_entity_translation__fr' => [
    'id' =>  1234,
    'title' => 'Un titre',
    'content' => 'Du contenu en français.',
  ],
];

...that's what I understood so far.

Merged.

PHP Unit tests fixed now.

C.I. has been enabled. However, there are 2 errors in the "testHorizontalAggregation" test. I checked the logic of test and it appears ok to me so there is a bug in the horizontal aggregator client I think. :-( I think it used to work so it might be a real regression.

Also, there is a couple of validation warnings and many of them could be easily fixed. If somebody volunteers, go ahead for a PR! :-D

I mark it fixed to cleanup the issue queue. Feel free to reopen if not.

I support your approach. I don't have time to test such a thing right now but I'll try to next week. I reviewed the code though and it looks very good!

Fair enough! :) Merged! (I got time to update the repo now)

I support the idea of using tokens and there may be other places where it could be useful as well.
But it means we will need the token module as a requirement right? (I didn't play with the token module in my modules since Drupal 7!) Not a big deal though...

Merged. Thanks!

Reviewed code. Look good and fair. Could you update the repository so I can merge it thanks? :)

Weird because I'm convinced I had increased URL length 2048 a long time ago! But it appears not. :) Thanks for pointing that out and I prefer to use the largest limit 2048 to be safe. I doubt people would have thousands of external entity types in their databases and would need a shorted field length to save database space! ,:-)
I also added the URL for counts for consistency.

guignonv → made their first commit to this issue’s fork.

Hi!
I reviewed the changes. Looks quite good so far and I agree on the change to get mappable fields from the xntt. It makes sens.
However, I less confident in this part you removed:

      $derived_entity_type = $this
        ->getExternalEntityType()
        ->getDerivedEntityType();
      if (empty($derived_entity_type)) {
        $this->logger->warning('FieldMapperBase::getFieldDefinition(): Cannot get external entity derived type.');
        return NULL;
      }

This is used by annotation entities. I did not make automated tests on these so I'm not sure it will work properly. I believe you removed that part also because it was not working with your use case right?
What about keeping that part but commented then?

      /*
      @todo The following code as been disabled and impacts on annotation nodes should be checked before removing it permanently.
      @code
      $derived_entity_type = $this
        ->getExternalEntityType()
        ->getDerivedEntityType();
      if (empty($derived_entity_type)) {
        $this->logger->warning('FieldMapperBase::getFieldDefinition(): Cannot get external entity derived type.');
        return NULL;
      }
      @endcode
      */

...or something similar.

Thanks!

Looks fine to me. I just reviewed the code and did not test it yet but I believe it would work.

I'll try to contact him as well and keep you tuned.

I've change the title and the description because we already have a set of tests but they are incomplete.

It might be still relevant for v2 but not anymore for v3. v3 Has also debug features to get better backend error support. Since it's a very old issue, I close it.

On v3, this is currently handled for a single custom header (ie. "Custom header" option in the "Authentication" section). Do you need more than one header item or could some of them be passed to the other section "HTTP protocol > Additional HTTP headers"?

Should an event be added to support more header customization?

Sounds good! Especially if I got some more help on v3! :-D
Regarding the module maintenance, I've asked Attiks → to get access and work on a v3. You should ask him to get access too.

Some of the issues you listed are already solved, some others aren't. When I started v3, it was because I needed many changes in v2 and the current maintainers could not handle those. So I got access to work on a separated v3 and left v2 to its current maintainers, to not break anything. That's why there are currently a v2 and a v3. Now v3 is quite mature, it might be time to discuss of the future of v2 with its current maintainers. Do they still need it or can they migrate to v3 while there are many breaking changes in the architecture...

I didn't read everything in details (not enough time to be honest) but I leave this post as a note for people who might want to do something for that issue.

There is something that has not been implemented yet in the external entity API for entity queries: in StorageClientBase::testDrupalConditions(), there is currently no (or limited) support for complex filtering on fields using syntax with sub-fields (like it is the case here with entity references) or with '%delta' (eg. ->condition('tags.%delta.value', 'news'))). That might be the reason why it does not behave like other Drupal entities with references.

I don't thinks this is still an issue on v3 (I got a 404 in such case). Could you confirm it is solved or give steps to reproduce the error? Do you use annotations, cache?

That's already supported in v3. It's documented on the UI under the "Endpoint options" section. I move the issue back to v2 so their maintainers will decide to close or support.

This feature is already partially supported by v3. V3 supports token authentication in the HTTP header as well as in the query string. However, there's no plugin system for more sophisticated ways of authentication.

It could be a nice feature though. I would see either a plugin that would provide automatic authentication for current Drupal user (would allow SSO but it could also use some user custom account settings) or it could be achieved through an authentication event thrown for REST queries that needs authentication. It might add additional REST settings to specify what requires authentication (all or only PUT/UPDATE/POST, or some calls,...).

This is already supported by v3. I move it back to v2. Either v2 maintainers decide to close it or do the support.

Added Sanity plugin and vertical/horizontal aggregators

Updated to clarify current status with views support in v3.

The views integration is done on v3 through a "companion" module (ie. sub-module provided with external entities that needs to be enabled) but needs improvements. So far, I use the view support of v3 on a production site and the only major problem I remember was column sorting not available on table display. But I don't have a good memory... I think there are also problems with multi-lingual support which is not complete in v3 yet (as you saw in other issues ;) ...there is still a PR that needs to be reworked/updated).

Regarding the documentation, it refers to v2 and it should be updated to tell that v3 does support views. In fact, it IS possible. I got inspired by the Search API module → . But still, keep in mind that it *may* not be efficient with REST services... while it can be relevant with other storage plugins (like external files or databases).

The separate module you saw is the one I did for v2, that I'm not really supporting it anymore (minor fixes) since I've integrated it to v3 (ie. supported in v3, no double work).

It appears I'm the only one who worked on that part and I would be grateful is I could get some help! I'm working on many things (and it's an understatement! :) ) and I have to prioritize my developments. Improving the views support is in my list but in the middle/bottom. {:-)

avpaderno → credited guignonv → .

guignonv → created an issue.

Any news on that one? Should I mark it fixed?

Noted! Thank you very much for your time! :)

guignonv → created an issue.

Noted! Thanks @avpaderno! :)

Thanks. The pipeline is not passing. Could you turn "^3.0@beta1" into "^3.0@beta" to see if it is better? I made a mistake by adding the beta version. I also saw some misspelling I made reported by the pipeline, could you have a look as well?

Also, why adding ".gitlab-ci.yml"? Could you explain in simple words what new features it brings to me? I did not use CI here and I was fine but I'm open to changes as long as I got good arguments.

I appreciate your work but reviewing issues that just bring cosmetic changes takes time and sometimes, it's faster for me to just use the code sniffer on my dev environment and do the changes by myself. This module is at a beta stage and not my priority at the moment. I may not have processed it through the latest version of PHP CS but I will for next release. To accept those changes, I really need to feel like it saves me time and helps me. So, while the time you spent is appreciated, it might not be the most efficient way to help improving this module. No offense behind that, it's just that you may not guess it if I don't tell you.

guignonv → created an issue.

guignonv → created an issue.

guignonv → created an issue.

Compatibility released in 3.0.0beta2.

Maybe you could try a different command line from parent directory:
./vendor/bin/phpunit -c ./core ./modules/contrib/
or something like that, adapted to your directory structure.

I had the same errors (Composer not found) and what worked for me was ./vendor/bin/phpunit -c ./web/core ./web/modules/contrib/. I also added vendor/bin to my PATH but not sure it helped.

If it works that way then, maybe the online documentation → should be updated accordingly...

It should have been fixed by commit #38e352db. Please test and confirm!... ;)
It was a regression because of change made on how sub-plugins are managed (now using DefaultLazyPluginCollection class).

I can reproduce the problem. I'll fix it.

Thanks! Of course, I had forgotten them! :) Well spotted!

Looks good. Tested on D10 and a clean D11. Works great, merging. Thanks!

I'll do my best to review that ASAP. Thank you very much!

I'll have a look in the coming days...

Awesome! :D

@guignonv I wondered why this MR was so gigantic when I realized you had requested a merge into the 7.x-1.x branch. I took the liberty to edit the MR to set it to 2.0.x.

Oups... :) Sorry!

Fixed by commit # 5b5ed2177b697ee9e2e4e8ffdb40c3ff75138574.

Please re-open if not fixed.

Solved by commit #fa310c42bd89fe8de28eddf7dbced45d2fbf013c .

Now storage clients can use the 2 new methods ::getRequestedDrupalFields() and ::getRequestedMapping() that are documented in StorageClientInterface to specify auto-creation of Drupal fields as needed as well as offer a mapping for specific fields.

UPDATE: there was an issue on Google chrome preventing the module form working than has been fixed in current dev.

guignonv → created an issue.

I started to review the code and it requires some time investment.
My first concern is: are there people using this module needing a port to xntt v3?
I even doubt I'll get some answers here... :-) but anyway, let's give it a try at least.

After reviewing the code, I think I'd better invest time on implementing a generic SPARQL storage client plugin (as a native xntt plugin) that would inherit form the QueryLanguageClientBase class. The problem is that I don't know much about SPARQL. I would prefer to not load entities using a REST endpoint (like it is in wikibase_external_entities) but rather use a SPARQL query to retrieve each entity data.
What I think we would need on the form would be:

• the SPARQL endpoint URL
• the prefix to use for id field (eg. "wd")
• the SPARQL query to load one or more entries with a "" placeholder to tell where to put ids. For instance:
  

  SELECT ?item ?itemLabel WHERE {
    VALUES ?item { <id> }
    SERVICE wikibase:label { bd:serviceParam wikibase:language "[AUTO_LANGUAGE],en". }
  }

  where "" could be replaced by "wd:Q378619 wd:Q498787 wd:Q677525".

• maybe a mapping interface between Drupal field names and their corresponding SPARQL "prefix"? (filter mapping)
• maybe a placeholder list interface to associate <placeholders> in the query with other SPARQL queries used to retrieve the value when the external entity type is saved and avoid running such queries all the time (ie. cache the result and include it statically in the queries)

I don't know how complicated it could be to use regexp to insert filters in the SPARQL query. I believe though it should not be complicated to transform a SPARQL query into a count query automatically but please correct me if I'm dreaming! :-)

guignonv → created an issue.

guignonv → created an issue.