🇨🇭Switzerland @das-peter

Just came across this as we've got a similar need.
I think we could rely on the Image Derivative Token handling already in core - if it's deemed secure enough.
Having a valid itok parameter indicates that the url was generated by our very own instance which should count pretty much as "managed".
One could probably argue about the cryptographic strength of the core token - as its goal is to prevent DOS and not essentially fending off proxying.

das-peter → made their first commit to this issue’s fork.

Created a PR with a possible approach how to tackle this.
Related PR: https://github.com/mkalkbrenner/search_api_solr/pull/105
Patch: https://github.com/mkalkbrenner/search_api_solr/pull/105.patch & as attachment

I'm not sure if we the module can or should provide further assistance with fixing the breaking change.
It hit me off guard when our solr image was re-built and it took me quite a while to dig through everything to sort out what had gone wrong.
I wonder if there's a way to check the available modules / the set config via API and show a clear message in the Search API Overview if this is off.

If I'm not completely mistaken -Dsolr.config.lib.enabled=true would have to be set via environment variables / startup parameters.
E.g. something like this
SOLR_OPTS="$SOLR_OPTS -Dsolr.config.lib.enabled=true"

However, the only location in the jump start template that would allow that is docker-compose.yml
And there we might as well switch to use the env var SOLR_MODULES:
SOLR_MODULES="extraction,langid,ltr,analysis-extras"

das-peter → created an issue.

@guignonv No worries, no rush. I can use the patching system or in the worst case I'll implement my own external entities client.
But my goal is to provide a json:api, specifically a Drupal json:api specific, integration that is as optimized and as easy to use as possible.
This FR essentially only expands on the handling of the "included" properties as it was available for single load events: https://git.drupalcode.org/project/external_entities/-/blob/2d5693e4a0af...

In order to be consistently usable that handling has to be available across all query types single, multi, filter etc.
To do so I've now implemented a custom external_entity_response_decoder - this allows a much cleaner handling of the raw response data across all query types.

On top of that the new handling addresses the limitations of JsonPath. With the current limitations it's not possible to resolve multi value relationships. My approach to this is to not just pack "included" into each result item, but inject the related include straight into the relationship referencing the data - essentially just a denormalization in order to keep access to the referenced data as simple as possible.

@svendecabooter Fantastic, thank you so much!

I've been chipping away on this and realized that \Drupal\external_entities\Plugin\ExternalEntities\StorageClient\JsonApi does some very interesting things with the results fetched in order to extract the included data via JsonPath.
That interesting way works for single item load, but was lacking querySource support.
That lead to the fact that only the first source of vertical data aggregators had included data present to use - which then leads to inconsistencies inbetween storage clients which might break the field mappings.

After looking deeper into that I think \Drupal\external_entities\Plugin\ExternalEntities\StorageClient\RestClient::querySource() might does to much - making it not as extensible as needed. The classes inheriting it only have access to the results after major parts of the parsing has been done. It probably would be more flexible to split the method up into communication with the endpoint the pure data fetch, and then processing of the fetched data.
Another option I see right of the bat is to add a custom Decoder based on \Drupal\Component\Serialization\Json. That way JsonApi would have access to the raw response, can decode and decorate it as needed without any "hacks" to access & collect the data.

das-peter → created an issue.

@rp7 Darn, how didn't I see that MR. Sorry about that.

@guignonv I think you covered it pretty much.
This change here allows you to dynamically combine the translations of different requests into single entity.

So you got
https://www.dummy.com/de/jsonapi/node/publication?filter[id]=d4133e27-92a8-40c9-9b57-122494e444c5 that returns

{
  "data": [
    {
      "type": "node--publication",
      "id": "d4133e27-92a8-40c9-9b57-122494e444c5",
      "links": {
        "self": {
          "href": "https:\/\/www.dummy.com\/de\/jsonapi\/node\/publication\/d4133e27-92a8-40c9-9b57-122494e444c5?resourceVersion=id%3A464359"
        }
      },
      "attributes": {
        "drupal_internal__nid": 106017,
        "langcode": "de",
        "status": true,
        "title": "TITLE DE"
      }
    }
  ]
}

And then https://www.dummy.com/de/jsonapi/node/publication?filter[id]=d4133e27-92a8-40c9-9b57-122494e444c5 that returns

