🇫🇮Finland @jhuhta

Thanks, great! Adding credits & merging.

#9: I think we should inject the language manager instead of calling \Drupal::languageManager() if this change is necessary. And continue from the previous merge request too.

I had to do an identical change to get the update process to work, before noticing there is a pending MR already. So, RTBC'ing this one. The bug breaks the drush process similarly.

Seems like my name/nick in the readme is triggering the spelling error. Could you fix those still so that everything is green?

Rebased. This works and helps getting rid of double loaded js-cookie library by removing the core asset. I can't RTBC this now as it's my rebase but still.

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

Rebased.

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

Thanks for the feedback! Would you be so kind and provide the fix as a MR? I don't have much time to work for this right now, so that would make it easier for me and faster for you to get a working solution (+ credit).

Just wondering if the code should try the default chat provider too, if none is configured.

The instructions for configuring the module are stated on the module front page.

How about this one? ✨ Support chat operation type AI providers Active

Just install the latest dev version. I can make a new release if someone could tell that it seems to work. (It works on our project pretty well though.)

I haven't had time to actually test it and check it properly. What I'd like to avoid though, is hardcoding contrib provided field types there - maybe either have it as an extendable structure or then get rid of of the thing completely and instead try to recognize if this or that field could have the feature enabled. The form mode config form shouldn't add the third party settings to every single existing field though.

Interesting. I don't think we should add direct support for various field types names provided by other contrib modules, but instead do something more generic, such as provide a hook or plugin type of approach that could add support for such so that module users could plug this into new field types.

The problem might be to know beforehand what kind of field processing would be needed in processElement to enable this kind of pluggable solution. But do you get my idea? Would it be something that you could work on? The plugin approach could be better in a sense that this module could already include plugins for various field types. Or, if it is possible to write the code so that it already supports many kind of fields automatically, then a hook to alter the list of supported field types would suffice. Allowing to add the button for just type of field widget would probably be a bit risky?

Rebased both branches. For 11.x, 📌 [ignore] Convert everything everywhere all at once Active (I wasn't familiar with) forced me to move a change from comment.module to src/Hook/CommentHooks.php. Hopefully it's ok now.

I'm not sure if the patches above contain all the same changes.

Crediting @pontus.talvikarhu for testing & review.

jhuhta → created an issue.

jhuhta → created an issue.

Thanks! Crediting @mitechworld for idea and testing.

Please RTBC this if you are sure it works - and doesn't break any existing functionality. I can create a new release at some point so that you can get rid of the patches.

See if the code in the MR would work for you.

It would be fairly easy to add a configuration option with a multiple select like "Offer the translation feature for these languages only".

Not against adding such a feature - I'd be happy to add it if you provide the patch.

The MR fixes the issue for me. See if there's problems in the approach or code.

Included also two unrelated minor things:
1) The delete button copy ("delete: 'Delete Model'") got saved to the settings, which I removed.
2) The Edit button text was misleading: "Edit Model", when the user was already editing and was about to save it.

Does it need an update hook?

jhuhta → created an issue.

Crediting @wouters_f for the idea and support to make this happen.

It also required writing a new operation type for the AI module → and a provider module for DeepL → .

Fixed in 2.0.x branch.

Looks good, merging.

The code looks ok by reading. Do you have these changes running on your project already?

Ok, so my work here should be done.

Just FYI, I published a simple DeepL provider module for the AI ecosystem right now: https://www.drupal.org/project/ai_provider_deepl → .

