🇫🇷France @guignonv

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.

Raf, could you give a try to this issue fork? I added a setter that might do the job for your issue case (since I cannot reproduce it exactly on my side).

guignonv → created an issue.

I was able to reproduce a similar problem. When I have field ("language" field for instance) that is using a generic field mapper and I want to set it to "not mapped", the change is not saved. However, changes to other fields are saved.

So I will investigate that case and see what's going on.

guignonv → created an issue.

Upgrade done. Please review, test and merge if ok. :)
...or report issues otherwise.

guignonv → created an issue.

Updated documentation for External Entities v3.

guignonv → created an issue.

guignonv → created an issue.

guignonv → created an issue.

guignonv → created an issue.

I'll create a new issue to review config schemas and constraints.

I have just released a first alpha that should include all the base features I though of. It includes rudimentary help. You can give it a try (uninstall and reinstall the module).

I've tried it with the site "https://data.ademe.fr/data-fair/app/ptp-cartographique-structures-zc":

• I created a new site integration with the base URL "https://data.ademe.fr/data-fair/app/ptp-cartographique-structures-zc", the Drupal path "/carto" and the integration mode "IFrame as local content"
• I had to set a frame height because it supports infinite scroll (so there's no way to get a dynamic content height value as it's "infinite"!) and I set it to "400px" (arbitrary)
• I added a regexp filter to replace "data.ademe.fr/data-fair" with "/data-fair" (replace "" with yours).
• I had to create a second site integration with the URL "https://data.ademe.fr/data-fair" and the Drupal path "/data-fair" (integration mode does not matter as it is for resources so choose whatever)

It appears to work but there are still a few things to fix if I look at the logs. I don't have much time to investigate but these don't seem insurmountable.

Please try and give feed-back (what blocked you, what could help solving problems, etc.)! Thanks.

On my site running Drupal 10.3.5 with PHP 8.1.14 (and PostgreSQL 12 db) and external entities dev 3ac5d32c4b09dd7beefeb42552013c6dbf921838, I was not able to reproduce your problem. Note: that xntt dev version should not differ from the beta 1 in the code used for config.

What I did try:
1) (single) export an existing external entity type of mine
2) make changes in the 'id' field mapping by changing the target source field name, changed the prefix of an aggregation group, changed the REST URL of a REST client
3) then imported that modified yml config
4) re-opened the external entity type of mine and the changes made were there. Loading the modified config did modify my current external entity type in database.

Then:
5) Loaded your example yml config still with the Drupal config importer (/admin/config/development/configuration/single/import)
It created a new external entity type as expected with the corresponding field mapping and REST client settings.
6) I modified you yml:

uuid: 92a92745-279b-4029-94b1-15c185c02948
langcode: en
status: true
dependencies: {  }
id: external_item
label: 'External item'
label_plural: 'External items'
description: ''
content_class: Drupal\external_entities\Entity\ExternalEntity
read_only: false
debug_level: 0
generate_aliases: false
field_mappers:
  id:
    id: generic
    config:
      property_mappings:
        value:
          id: direct
          config:
            mapping: id
            required_field: true
            main_property: true
            data_processors: {  }
            description: ''
  uuid:
    id: generic
    config:
      property_mappings:
        value:
          id: simple
          config:
            mapping: blabla
            required_field: false
            main_property: true
            data_processors: {  }
            description: ''
  title:
    id: generic
    config:
      property_mappings:
        value:
          id: simple
          config:
            mapping: title
            required_field: true
            main_property: true
            data_processors: {  }
            description: ''
  langcode:
    id: generic
    config:
      property_mappings:
        value:
          id: simple
          config:
            mapping: language
            required_field: false
            main_property: true
            data_processors: {  }
            description: ''
field_mapping_notes: ''
data_aggregator:
  id: single
  config:
    storage_clients:
      -
        id: rest
        config:
          endpoint: 'https://www.toto.com'
          endpoint_options:
            single: ''
            count: ''
            count_mode: entities
            cache: false
            limit_qcount: 0
            limit_qtime: 0
            requests_by_user: false
          response_format: json
          data_path:
            list: ''
            single: ''
            keyed_by_id: false
            count: ''
          pager:
            default_limit: 0
            type: pagination
            page_parameter: ''
            page_parameter_type: pagenum
            page_start_one: false
            always_query: false
            page_size_parameter: ''
            page_size_parameter_type: pagesize
          api_key:
            type: none
            header_name: ''
            key: ''
          http:
            headers: ''
          parameters:
            list: {  }
            list_param_mode: query
            single: {  }
            single_param_mode: query
          filtering:
            drupal: false
            basic: false
            basic_fields: {  }
            list_support: none
            list_join: ''
