Gent
Account created on 10 October 2005, over 19 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium svendecabooter Gent

I think a plugin system for detecting the current environment would be really helpful.
While a lot of people would opt to use URL matching to discover the current environment, I can see other methods being used:
- a config override in settings.php
- a setting in \Drupal::state()
- a system environment variable (retrieved via getenv()) - e.g. set by a CI script targetting a specific environment
- ...

🇧🇪Belgium svendecabooter Gent

Together with the patch to default_content mentioned above, this MR allows custom_field data to be exported by default_content, and imported by default_content import OR core's DefaultContent API, used by recipes.

🇧🇪Belgium svendecabooter Gent

Attached is a patch file for the current state of the MR, useful for Composer based patching workflows.

🇧🇪Belgium svendecabooter Gent

A patch has been added to default_content module, to make sure the Default Content exported YML files use the UUID identifiers of referenced entities within a custom_field field, rather than their (numeric) entity ID.
The patch can be found at Add support for custom_field module Active

🇧🇪Belgium svendecabooter Gent

On the import side of things, there is a patch for custom_field that makes this work: 📌 Support Drupal core DefaultContent API Active .

🇧🇪Belgium svendecabooter Gent

Created an MR that updates custom_field data in the Default Content export for referenced entities.

Original output without this MR:

field_my_custom_field:
    -
      label: 'Exported label'
      image: 21 # Some random file ID
      image__width: 1024
      image__height: 1024
      image__alt: Sunset
      image__title: ''
      file: 22 # Some random file ID
      term: 4 # Some random taxonomy term ID

After applying this MR. this becomes:

  field_my_custom_field:
    -
      label: 'Exported label'
      image: aa6340f0-0006-4f31-9f2d-5595f5cce028
      image__width: 1024
      image__height: 1024
      image__alt: Sunset
      image__title: ''
      file: c36ea199-cd17-4fe5-bec7-beef1ffb51b4
      term: 78ae1ee7-f2d4-4fc0-871e-1af92730df3b

The referenced UUIDs also get added as a dependency to the "_meta" definition.

🇧🇪Belgium svendecabooter Gent

Tested with the changed that got committed into the ai_translate module.
Everything seems to work now!

🇧🇪Belgium svendecabooter Gent

MR added for this functionality.
Also attaching as a patch file, for Composer based patching workflows.

🇧🇪Belgium svendecabooter Gent

Seems related to https://www.drupal.org/project/ai/issues/3529669 Handle link field translation as well Active

🇧🇪Belgium svendecabooter Gent

As reported in 🐛 [5.0.0-alpha3] Menu items do not get is-active class Active , this MR fixes the issue for me in ui_suite_daisyui for me, after adding some extra logic in the menu component of that theme.

I think having active menu items is pretty basic expected behaviour for a theme, so would be good to get this included sooner rather than later. Having to patch both a module and theme to get a visual indicator of the active menu item, isn't the best situation for getting adoption IMHO. Therefor I took the liberty to increase the priority of this issue. Feel free to lower priority if you disagree.

🇧🇪Belgium svendecabooter Gent

I fixed this by applying the patch in Add ability to track active menu items in menu source Active and adding the following to the ui_suite_daisyui:menu component:

(...)
  {% for item in items %}
  <li{{ (not item.url and loop.first) ? ' class="menu-title"' : '' }}>
    {% set item_attributes = item.attributes|default(create_attribute()) %}
    {# Start adding class for menu active trail #}
    {% if item.in_active_trail %}
      {% set item_attributes = item_attributes.addClass('menu-active') %}
    {% endif %}
    {# End adding class for menu active trail #}
    {% if item.url %}
(...)

Not sure if this logic can already be incorporated into ui_suite_daisyui, while waiting on its commit in ui_patterns.
It would be good to have this already in place by the time ui_patterns gets updated, but not sure what the policy is there...

🇧🇪Belgium svendecabooter Gent

I'm not sure if this is the right place for this feature request, but I would propose to add an alter hook to the getFeaturedPageActions() method.
In my case, I want a custom entity to have another "featured action", rather than the edit form.
Currently I don't see a way to achieve this.
IMHO core should provide sane defaults, but allow the option for contrib / custom module to change the default behaviour.

🇧🇪Belgium svendecabooter Gent

The logic in the MR now seems to work for most of the custom_field property types I have tested.
Still WIP:

  • Link field is a challenge. The link itself gets translated currently, since it's a string. However source link https://google.com gets translated in my OpenAI instance to \nhttps://google.com\n, where the extra newline characters mess up the data. Ideally the link itself shouldn't go to translation...
  • Image alt / title texts do not get translated yet, because their property is not a string DataType, but rather an entity reference...
🇧🇪Belgium svendecabooter Gent

Thanks for the commit!

We use Single Directory Components in our themes, so using a custom template override allows us to render the custom_field through a SDC component.
E.g.:

{{ include('themename:card', {
  label: item['label'],
  title: item['title'],
  subtitle: item['subtitle']
  body: item['body'],
}, only) }}
🇧🇪Belgium svendecabooter Gent

As per conversation in #ai on Slack, moving this to 1.2.x branch, since it's a new / extra feature.

🇧🇪Belgium svendecabooter Gent

As per conversation in #ai on Slack, moving this to 1.2.x branch, since it's a new / extra feature.

🇧🇪Belgium svendecabooter Gent

I'm encountering the same issue when using Heroicons.

🇧🇪Belgium svendecabooter Gent

OK I think this should be doable, but it would require the patch in Don't hardcode 'value' key for textual field translation Active to be committed first.
Not sure how fast that can happen, so this extra plugin probably won't be committed into custom_field, before that issue is resolved.
I'll try to get the MR here working in combination with that issue, and hopefully keep it as simple as possible.

Currently there is some logic that checks if a custom_field subfield / property is translatable or not.
But ideally that should check if that subfield is textual or not. Is there some mechanism in place to do that?
I guess just doing checks on CustomFieldType plugin instances? I.e. StringType & StringLongType?

🇧🇪Belgium svendecabooter Gent

Created a merge request that abstracts out the hardcoded 'value' logic.
If the FieldTextExtractor plugin returns an array of type ['value' => 'Some text'] in their extract() method, everything should keep working as before. But if the FieldTextExtractor plugin returns more dynamic properties, e.g. via the custom_field module, this would now work.
Example data in the extract() method for a "custom_field" FieldTextExtractor would then be:

['question' => 'Some question'. 'answer' => 'Some answer']

The changed logic applies both to the translation via the UI (Controller class) or via Drush. Ideally, both should be refactored to abstract out logic that is the same in both implementations, to allow less code duplication, but that seems out of scope for this issue.

Thanks for giving this some thought / review.

🇧🇪Belgium svendecabooter Gent

Thanks for the feedback. I'm working on a fix. Might take me a few days to push some code though...

🇧🇪Belgium svendecabooter Gent

Linking issue in AI module, that complicates this functionality.

🇧🇪Belgium svendecabooter Gent

I applied the MR logic from Add ability to track active menu items in menu source Active .
Then in ui_suite_daisyui:menu component, I add a check:

{% if item.in_active_trail %}
      <!-- Try adding extra class here -->
{% endif %}

That doesn't seem to work... Maybe I'm missing some additional context

🇧🇪Belgium svendecabooter Gent

svendecabooter changed the visibility of the branch 3528586-support-drupal-core to hidden.

🇧🇪Belgium svendecabooter Gent

OK it should be possible after all I think.

Say we can integrate with the contrib Default Content module, to produce output as follows:

field_logos:
    -
      logo:
        entity_type: 'media'
        entity: eab657e0-3320-41d2-ab4e-53a09b397758
      link: 'https://google.com'
      link__title: ''
      link__options: { }

(if not possible, worst case we need to document the exported YAML files have to be manually edited).

Then in CustomFieldEntityReference::setValue(). we can do something like:

  /**
   * {@inheritdoc}
   */
  public function setValue($value, $notify = TRUE): void {
    $entity = $value['entity'] ?? NULL;
    // START ADDED LOGIC.
    if (is_array($value)) {
      if ($value['entity_type'] && Uuid::isValid($value['entity'])) {
        $entity = \Drupal::entityTypeManager()->getStorage($value['entity_type'])->loadByProperties(['uuid' => $value['entity']]);
        $entity = !empty($entity) ? current($entity) : NULL;
      }
    }
    // END ADDED LOGIC.
    if ($entity instanceof EntityInterface) {
      ...
   }

I can spend some time to look further into this, but am currently time restrained, so just doing some braindump here.
Will try to elaborate on it next week.
Let me know if it would be a workable solution to extend the setValue() logic, for the use case of default content exports / imports.

🇧🇪Belgium svendecabooter Gent

Did some more digging, and it seems the datatype used in custom_field module is "CustomFieldEntityReference", which does not inherit from core "EntityReference" data type, so guess this will never work :'(

🇧🇪Belgium svendecabooter Gent

Thanks for the fix.
Committed now.

🇧🇪Belgium svendecabooter Gent

Thanks for looking into this sarvjeetsingh.
It hadn't occurred to me that this could be the source of the issue.
Fix in MR resolves the issue for me.

🇧🇪Belgium svendecabooter Gent

Added MR that adds this functionality + a patch file for Composer-based patching workflows.
Thanks for considering merging this functionality.

🇧🇪Belgium svendecabooter Gent

Merge request added.
This probably also can be committed to 1.0.x and 1.2.x branch, but I made the MR against 1.1.x, since I'm actively working with that branch.

🇧🇪Belgium svendecabooter Gent

Added patch file for the current state of the MR, for composer based patching workflows.

🇧🇪Belgium svendecabooter Gent

I added a MR to add this functionality.
Whether or not the Provider Configuration fieldset is open by default or not, is managed via config. The default setting for this config is to remain open, to keep backwards compatibility.

🇧🇪Belgium svendecabooter Gent

EDIT: seems to be unrelated to this patch, because I'm getting the same errors when patch is not applied.

🇧🇪Belgium svendecabooter Gent

Tested with OpenAI, and getting the following errors:

Warning: Undefined array key "fids" in Drupal\media_library\Form\FileUploadForm->validateUploadElement() (line 212 of /var/www/html/web/core/modules/media_library/src/Form/FileUploadForm.php)

ypeError: count(): Argument #1 ($value) must be of type Countable|array, null given in count() (line 212 of /var/www/html/web/core/modules/media_library/src/Form/FileUploadForm.php).

Haven't investigated the root cause yet...

🇧🇪Belgium svendecabooter Gent

All code review issues have been fixed, and extra explanation has been provided.
Setting this to Needs Review.

🇧🇪Belgium svendecabooter Gent

Added a patch file, to be used in Composer-based patching workflows.

🇧🇪Belgium svendecabooter Gent

Rebased the MR on current 11.x to resolve merge conflict.

🇧🇪Belgium svendecabooter Gent

svendecabooter changed the visibility of the branch 3526978-add-providerid-context-1.1.x to hidden.

🇧🇪Belgium svendecabooter Gent

Created 2nd branch with same commit - branched from 1.1.x - since I needed that for my testing purposes.

🇧🇪Belgium svendecabooter Gent

Ah yes I did add the CSS like in #9 - forgot to document here.

🇧🇪Belgium svendecabooter Gent

@anjaliprasannan shouldn't the MR be against 1.1.x branch?
Not sure what the branching strategy for AI is exactly, but seems to me active dev is against 1.1.x now.

🇧🇪Belgium svendecabooter Gent

@sarvjeetsingh:

I'm not sure if that would work - I'm still tipping my toes into how all of this works...

It would be something like this, in the ChatOutput constructor?:

    if ($normalized instanceof StreamedChatMessageIteratorInterface) {
      $normalized->triggerEvent();
    }

In my initial testing, that doesn't seem to work. I then get the same error as in the tests:
TypeError: Drupal\ai\Event\PostStreamingResponseEvent::__construct(): Argument #1 ($request_thread_id) must be of type string, null given

Not sure if that's something that also needs to be fixed, or if this means this approach is not workable...

🇧🇪Belgium svendecabooter Gent

Basic integration logic has been committed in 1.0.x branch, but still needs some work.

🇧🇪Belgium svendecabooter Gent

Test PromptJsonDecoderTest seems to fail now, because \Drupal\ai\Event\PostStreamingResponseEvent::__construct doesn't receive a request_thread_id string in its constructor.
Not sure if this value is NULL in this test by design, or by accident.
If by design, then the PostStreamingResponseEvent $request_thread_id parameter should be allowed to be nullable?

🇧🇪Belgium svendecabooter Gent

Added related issues in AI module, that need to be fixed first, before both token usage for both regular chat and streamed chat can be captured.

🇧🇪Belgium svendecabooter Gent

MR now triggers the event, when the StreamedChatMessageIterator object gets destructed.

🇧🇪Belgium svendecabooter Gent

Seems like the failure of PostStreamingResponseEvent not being triggered, is a separate issue.
See 🐛 PostStreamingResponseEvent never gets triggered Active .

The code in this MR can probably be already reviewed, but will not be functional until this other issue is fixed first.

🇧🇪Belgium svendecabooter Gent

Thanks for the merge.
I guess we can now create followup tickets, so different local sites can request to merge in their own config_split.config_split.la_[LOCAL_SITE].yml and appropriate config/la_[LOCAL_SITE] config split folder.

🇧🇪Belgium svendecabooter Gent

Thanks for the added logic sarvjeetsingh.
I am unable to make this work on my local installation though, as mentioned above.

Test scenario:
- Enable AI CKEditor integration module
- Add AI button to CKEditor
- Enable "Fix spelling" for example
- Select a text in CKEditor that needs spelling fixes.
- The action gets performed, but my debugger never enters the method `\Drupal\ai\OperationType\Chat\StreamedChatMessageIterator::triggerEvent`

Can also be tested by installing dev version of https://www.drupal.org/project/ai_usage_limits module, and configuring it with token usage limits. The usage stats will not go up in the test scenario mentioned above, even though they should...

Production build 0.71.5 2024