My translation module ( https://www.drupal.org/project/ai_translate_textfield → ), which has different approach than the AI Translate module, will be using it soon.

Should be ready now.

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

I'll just change this to feature request, which category probably better fits the request.

Happy to include the change, and even happier if there's a contribution! So the exception needs to be passed on and caught on a later phase, to be able to pass the error message.

Let's have this in and make another release once it's there.

And now you have the alpha release too, thanks for requesting that!

Nevermind, I gave it a try and it seemed to do the trick. Merging.

Nice, thanks! Would you get someone (ie. from your project team) to test the feature and RTBC this issue? It would speed things up so that I can merge this. And you're really using this module, I'd be happy to make a release too.

Ok, case closed then, a full release won't be done. I'll deprecate this one when I have time to replace it myself from a live project.

For the entity select need if yours, I'm quite sure there's a module for that, too, and if not, it's not difficult for you to write one.

Hi @Anybody and thanks for asking! I'd gladly make this a real module and add you as a co-maintainer. But why the rename? This is not about a select element (such as ie. those term select elements) but just to show an entity on the form.

By the way this module looks a lot like https://www.drupal.org/project/webform_entity_view → (which didn't exist when I originally wrote this) by its description, but is this one more suitable for your needs?

The changes look good and the touched functionalities still work, given that ExportTermsForm is ignored as @sokru said.

Phpstan and the likes will be addressed separately I assume.

jhuhta → created an issue.

This patch fixes the issue for us. I can make an MR out of it if the maintainer thinks this makes any sense.

jhuhta → created an issue.

I think the first option is not that good as the Azure OpenAI API is somewhat different and may diverge even more. I noticed this when trying to use the openai-php/client library in a custom chat solution: the structure of a response message was slightly different and the library couldn't process it as it wrongly assumed an array key existing - I don't remember which one exactly though.

I asked if the library could support Azure OpenAI too but haven't gotten a reply yet: https://github.com/openai-php/client/issues/320

Now that I plan to start using this module too, I'd need a provider that would supports at least the chat operation to begin with. Currently that would require reinventing the wheel for the client library, if I'm not mistaken.

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

The suggestion sounds very reasonable. By the time of writing this module, the need was to make it as simple as possible and match the requirements from the client. Writing the LLM client appeared to be so simple that adding a dependency to the AI module ecosystem felt a bit overkill, but I'm ready to reconsider that. A working MR would make the change easier. An upgrade path would also help.

But now this current implementation provides also a possibility to include service provider specific processing, which I'm not sure would be possible.

Added credit to @akseli.miettinen for report, debugging, review and test.

Does adding a config option solve the problem with the code from https://git.drupalcode.org/project/ai_translate_textfield/-/merge_reques...?

    language_fallbacks:
      en: en-GB

It seems DeepL really doesn't support 'en' anymore(?) (https://developers.deepl.com/docs/api-reference/translate/openapi-spec-f...). I'll add a language fallback config option for the module for this case, as the language code from Drupal, which the module uses ('en'), doesn't necessarily include the full locale name.

xjm → credited jhuhta → .

Confirming the problem.
I have a list of path aliases like these:
/en/english-alias
/fi/finnish-alias
/sv/swedish-alias

And I'd like to create Url objects of them, removing those that point to other translations than the one of active language. But Drupal don't seem to consider those ones with non-active language prefix routable urls, even though the creation function has been explicitly provided with the correct language option.

Following the code with debugger, it seems getRouteCollectionCacheId() doesn't use the provided language anymore, but uses the current language instead. Which leads to null result.

It is not intended, we just didn't have that need and thus time to test it. I'd be happy to enable it if you'd like to have it. Would you send a patch?

Great, thanks for the contribution! It works and looks good so merged.

jhuhta → created an issue.

It seems that the attribute set as mail_attr is not always populated in the data, which I didn't know. A log entry, which I didn't immediately notice, states that clearly:

Drupal\simplesamlphp_auth\Exception\SimplesamlphpAttributeException: Error in simplesamlphp_auth.module: no valid "urn:oid:0.9.2342.19200300.100.1.3" attribute set

I'm assuming that this exception caused the user provisioning to fail in the middle of the process. Disabling the email syncing in the configuration should fix the problem, as we have anyway some custom attribute syncing code in place that can handle this - and tolerate better missing data.

I'm seeing exactly the same problem too. It seems some users are randomly getting the simplesamlphp_auth_ prefix to their username - and also the User entity fields and roles are not set, while some other users get provisioned normally as before. There's no visible common denominator in these cases.

There's no custom hook_entity_insert that could be messing with this in my case though.

Thanks for the report and patch! For some reason I didn't notice the missing file extension.

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

Thanks for the first contributed change to this new project!

The description is good, the bug exists and was introduced in 🐛 Unreliable translation on select #options optgroups Fixed by reusing the $option variable. Patch in #2 fixes the problem, so I made it easier for someone else to continue the work by creating a MR out of @yaartse's patch. Let's see if the missing tests are a blocker.

I'd add that you can't even work around the bug by adding the missing translation, and the bug is there on the translation form too as it uses the same code.

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

I'm RTBC'ing the last change by @pontus.talvikarhu - maintainer consideration is ofc needed too as I wrote the original patch.

Here I removed a redundant test function, as the kernel test suffices. Got help with this from @lauriii.

I'm not sure if this issue is relevant anymore, or could the related issue 🐛 It is not possible to react to an entity being duplicated Needs work provide some of what's needed here. Closing as duplicate, but feel free to reopen if appropriate.

This is a reroll based on #8.

Fixing a missing import. Works even better now.

I'm not 100% sure if I had the same issue. However, the new 10.1 aggregation and Redirect together causes an extra language prefix redirect on a multilingual site. But there's a simpler way to prevent that from happening: just disable the normalization on a given route. Here's a patch that works for me on that.

#11 didn't quite work for me so I improved the code and its security a bit: only return the data if the key matches. Also, the form has now the textarea element which is far better for multiple lines.

Re-applied the patch against 10.1.x and 11.x branches. Didn't have that much luck with GitLab so here we go traditionally with patch files. Feel free to test and review.

How about this then? I decided to abandon the permission based approach completely and use role based one. This is maybe not an optimal way to do Drupal features, but I do think this is way better than before anyway (thanks for the suggestion!) and the permission based one just can't work for admins. This is clean and straightforward, and the admin can decide the roles which are affected, if any.

Also, the update path should work. Please review.

Translated the patch #8 to be compatible with the 2.0 version of the module.

This fixes the issue for us too.

Me neither. I just wanted to create as little intrusive change as possible, to spare the need for warning users or creating update hooks. I don't really get the point of hardcoding uid 1 there, so if you think, as a maintainer, that refactoring that would be a better approach, I can probably take another look at it later.

catch → credited jhuhta → .

This should provide a solution. Sorry for the mostly bogus diff for the schema.yml, the original had dos newlines.

jhuhta → created an issue.

jhuhta → created an issue.

jhuhta → created an issue.

I need to withdraw my RTBC status as I noticed that this is not working properly without 🐛 Paragraphs module compatibility Needs review . The idea is included there though.

I just wanted to point out that there's a similar issue with a patch: 🐛 Paragraphs module compatibility Needs review . Maybe one of these could be closed as duplicate?

The data is not lost only when opening/closing paragraphs, but also when adding/deleting unrelated paragraph types from the entity.

#2 was not quite enough. The problem with the Paragraphs use seems to be that the H5P module expects the field name to be unique, and that's not the case if H5P is a paragraph type. This patch should fix it.

This one also contains an idea from 📌 Indication of uploaded .h5p file RTBC , but it didn't quite work standalone without this one either.

This fixes the issue for us. For instance, when there's a possibility to have multiple H5P items on a page, it's quite difficult to guess which one is the correct one, as all the widgets look the same regardless of they're having a component already, or waiting for one to be uploaded.

TL;DR: @mahtab_alam's change fixes this for me, but I noticed it a bit late. The CKEditor5 library dependency declaration was indeed missing.

Longer explanation: I had the similar problem when using Maxlength on Webform's text_format fields. First of all, there is no UI that I know of for adding Maxlength to the fields, but you can achieve it in YAML by issuing something like this:

field_name:
  '#type': text_format
  '#title': 'Field title'
  '#allowed_formats':
    simple_text: simple_text
  '#hide_help': true
  '#maxlength_js': true
  '#attributes':
    class:
      - maxlength
    data-maxlength: 200
  '#attached':
    library:
      - maxlength/maxlength

Now the maxlength text appeared, but the calculation didn't work at all, and I found this issue after having realized that CKEditor is not initialized by the time the calculation functionality is attached. After reading @joevagyok's comment I developed a solution with slightly different approach, using MutationObserver. I didn't want to create a patch as I believe there's a solution already, but saving the code here anyway just in case.

  Drupal.behaviors.maxlength = {
    attach: function(context) {
      ...

      // Create a function to handle the CKEditor 5 integration.
      function integrateWithCKEditor5(element, options) {
        const eid = element.dataset.ckeditor5Id;
        const ckeditorInstance = Drupal.CKEditor5Instances.get(eid);

        if (eid && ckeditorInstance) {
          ml.ckeditor5(ckeditorInstance, options);
        }
      }

      // Select the elements with the class 'maxlength' and filter them to input elements.
      const maxlengthInputs = $context.find('.maxlength').filter(':input');

      // Function to monitor changes in dataset.
      function observeDatasetChanges(element, callback) {
        const observer = new MutationObserver(() => {
          callback(element);
        });

        observer.observe(element, { attributes: true });
      }

      // Iterate over the input elements.
      $(once('maxlength', maxlengthInputs)).each(function () {
        const options = {};
        const $this = $(this);
        options['counterText'] = $this.attr('maxlength_js_label');
        if ($this.hasClass('maxlength_js_enforce')) {
          options['enforce'] = true;
        }

        $this.charCount(options);

        // Observe changes in dataset.
        observeDatasetChanges(this, (element) => {
          if (element.dataset.ckeditor5Id) {
            integrateWithCKEditor5(element, options);
          }
        });

        // Use setInterval to periodically check for CKEditor 5 instance in Drupal.CKEditor5Instances.
        const checkInterval = setInterval(() => {
          integrateWithCKEditor5(this, options);

          // If CKEditor 5 instance and data-ckeditor5-id are found, clear the interval.
          if (Drupal.CKEditor5Instances.has(this.dataset.ckeditor5Id)) {
            clearInterval(checkInterval);
          }
        }, 100); // Check every 100 milliseconds.
      });

    },

I can confirm the problem and that the change in the patch #4 fixes it.

Per session cache context is effectively same as max-age: 0, no caching. Except that it still spends time on trying to cache things. Not recommended.

Sorry for the accidental title change, we we're editing this simultaneously.

When I originally developed this thing, I first took a look at lazy building. Its problem here is that the lazy built value will be eventually cached by page cache, and it won't be loaded for the anonymous end user anymore, so we would end up in the same situation.

By reading the core commit, I got an idea of optimizing my code to avoid reloading the value from the backend, by duplicating the calculation in the JS end based on the data that could be sent as element attributes for instance. But that could make it quite complicated so I don't know if it's worth it.

Thank you for fixing the fatal error on the cron script. With this change - which is hopefully soon included in a release - the code bails out silently instead of breaking badly.

I still want to emphasize that no functionality should rely on an assumption that the database is used as a caching backend. On busy sites, this should never be the case, and they might not have the cache_render table in their database at all. I would prefer not having such code in the codebase.