{
  "data": [
    {
      "type": "node--publication",
      "id": "d4133e27-92a8-40c9-9b57-122494e444c5",
      "links": {
        "self": {
          "href": "https:\/\/www.dummy.com\/fr\/jsonapi\/node\/publication\/d4133e27-92a8-40c9-9b57-122494e444c5?resourceVersion=id%3A464359"
        }
      },
      "attributes": {
        "drupal_internal__nid": 106017,
        "langcode": "fr",
        "status": true,
        "title": "TITLE FR"
      }
    }
  ]
}

which is combined into a single entity.

Does this make #3476326: Make external entities translatable obsolete?

I don't think so actually. That MR seems to "hope" for already structured data in the response of the REST Endpoint.
So you got https://www.dummy.com/jsonapi/node/publication?filter[id]=d4133e27-92a8-40c9-9b57-122494e444c5 that returns:

{
  "data": [
    {
      "type": "node--publication",
      "id": "d4133e27-92a8-40c9-9b57-122494e444c5",
      "links": {
        "self": {
          "href": "https:\/\/www.dummy.com\/jsonapi\/node\/publication\/d4133e27-92a8-40c9-9b57-122494e444c5?resourceVersion=id%3A464359"
        }
      },
      "de": {
        "attributes": {
          "drupal_internal__nid": 106017,
          "langcode": "fr",
          "status": true,
          "title": "TITLE FR"
        }
      },
      "fr": {
        "attributes": {
          "drupal_internal__nid": 106017,
          "langcode": "fr",
          "status": true,
          "title": "TITLE FR"
        }
      }
    }
  ]
}

And the raw data are "split" into the translations of a single entity.

I cheekingly say "hope" above because I don't think that MR currently allows you to map the language data but the provided data has to match or brought into the expected format.
I'd love to see a language data mapping which would allow to map the language data, with whatever langcode is used, to the "magic" translation keys "__external_entity_translation__fr".
So that a config like the following then would lead to the __external_entity_translation__* keys being populated:

• Language Map De: $.data.de
• Language Map FR: $.data.fr

Another option would be if the data requested come like:

{
  "data": [
    {
      "type": "node--publication",
      "id": "d4133e27-92a8-40c9-9b57-122494e444c5",
      "links": {
        "self": {
          "href": "https:\/\/www.dummy.com\/jsonapi\/node\/publication\/d4133e27-92a8-40c9-9b57-122494e444c5?resourceVersion=id%3A464359"
        }
      },
        "attributes": {
          "drupal_internal__nid": 106017,
          "status": true,
          "title": {
              "de": "TITLE DE",
              "fr": "TITLE FR",
          }
        }
    }
  ]
}

I think one could somehow work out a mapping here too.
Something like $data[*].attributes[*].de but that might need another merge mode too :|

@guignonv Awesome! Have a fantastic weekend!

I've created POC code.
However, due to downtime of the API I'm using and looming weekend this hasn't been tested.
But I thought this at least provides something do talk about if this is an interesting proposal.
Alternatively, I'll add it via a custom StorageClient plugin - easy enough to do so :)

das-peter → created an issue.

@guignonv Thanks for the feedback, much appreciated!

I removed it because with the new code $derived_entity_type isn't used anymore and pretty much the same code is used in the concrete implementation of ConfigurableExternalEntityTypeInterface::getMappableFields() -
ExternalEntityType::getMappableFields:getMappableFields():

      $derived_entity_type = $this->getDerivedEntityType();

      $fields = [];
      if (!empty($derived_entity_type)) {
        $fields = $this
          ->entityFieldManager()
          ->getFieldDefinitions(
            $derived_entity_type->id(),
            $derived_entity_type->id()
          );
      }

It just also fires the event.

If we want the error logging we might want to move it to ExternalEntityType::getMappableFields:getMappableFields()?
But annotation entities support should be the same, at least in this concrete implementation.
If we do not want to rely on the concrete implementation we might need to add another interface so that concrete implementations signal that they support annotation.

@guignonv Thanks for your responsiveness!
MR Rebased.

das-peter → created an issue.

das-peter → created an issue.

das-peter → created an issue.

The compatibility patch 📌 Automated Drupal 11 compatibility fixes for layout_builder_at Needs review changed the original callback from:
layout_builder_form_entity_form_display_edit_form_alter()
to
layout_builder_at_form_entity_form_display_edit_form_alter()

das-peter → created an issue.

Took a first shot at what I outlined in the summary.
Is missing tests for now - will do some manual testing for our scenario first.

das-peter → created an issue.

das-peter → created an issue.

das-peter → created an issue.

