I fixed the TestWith attribute.
Failing tests are functional javascript tests now, related to open telemetry. Doesn't seem related to this MR.
Tests seem to be acting up...
They are passing locally though, so setting to Needs review until further notice.
Note also that the numeric block_id and block_revision_id get exported, which is not really interesting for use in recipes, since that ID could be in use by another block already when importing the exported content via a recipe.
I'm not sure if the import system will strip that out or not, but on Drupal 11.2 with contrib default_content I had to jump through a bunch of hoops to avoid that being an issue.
Thanks for the MR!
I finally got around doing a review, and have added comments on the MR.
Can you have a look at those, and update where relevant?
I helped setting up pipelines with kraut (mostly just pushing 1 button ;)), tried to help Greece and Germany, and tried getting the drupal.be website up to date with upstream.
Improved the multiple invocations test coverage as well.
Tests keep failing, but the failures seem unrelated to this functionality.
The duplicate array values is still an outstanding issue though...
If there are no objections, you can add me as an administrator for Drupal BE:
name: Sven Decabooter
email: info@drupal.be
I noticed another potential issue when testing this: should we account for duplicate array values?
Take for example:
mymodule.settings:
simpleConfigArray:append:
property: something
values:
- a
- b
Given that recipes are idempotent, and thus can be run multiple times, this then results into config like:
mymodule.settings:
- a
- b
- a
- b
- a
- b
(if the recipe / config action gets applied 3 times)
As discussed on Drupal Slack with phenaproxima, I also think support for config entities is out of scope here.
Adding quote by phenaproxima here for future reference:
The presumptive reason being that doing this for config entities would be a more semantically meaningful action that might call for a completely dedicated, specialized action per use case, rather than generic array manipulation.
- Invoking the config action with a non-array value does not work currently. This needs to be added, with test coverage.
- Multiple invocations do work, and there is the
testSimpleConfigArrayMultipleInvocationstest. It only tests multiple invocations to the same property though, not to separate properties.
Linking the issue in the custom_field issue queue, where this functionality could be added
This seems to fix D11 compatibility.
Can this be merged and a new release tagged?
+1 - this module is blocking us from upgrading a site to D11.
The dependency change is probably not needed.
I thought it would remove the ai_prompt entity upon uninstalling the ai_translate module, but after some extra testing, it seems it does not.
It just lets the entity exist. But the prompt type does get removed, so a stale ai_prompt entity remains, without a valid prompt type.
Not sure if there is a way to get that entity removed as well (if that is desired).
Maybe general logic for deleting ai_prompt entities when their dependencies are removed, is something for a separate issue?
Created a new branch with fixes I noticed were missing in the merged functionality.
Could you review and add as well?
Test seems to fail - still needs to be addressed.
svendecabooter → changed the visibility of the branch 3.x to hidden.
svendecabooter → changed the visibility of the branch 3010914-product-variation-access to hidden.
@valthebald
This logic was taken from the documentation in the AI module.
Seemed logical to me, but indeed the update hook would just need to focus on the prompt that is currently active on that particular website, not what the module ships with by default...
Hi richgerdes,
Thanks for the feedback. Moving this back to the original queue.
I haven't been active in the queue indeed. We're using the module on a few sites, and it mainly works as intended, so no direct need.
The most urgent need is a D11 release indeed, so I'll check if I can get that out of the door.
I'll then look at the roadmap to see if I can assist with moving some of these things along.
If there is no response from the project maintainers, I'd be willing to volunteer as co-maintainer as well, to move this module forward.
I can opt projects into security advisory coverage.
I could then look into working together with sokru where relevant or appropriate.
Let's see if there is any response here.
Moving to the Drupal.org project ownership queue, as per instructions in https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → , since there has been no reply for 14 days.
Yes, since 1.2.x requires ^10.4 || ^11, I figured it was safe to use those.
See draft MR for a proof of concept of how this could work.
This would validate different prompt types differently.
See screenshots from output of the
Config Inspector →
module:
ai.ai_prompt.suggest_tags__suggest_tags_default
Uses general ai.ai_prompt_text.* schema
ai.ai_prompt.ai_translate__blaat
Uses specific ai.ai_prompt_text.ai_translate schema
The reason why the '#parents' property might need to be added, might need to be clarified, since I don't know exactly.
Is it because the form element is rendered within a tree structure, that it might be needed?
Or because there might be multiple ai_prompt form elements on the same form page?
I had a scenario where both of those were true, and adding it made everything work for me, but I don't know which of the scenario's it fixed exactly :)
svendecabooter → made their first commit to this issue’s fork.
Wouldn't it be a better DX if there were a dedicated config action in the Key module?
Then developers would more easily find out how to do so.
Updated the user stories in the original description, for extra clarification.
All dependencies of this module now have a release, so I tagged an alpha2 version.
This should allow the module to work without patches.
Thanks for considering linking to it from the project page.
Thanks for the heads up.
It seems to work fine on my clean install, but perhaps I have another library activated that uses the once library on the page.
Closing this issue again, in favor of the followup issue
🐛
Add drupal/once library dependency
Fixed
svendecabooter → made their first commit to this issue’s fork.
svendecabooter → made their first commit to this issue’s fork.
I think OR logic should be fine...
You are giving explicit "Create AI translation" permission to a given role, so it's not unreasonable you would expect them to be able to create AI translations then, even if they can't create manual content translations...
Thanks for the MR update. Looks good to me.
I will update the logic in
https://www.drupal.org/project/advancedqueue_mail →
, so it can keep working with the newly proposed event names.
svendecabooter → made their first commit to this issue’s fork.
Fixed by https://www.drupal.org/project/ai_usage_limits → module.
https://www.drupal.org/project/ai_usage_limits →
is a module that provides usage limit control, although it is limiting the usage per AI provider, not on a per-user or per-role basis.
That might be a good idea for an additional feature in the module (or a submodule).
Could the
https://www.drupal.org/project/ai_usage_limits →
module help in any way with the usage costs parts, or vice versa?
It does provide the option to calculate the current token cost per provider, and enforce a limit based on that.