data_aggregator_notes: ''
persistent_cache_max_age: 0
annotation_entity_type_id: null
annotation_bundle_id: null
annotation_field_name: null
inherits_annotation_fields: false

If you then, for example, change the mapping (in config) for the id field to:

  id:
    id: generic
    config:
      property_mappings:
        value:
          id: simple
          config:
            mapping: identifier // This is what I changed
            required_field: true
            main_property: true
            data_processors: {  }
            description: ''

(basically, the 'id' field mapper type, the 'uuid' mapping, and the endpoint of the REST client)
6) I re-imported the config with the changes.
...the changes were there on the UI when I edited "external_item".

By the way, I also have "Configuration Inspector" 2.1.9" recently update that "now" displays config schema problems that it did not before. That might help me investigate the other issue #3476772 you had I did not have before...

Could it be a Drupal version issue or some other changes you made in the source code of external entities?
If it is confirmed on your side, it is a major problem.

The code part you disabled is required as we have multiple configs living in parallel that need to be synchronized at some point. The external entity type itself contains the configs of its field mappers and its storage aggregator. But those configs also include the configs of sub-plugins (ie. property mappers, data processors, storage clients). We can't separate those configs as they define the whole external entity type. But when we instantiate each plugin, it needs its own part of the config. So, later, if when change the config of a plugin using its own methods to do so, its own part of the config will be modified but the whole external entity config hold by the external entity itself won't be modified accordingly at the same time. Therefore, when an external process will need to access to the external entity type config, it will require a synchronization. That's the reason why this "get" method has been overridden. I hope it clarify things but don't hesitate to ask for more details if it not clear (I'm also available on drupalchat.me from times to times or on demand).
I also thought about using PHP variable references to have "sub-configs" linked to each plugin config which would solve the need of any synchronization process but I forgot why but it was not possible for some reasons... and anyway, the config stored in memory contains extra-elements that must not be exported so the "get" method also clear those.

Hi!
I just created the module last week and added the Drupal project yesterday! There are no release right now not even an alpha because I consider this module not ready to be used yet. I plan to release an alpha maybe this week or the next one.
For the doc, of course, there will be one. I'll add the README probably tomorrow and I will also add an help page. So far, for *testing*, you can go to "/integrated-site" to create a new site and choose the Drupal "path" that will be used to display the integration (for example "/cartographique-structures").

But thanks for the link: I'll test that site for integration as an example. :)

Yep, that code is needed. I'll try to remind why, but it is needed... I'll see what's going on.

Could you confirm you changed back of fix I made recently in src/Form/ExternalEntityForm.php.
The fix:

      if (!empty($fm) && !empty($fm->getPropertyMapping('value')['id'])) {

Your version (which was what was there before):

      if (!empty($fm) && !empty($fm->getMappedSourceFieldName())) {
        // @todo When complex mapping is set, we don't get a source field name.

I suspect you had a conflict to resolve and discarded the fix by mistake.
As the @todo said, "$fm->getMappedSourceFieldName()" is not working with complex mapping: it will find a field mapper but that field mapper will not return a source field name, which will lead to an incorrect redirection to the mapping editing page on saving while a mapping can be there and correct.

Basically, since the id field mapping is required, "!empty($fm)" should be enough to discriminate between newly created external entities and ones that have and id field mapped. I added "$fm->getPropertyMapping('value')['id']" part in case, for some reasons, the property has no property mapper set. It should not happen in theory as setting a property mapper is also required for the id field. But I don't trust theory much and we live in the practical world so I preferred to add that (superfluous) test. After thinking, it could still be improved by checking if the property mapper plugin exists/is available... (mays a @todo should be added there in that sense?)

Anyway, you should definitely remove the "&& !empty($fm->getMappedSourceFieldName())" part which is useless and brings problems.

I changed it on purpose because Configuration Inspector was complaining that this field (like many others by the way) needed constraints. So I tried to add those constraints and it appeared fixed on my site. What kind of identifier do you have that does not meet those constraints?

But we can go back for sure if it is a problem.

This is not so simple from my perspective and I'm sorry there will be some reading here, to understand... :s

I stated in the API doc for FieldMapperInterface::getMappedSourceFieldName():

   * This method can be used to know which raw data source field was used by a
   * given Drupal field property.
   *
   * The returned source field name might be a simple field name or a path to a
   * given sub-field using dots ('.') as separator. For instance, if a Drupal
   * field property value is mapped to the sub-field "processed" of the
   * sub-field "body" of the field "attributes" of a raw data array, the
   * returned source field name would be "attributes.body.processed". However,
   * other more complex sub-field specification (such as '*' for the simple
   * propertymapper or JSON Path mappings) are not supported and must not be
   * returned.
   *
   * To get all the raw source fields involved in a complex property mapping,
   * use self::addFieldValuesToRawData() with only data from the requested
   * Drupal field and property. It would return a raw structure containing only
   * the raw fields used by the Drupal field mapping unless the reverse mapping
   * is not possible.

The API doc for the same method in PropertyMapperInterface::getMappedSourceFieldName() is a bit less precise and would allow the behavior you expect.

The reason for this "new" method (not in v2) is to be able to directly work on a source field without needing to map when it is possible. At the beginning, I was only allowing simple fields and not sub-fields. But then, I noticed in some cases, using a sub-field is not really different form using a simple field and I allowed the use of "dots" for the field path. A very similar syntax is used by Drupal query API and that's what made me change my mind as well as changing the original syntax of the old "simple field mapper" (now "simple property mapper") that was using slashes instead of dots.

Where do we need to use that method? When trying to join external data from different sources using an aggregator, when storage clients needs to now which Drupal field is mapped to which source field often for filtering queries on source-side, in External Entity Storage to faster map ids to entities (but direct mapping is not required as there is a slower workaround), and for the external entity manager to report mapping issues (again, not necessary there, NULL can be returned).

There are currently parts of the external entities module that need to be updated to support dots in the field path (which can be considered as bugs in the current beta). There is also some work to do in the storage clients ::transliterateDrupalFilters() to manage that notation as well as in StorageClientBase::testDrupalConditions() to fully support Drupal field syntax for filtering (see @todo lines 585, 594, 618 for instance).

If we support the syntax $.myField[*].myProperty --> myField.myProperty for instance, then in those places, it will be (I believe) hard to manage. Consider those 2 arrays:

$array1 = [
  'myField' => [
    [
      'myProperty' => 'value1',
    ],
    [
      'myProperty' => 'value2',
    ],
  ],
];

$array2 = [
  'myField' => [
    'myProperty' => 'value1',
  ],
];

$.myField[*].myProperty on $array1 one will return ['value1', 'value2'] while it will return NULL on $array2. However, the behavior your expecting (ie. "myField.myProperty") should work on both arrays and should let us find ['value1', 'value2'] for $array1 and ['value1'] for $array2.

We could consider clarifying the API and adjust the code of FieldMapperInterface::getMappedSourceFieldName() to allow PropertyMapperInterface::getMappedSourceFieldName() to return a simplified expression of the mapping when it is possible, and this case "$.myField[*].myProperty" would be returned as "myField.*.myProperty" but then, FieldMapperInterface::getMappedSourceFieldName() would detect the '*' and return NULL because it would be still only allowed to return very simple mapping expressions with only dots supported. I would agree on that but it would not solve your original problem.

I think we should consider your problem differently and see how the code could be changed to manage the case when you got a complex JSONPath mapping and ::getMappedSourceFieldName() returns NULL. There are workarounds to solve complex mapping (see ExternalEntityStorage class near line 600 for example).

So for me, the current code work as expected but there is still a problem to solve (feature request).

I tested the example and adjusted a couple of things in the configs you made @mortona2k. Nice work, thank you for your help on that! I also found a couple of issues related to that example in External Entity storage and JSON:API client that were fixed at the same time.
Committed with Views integration by commit 2538706.
As usual, feel free to reopen the issue if needed.

Thanks @rp7 for testing and fixing! :)

Your patch looks good to me. It should be enough to fix the problem you noticed.

A line of code was missing in the update... :-) Fixed by commit 08990d6c.

Nope. I missed some issues...

[removed]

It should be fixed by last commit (604cd3d7). Feel free to re-open if not.

I think you forgot to merge this, though (:

I did not. I tried to merge but did not notice there was an error. It was 2 commits behind so I updated the branch and it seems it has been merged now. Sorry for the inconvenience. :-]

I think you can ask yourself: why is there even a getLogger() method, while the class already has a logger() method.

Good question. My bad. Maybe I need some rest... (but not talking about "REST" services! :p )
OK, there should be some cleaning to do there and maybe to check also other similar issue on other classes. The logger service was not added the same way (at the same time) to every plugin and I tried to homogenize things but it looks like I failed. :-s I'm thinking of a new issue for that... or add it to the to-do list of the v3 roadmap.

Anyway, merged! :)

I reviewed the code quickly, I doubt it would work as expected. "LoggerInterface" is not defined (in the scope of the class). What is the expected behavior then? PHP will try to resolve the LoggerInterface by looking at \Drupal\external_entities\Entity\ for that interface right?
Did you forget to add

use Psr\Log\LoggerInterface;

Or was it on purpose?

Merged. Thanks!