Added a hint to the role mapping help.
Am not sure how happy I am with the formatting / wording.
Happy for suggestions :)

Only when digging through the code I found:

foreach ($roles as $role_id => $role) {
      $default = '';
      if (isset($role_mappings[$role_id]) && is_array($role_mappings[$role_id])) {
        // Surround any mappings with spaces with double quotes.
        foreach ($role_mappings[$role_id] as $key => $mapping) {
          if (strpos($mapping, ' ') !== FALSE) {
            $role_mappings[$role_id][$key] = '"' . $mapping . '"';
          }
        }
        $default = implode(' ', $role_mappings[$role_id]);
      }

So this is supported but just lacks documentation

das-peter → created an issue.

das-peter → created an issue.

das-peter → created an issue.

Patch & MR created

das-peter → created an issue.

Encountered this issue too.
Patch fixed the issue.
From what I gather using the concrete LoggerChannelFactory instead an interface was an outlier when checking the constructor.
Previously required concrete implementation actually implements the interface. So unless there's some "unsanctioned" use something that only the concrete implementations provides this should be just fine :)

Created MR that can be installed via composer.
Add this to your composer.json repository section:

"drupal/entity_decorator": {
    "type": "git",
    "url": "https://git.drupalcode.org/issue/entity_decorator-3194199.git"
},

Install via composer: composer require "drupal/entity_decorator:3194199-d-9-10-compatability-dev"

das-peter → made their first commit to this issue’s fork.

Added unit test. Test only branch would fail. I guess testing pipeline will only work with MR's but I don't think I want to clutter MRs with test only. What's the best practice nowadays for test only changes to demonstrate bugfix & test coverage?

Btw. that unit test setup doesn't seem cool.
Happy to take pointers to enhance it...

Will leave in review but still believe needs a test case.

Thanks for the feedback. I'll look into that. I suppose a simple UnitTest should do.

Thanks for the feedback.

Issue should be against 11.x as the current development branch

As written in the summary - For 11.x this is obsolete due: 3442162 📌 Remove redudundat hook_ENTITY_TYPE_presave() and ViewsConfigUpdater methods Active
There's no longer any similar code - at least not that I could find any.

Also as a bug will need test coverage.

Does it? Because the current behavior ignores/violates the contract set forth by the method called.
As outlined in the summary:
According to \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinition() it is valid to return NULL :

   * @return mixed
   *   A plugin definition, or NULL if the plugin ID is invalid and
   *   $exception_on_invalid is FALSE.
   *
   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   *   Thrown if $plugin_id is invalid and $exception_on_invalid is TRUE.
   */
  public function getDefinition($plugin_id, $exception_on_invalid = TRUE);

I'm not sure if it really is necessary to have a test case for using the API not entirely correct.

That said may need to do some backtracing to see why that value is empty vs just an isset check.

I partially agree, at least on behalf of extra_field_plus → / Frontend Editing → but not from the perspective of the datetime_range module.
The value gotten is NULL, which is a valid value for our purpose. If that wasn't meant to be NULL that's an issue for extra_field_plus → / Frontend Editing → .
Or one could argue that there should be somewhere a warning triggered by some core component about that - but this isn't the scope of this issue. Happy to create a follow up issue if required.

das-peter → created an issue.

das-peter → created an issue.

Install patched version:

1. Modify composer.json - add repository:

           "drupal/ai_image_alt_text": {
               "type": "git",
               "url": "https://git.drupalcode.org/issue/ai_image_alt_text-3484955.git"
           },
   

2. Install branch package: composer require 'drupal/ai_image_alt_text:3484955-update-composer-dependency-dev'

das-peter → created an issue.

das-peter → created an issue.

Declaring this external library as external seems like a good idea and it indeed fixes the problem -> RTBC

Stumbled over this too when doing a visual code review when evaluating the module.
Change looks just fine to me -> RTBC

@ayalon Thanks for the feedback! Was totally not aware of that core issue.
That obviously explains why if ($value !== $target_url ... is tripped.

Would it be better to change the routing logic in our module?

That's a good question. If core gest fixed that's a pretty essential change in how the frontend path behaves. I guess that's also why concerns about needing a CR for this change were raised.
That said, if the behavior changes and we transparently forward the change we probably break 3rd-party APIs that have no affiliation with Drupal. Within Drupal itself we might can handle this using version constraints - but not with API dependencies.

This again would make me wonder if instead of being a query property this could be a routing-setting in the GraphQL Core Schema config.
My first local implementation was actually built as a setting and not a query argument.
A setting would allow us to maintain / toggle the behavior transparently (without the need to change the queries) - which would avoid API breaks.

Overall I wonder how soon we can expect an official change.
I wouldn't bet on any time soon - the need is still here thought.
That said and with above considerations I propose to expand the current patch and add the mentioned configuration.
In case the core change comes, the setting is in place and an update hook or another condition can take care of maintaining API behavior for all pre-existing Graphql configs.

Let me know what you think - happy to extend the patch.

@a.dmitriiev No worries, thanks for taking the time to wrangling the issue queue. Much appreciated!

@a.dmitriiev Thanks for the quick response. Unfortunately there's a error in the version committed:

function frontend_editing_ajax_render_alter(array &$data) {
  $route_name = \Drupal::routeMatch()->getRouteName()

Semicolon at the end is missing.

Created a follow-up MR: https://git.drupalcode.org/project/frontend_editing/-/merge_requests/89

First draft created.
Reviews and overall feedback welcome.
Tests are currently missing - don't have the setup ready yet - still setting to "Needs Review" to get first feedback.

das-peter → created an issue.

das-peter → created an issue.

das-peter → created an issue.

Created MR.

das-peter → changed the visibility of the branch 1.0.x to hidden.

das-peter → created an issue.

Here my 2 cents:
The operator class variable is part of ArgumentPluginBase - which as the name says is a base class.
As it behaves now, it is completely up to whoever inherits ArgumentPluginBase what to do if the operator is not set.
And I think this should be the case - introducing a default value would change the base to something opinionated which would be quite a change.
The same basic principle seems to apply to HandlerBase::breakString() - if it can't decide what to do based on the input given - it essentially declares that by returning NULL for the operator.
As such I'd advocate to allow NULL as value for the operator class variable.
However, I'd change the current MR code slightly to :

public ?string $operator = NULL;

Simply because this would fly with phps declare(strict_types=1); already and will never produce an exception like Typed property ArgumentPluginBase::$operator must not be accessed before initialization and NULL will still pass all potential isset() tests.

Now the mentioned issues with HandlerBase::breakString() not properly handling some strings is kind of unrelated to this issue - there are other issues related to this e.g. #672606 🐛 Hyphens and forward slashes (-/) break Views contextual filters Needs work

My use case is a combination of Search API & Views Reference → where tokens are used to build views arguments. Which is not related to any broken D9 D10 upgrade path but simply how one can configure a views setup.

Well, welcome back on board :)

Thanks! And thank you for being so responsive :)

1. ✓ Patch still applies
2. ✓ Seems to follow best practice with using the trait.
3. ✓ Can't find any leftover t() calls
4. ✓ Code's clean
5. ✓ No unrelated changes.
6. ✓ No errors on brief test

I'd say this is fine -> RTBC.

Maybe we should check for EntityFormInterface instead of EntityForm?

Absolutely correct! Adjusted.

Hey Peter! Really long time no see! Yeah you are right.

Hey Pascal! Indeed, looong time - nice to see ya! :) Didn't have proper drupal projects for a while - but right now getting back in the game.

Install D10 version:

1. Modify composer.json - add repository:
   

       "repositories": {
           "drupal/migrate_source_jsonpath": {
               "type": "git",
               "url": "https://git.drupalcode.org/issue/migrate_source_jsonpath-3297669.git"
           }
       }
   

2. Install branch package: composer require 'drupal/migrate_source_jsonpath:3297669-automated-drupal-10-dev'

das-peter → made their first commit to this issue’s fork.

The options to render the remote file seem very limited. There is not even an option to display as an image?

That's what I thought too.
All one has to do is to allow image as allowed_field_types in the source plugin.
However, because file / image is somewhat special I think a dedicated source plugin should be provided.
I've done that in the MR branch. It's pretty much just copy & paste from the code already provided with slight adjustments for the type and type check extension in the form class.

das-peter → made their first commit to this issue’s fork.

Created MR with extended condition before attempting to call ::getEntity()

das-peter → created an issue.

Integration added.
Before the integration one could execute the referenced view like this:

query ViewsReferenceFieldDemo {
  entityById(entityType: PARAGRAPH, id: 110) {
    ... on ParagraphViewsReferenceFieldDemo {
      bpViewRawField {
        first {
          entity {
            executable(displayId: "block_1") {
              execute {
                rows {
                  __typename
                  id
                  label
                }
              }
            }
          }
        }
      }
    }
  }
}

The change keeps this behavior untouched - but you can now use the following query to execute the view with viewsreference pre-configured:

query ViewsReferenceFieldDemo {
  entityById(entityType: PARAGRAPH, id: 110) {
    ... on ParagraphViewsReferenceFieldDemo {
      bpViewRawField {
        first {
          executable {
            execute {
              rows {
                __typename
                id
                label
              }
            }
          }
        }
      }
    }
  }
}

I had to duplicate quite a bit of code from \Drupal\viewsreference\Plugin\Field\FieldFormatter\ViewsReferenceFieldFormatter::viewElements().
I think it's worth considering outsouring the preparation / building of the view into maybe a trait.

das-peter → created an issue.

das-peter → changed the visibility of the branch 2985364-pass-token-from-paragraph-legacy-approach to hidden.

das-peter → changed the visibility of the branch 2985364-pass-token-from-paragraph to active.

das-peter → changed the visibility of the branch 2985364-pass-token-from-paragraph to hidden.

das-peter → changed the visibility of the branch 2985364-pass-token-from-paragraph-light-approach to hidden.

Review of: #59

1. +++ b/src/Plugin/ViewsReferenceSetting/ViewsReferenceArgument.php
   @@ -77,11 +78,32 @@
   +      $token_service = \Drupal::token();
          if (is_array($arguments)) {
            foreach ($arguments as $index => $argument) {
              if (!empty($this->tokenService->scan($argument))) {
   -            $arguments[$index] = $this->tokenService->replace($argument, ['node' => $node]);
   +            $arguments[$index] = $token_service->replace($argument, $replacements);
   

   Why are we getting the token service while we already have the service via DI? It is even used in the condition before the replacement.

2. +++ b/viewsreference.module
   @@ -95,9 +95,9 @@ function viewsreference_views_pre_view(ViewExecutable $view, $display_id, array
   -  $viewsreference_plugin_manager = \Drupal::service('plugin.manager.viewsreference.setting');
   -  $plugin_definitions = $viewsreference_plugin_manager->getDefinitions();
   -  if (isset($view->element['#viewsreference']['enabled_settings'])) {
   +  if (isset($view->element['#viewsreference']['entity'])) {
   +    $viewsreference_plugin_manager = \Drupal::service('plugin.manager.viewsreference.setting');
   +    $plugin_definitions = $viewsreference_plugin_manager->getDefinitions();
   

   Seems unnecessary - the ['entity'] addition came from the other approach it seems (pre #48).

If I get the general consensus the new approach started in #48 is the preferred one.
I'd agree with that since that approach needs less changes and seems to work in a generic approach while maintaining backward compatibility.
That said - I've created new branches in the fork:

• 2985364-pass-token-from-paragraph-light-approach: Patch from #59 with the changes I mentioned above
• 2985364-pass-token-from-paragraph-legacy-approach: Pre-#48 code

The MR branch 2985364-pass-token-from-paragraph contains now the light approach

@scott_euser There should be now only one Branch / MR - I've also added an update to the issue summary. Haven't gotten around to add tests yet - hence keeping on needs work.

das-peter → changed the visibility of the branch 2985364-updates-for-beta6 to hidden.

das-peter → made their first commit to this issue’s fork.

Stumbled over this too - for now I resorted to these drush commands to register migrations - might be handy for someone finding this issue too:
cat docroot/modules/custom/my_module/migrations/my_migration.yml | drush config:set --input-format=yaml migrate_plus.migration.my_migration ? -

And here the a way to import all migrations in a folder:
find docroot/modules/custom/my_module/migrations/ -type f -name "*.yml" -print0 | xargs -0 -I {} sh -c 'echo "Importing {}"; cat {} | drush config:set --input-format=yaml migrate_plus.migration.$(basename "{}" .yml;) ? -'

In the section "Importing migration YAML definitions" the drush equivalent admin/config/development/configuration/single/import to would be handy:

cat docroot/modules/custom/my_module/migrations/my_migration.yml | drush config:set --input-format=yaml migrate_plus.migration.my_migration ? -

And here the a way to import all migrations in a folder:

find docroot/modules/custom/my_module/migrations/ -type f -name "*.yml" -print0  | xargs -0 -I {}  sh -c 'echo "Importing {}"; cat {} | drush config:set --input-format=yaml migrate_plus.migration.$(basename "{}" .yml;) ? -'

das-peter → created an issue.

Made proposal to use a conservative update and still require a specific minor version iteration.