Then we'll say RTBC :)
Yep, looks better and works better :)
Translate Text
Default provider: No default
That looks to be the issue: is it possible to set your chosen provider and model in that section?
The "Warning: Undefined array key "#entity"" error is fixed by the patch in the linked ticket, not by uninstalling any modules.
Setting back to needs work: the previous comment confirms it doesn't work if the user has access to multiple text formats, so this is not ready to merge. Also needs to address bringing the solution inside the existing JS, and removing the need for the form alter.
Just to add, the second error in the description is a duplicate of https://www.drupal.org/project/ai/issues/3513409 🐛 Warning: Undefined array key "#entity" in Drupal\ai_translate\Form\AiTranslateForm Active and should be resolved by the patch there.
Still require the information in my previous comment to investigate this further.
Still not working for me, I'm afraid. Steps to recreate:
- Clean site install.
- Enable AI CKEditor with patch above.
- Update Full HTML text format to use AI CKEditor. Ensure Basic HTML does NOT allow use of CK Editor.
- Create content type with WYSIWYG field that uses Basic HTML as default format.
- Create user with permission to use Basic HTML and Full HTML but NOT permission to use AK CKEditor
- Log in as user and create content with new content type.
The code in the form alter runs when the form is first opened and correctly identifies that a WYSIWYG field is in use and that the logged in user does not have the required permission. However, switching the field's text format from the default Basic HTML (no AI) to Full HTML (uses AI) results in the AI button appearing. There are no errors in console or in watchdog.
The form alter also runs now on every form on the site, even those that can never have CK Editor in use: I think this might be better if the access is checked within the existing CK Editor javascript so that it only runs when CK Editor is present.
Can you confirm which of these provider modules you have installed on the site? https://project.pages.drupalcode.org/ai/providers/matris/
I can't confirm your issue without being able to replicate your set-up.
As your screenshot shows, there is a message asking you to do something before you can translate the menu item, including a link to where you need to do it: I also need you to confirm you have done that, because otherwise it is likely the issue is you have misconfigured the module not that the module has a bug.
Sorry I'm losing you: are you saying "Yes I have followed the instructions to set up an AI Provider (can you link to it for us as well?) and have set the AI's modules default settings to say which provider to use for translations"?
It sounds to be like the most robust solution is for v2.0 to abstract what it needs to out of ai_agents to provide the infrastructure to build and run agents, with the separate AI Agents module then housing the "core" agents. The AI Assistant and AI Chatbot modules could then be deprecated in favour of AI Agents introducing a Chatbot agent, probably with an upgrade path between the two to help the early adopters.
The AI module is ballooning and getting very complicated to use and understand: it's trying to do everything without breaking older versions of itself, and provides multiple things that in practice aren't really that different. The Core AI module should really just be an API to send data of the different supported types to an AI and then do something with the reply: the specific implementations like "put the response into a WYSIWYG field" or "show the response to a user" or "create an entity from the response" should be handled by external modules you can install and use if that's the specific way you want to use AI.
That's going to need very careful planning and management though if updates aren't going to brick a lot of sites.
Can you confirm you have followed the installation instructions for the module, particularly the requirement to choose, install and set up an AI Provider to handle the connection to your chosen AI service? And if so, have you visited the AI Settings page to select a default provider and model for translations?
Applies fine and does what it says it will.
The default value functionality however requires you to set up a default when adding the field type to a node type, which isn't the behaviour of others field: default values are usually optional when setting up a content type, with a "Set default value" checkbox. I think users will expect it to work this way, given it is the standard for Drupal field types.
Possibly the field type should just be an additional item in the Reference category rathewr than its own thing when adding a new field - it doesn't do anything different to the other reference field types - but that may be a personal preference.
There seem to be some commits relating to AI Assistant Block access that have leaked into this MR.
Code all looks good an applies cleanly, but as this is such an under-the-hood change I'm finding it hard to reassure myself it has passed testing. Can you maybe suggest a test or two that would confirm it's working as expected?
This looks good and makes sense. Candidate for porting back to 1.0?
OK, well I get this every time I try to enable the 1.1 dev version, so the MR makes a change that resolves the issue for me: I'll leave it to the maintainers to work out if this is required,.
LGTM then :)
I'm not sure that's an answer to my question? I was asking about the status of this ticket currently being "active", which means it isn't ready to be reviewed or merged.
Tests still failing: https://git.drupalcode.org/issue/ai-3515209/-/jobs/4833128 - the $provider_id variable only exists if the $item is not empty, so it is theoretically possible that you then try to put a non-existent variable in the $form_state. The easiest fix is to give the $provider_id a NULL value outside of your if statement.
Couple of suggestions to improve the code.
mrdalesmith → created an issue.
Fixed the errors in the MR and updated the branch. Attempted to test, but cannot find how to trigger these particular tools to confirm they are working.
mrdalesmith → made their first commit to this issue’s fork.
Looks good, works for me. Constraint exists in 10.2 so fine to replace, in 1.0 and 1.1
Looks good and a simple fix that won't break anything: should this issue be in needs review @ipumpkin?
I'm not quite getting from the description and a review of the codebase exactly what changes you're suggesting here: is the chatbackendInterface intended as a potential drop in replacement for the AiAssistantRunner that sits on the block forms? As far as I can see the chatbots are just forms wrapped around the ai_assistant_api's functions so it isn't immediately obvious which bit you think might be possible to make more flexible.
The MR is in draft: it would be good to remove that if you have finished with it, regardless of the maintainers decision about whether it can be merged yet.
Applies cleanly, works well for meL: I *much* prefer this flow which makes a lot more sense than the current one.
Works fine, applies cleaning, but I don't think the update hook should assume that all users want this enabling by default. I think it would be more appropriate to add it as disabled and show a message in the update hook to advise it is available if needed.
The AI recipe for the Drupal CMS project contains working configuration for an AI Chatbot: it migt be worth in the first instance looking there to see if you can identify any issues with your set-up.
The code that gives that particular error message has the comment "// Anything else is probably due to a bad request." so it would sound like the issue is with how you've set it up.
Failing tests.
That works very nicely and is a good idea. I can't quite shake the suspicion that this is a little non-standard, as a lot of the other buttons on the CKEditor toolbar *could* also work the same way (formatting, font choices) but don't. I'll let the maintainers have the final decision, but at the very least it might be nice to make this something you could turn off in a setting?
No problems as such, but this might be better moved into the blockAccess() method to ensure it is not shown anywhere if the related assistant isn't available?
This doesn't appear to be an actual issue, just a way of documenting a fix for future readers? I'm closing the ticket but please update if there is an issue requiring addressing here.
Yeah the patch doesn't work because the form_alter only adds the library to the filter_format_edit_form form, so the javascript never makes it onto the page when content using CKEditor is being created/edited.
I think this may need a rethink @anjaliprasannan - all you're doing is hiding it when the filters are being edited by admins.
I'm not 100% convinced this is the right way to go about it, as we've got multiple "things" using "AI" that have multiple ways of telling Drupal they need specific modules to do it and it feels like a lot of complexity and duplication: certainly with this change, if I understand what's going on (which I can't guarantee) the isAvailable functionality feels pointless - if the dependencies required aren't available, the plugin will not instantiate so isAvailable() can't be called.
But there's a MR for review and further discussion, anyway.
I have erred on the side of caution with the DownloadFilefromUrl function, which requires the file module to be enabled if the option is selected to save the downloaded file as a Drupal file entity, but there are strong arguments to be made either way for that.
mrdalesmith → created an issue.
The plugin also assumes that the site is configured with a provider with 'chat_with_image_vision' capabilities: should we fix this at the same time?
mrdalesmith → created an issue.
Are you saying you want some kind of system that works without instantiating the plugin?
I believe I put in access checks on the menu items for these that did this, and they got slowly removed so that the menu items could appear instead and show a message saying there was nothing to see. It's probably worth defining the expected behaviour here so we can be certain every agrees and it does what they expect in all situations.
This is closed, but see the link in #4
You have failing tests. I thought it might be better to address the issue within the existing JS, but I think this is a fairly elegant solution.
That would be https://www.drupal.org/project/ai_provider_openai → - the providers are no longer part of the AI core module
That would need to be an issue in the Open AI provider's queue.
The content type AI Agent has:
/**
* {@inheritDoc}
*/
public function isAvailable() {
// Check if node module is installed.
return $this->agentHelper->isModuleEnabled('node');
}
so I'm not entirely sure how you got it to be available on a site without node enabled?
Can you provide steps to recreate?
The CKEditor plugins don't appear to have a concept of access at the plugin level, so this would probably need to be handled in the aiui.js javascript file, which would need to be able to check the permissions of the current user.
That feels like overkill - it's clearly fixing a common mistake most people do when adding css classes to render arrays: are we really going to add overhead to say this specific one has been added correctly?
This is the issue https://www.drupal.org/project/ai/issues/3510599 ✨ Allow skipping of moderations for some embeddings (controlled input) Active is attempting to resolve, so I'm closing this as a duplicate.
This will require some kind of idea of the direction/method of expansion.
My 2p worth is that this wouldn't be an effective use of anyone's time: Drupal seems to have acknowledged that distros have severe limitations, particularly around the need for them to be kept up to date with every security release of the modules in them to prevent people getting locked to insecure versions.
The recipes push seems to be intended to replace distros, and spending time porting the recipe to effectively a lesser copy of the AI recipe and then having to keep it up to date doesn't seem like something we should be doing.
Hopefully this issue means something to someone
Across both? Think we may be missing a word or two.
The tests are failing because you are using Drupal::service() inside an object: you need to be using dependency injection to get the Ai provider service in there. See https://drupal.stackexchange.com/questions/224247/how-do-i-inject-a-depe... for a steer if you don't know how to do it.
Any reason why this has been set to needs work @smustgrave? There are no comments to advise what needs doing, and it's a very simple bit of code: I can't think what additional changes are needed?
As far as I can see, this is a safe change and makes sense to kleep the css load down.
@ishani patel - your video shows that you have the text format set top. basic: the error does not occur if "Limit allowed HTML tags and correct faulty HTML" is checked for the format you are testing against. See #7, #9 and #11
LGTM
looks good and works for me.
Change applies cleanly, test pass and the correct dependency is now added to the exported config. I think this is OK to go.
Not 100% sure about the message being set - see comments on MR.
Possibly unrelated to this issue, but this change would effectively make the "create term if it doesn't exist" setting impossible to use on any term vocabulary that had required fields: you would never be able to create the terms as the validation would always fail but the edit form has no ability to let you set the field values on the new term.
My personal preference here would be the current behaviour - stub taxonomy created that requires future updating - so that the setting on the field actually has a function. I can understand that not everybody would, however: perhaps we need some kind of validation on the field settings form that advises terms with required fields can't use this setting (if this change is ti progress)? Or at least a warning in the help text?
Works perfectly for me. RTBC?
Looks good to me, and the random test failure has been resolved by a re-run.
Looks OK to me.
That seems to be related to fixing https://git.drupalcode.org/issue/ai-3513409/-/jobs/4714593 so I'm not sure what is going on here: I may need a maintainer to look at confirm what the issue is.
OK: there's value in what you've done - and the test fail was my config not yours - so I've committed the non-gitlab related changes you've made and tried to give you credit: dunno if it works when the MR itself hasn't been merged, but we'll see. Thanks for your time.
Your fails are all to do with the config schema you've added, which is using string types for integer variables.
TBH the reason I've never added gitlab cl to the project is that the tests don't provide any real coverage: they are boilerplate tests added by drupal console when I used it to create the module template. The only thing being tested is that the front page still loads once the module is enabled. Whilst there is some use to that, I'm not certain it's worth adding the MR until the tests do a little more than that.
I also do not like cspell as a test - tests failing because a dictionary doesn't recognise a word like "patreon" or the module code is written by a British English writer doesn't seem like a useful thing to test. I'm not sure I want the overhead of maintaining the spell check side of things as I see very little benefit from it.
This issue is closed so you'd need to check the parent issue; I believe its already been fixed there although this may be a new bug.
Whilst the editor shows the correct link has been added, by the time the form values are validated the original link is in the form state values. Accordingly, this looks to be a problem with the CK Editor JS not getting the value into the Drupal side correctly: clicking the "edit Source" button shows the old URL in the HTML even when the editor shows the updated version.
Checking "Limit allowed HTML tags and correct faulty HTML" on the Full HTML text format resolves the issue, so it would appear this bug is related to something that isn't done if the HTML tags don't need to be stripped.
Can confirm the error exists on latest clean installs of D10.4.5 and D11.1.5, and as mentioned in #7 it only occurs when the text format is set to Full HTML.
Patch doesn't cleanly apply to the fork branch, so will need reworking so that it does to allow testing. It might be worth considering if we need all these separate events or whether they can be consolidated into a single one, possibly in consideration with https://www.drupal.org/project/ai/issues/3506391 ✨ Alter PreGenerateResponseEvent response Active .
mrdalesmith → made their first commit to this issue’s fork.
LGTM
Just flagging that this is going to conflict with the solution in https://www.drupal.org/project/ai/issues/3513409 🐛 Warning: Undefined array key "#entity" in Drupal\ai_translate\Form\AiTranslateForm Active - just out of curiosity, does the issue still exist in your supported module if you use the patch from that ticket?
It's probably me, but following those instructions I just get the standard image upload field not the image I chose for the final step. I think I will let someone more familiar with the automators test this because I can't confirm the change is working.
This is an odd situation caused by the providers being moved into individual contrib modules I think: the AI module itself provides the base provider that requires a KeyRepositoryInterface to create any new provider, and provides the base loadApiKey() method that they uses the KeyRepository. Given that the code using the key module lives in the AI module, there's an argument the requirement is in the right place.
So code-wise the AI module contains nothing that requires Key, but the provider could be relying on the AI module to pull it in for methods it implements without the word KeyRepository ever appearing in its code. (OpenAI for example does rely on the Key module for storing its keys in its settings form, but doesn't have any code using it in its provider because this is inherited from the base module's code).
It's going to be tricky to implement a solution that doesn't break anything else, and the work may well need to be carried out and co-ordinated across a number of different codebases at the same time.
Not sure I follow you @mglaman.
LGTM
Failing tests.
LGTM
I think this might be more involved that just removing the dependency, because the KeyRepositoryInterface is explicitly referenced in the construct statements for AiVdbProviderClientBase and AiProviderClientBase and so even providers that don't use Key to save the credentials will still break if it isn't there.
This will probably require refactoring so that Key becomes an optional dependency and the KeyRepositoryInterface can be nulled .in the provider constructs.
It would be good to get some maintainer feedback on whether that's a good idea before we do anything though.
Works Ok for me.
This issue is resolve in an MR in https://www.drupal.org/project/ai/issues/3513409 🐛 Warning: Undefined array key "#entity" in Drupal\ai_translate\Form\AiTranslateForm Active
What we are seeing here is the difficult in using this module with any other translations because of the issue noted in https://www.drupal.org/project/ai/issues/3497214 📌 [AI Translate] Module uses Form that isn't a form Active - the existing code builds its own custom form so it can add in one column to the translation page, and depending on which particular controller is used that can remove a lot of important data.
What I've done in https://git.drupalcode.org/project/ai/-/merge_requests/516 is attempt to resolve this situation by altering whatever existing page we get to add in our extra column. If anything goes wrong, it falls back to just showing what was there before ai_translate was turned on, so at the very least it won't cause major issues.
The specific issue here was unrelated to ai_tmgmt - the problem existed with its dependency tmgmt_content so would have occurred without ai_tmgmt being installed. I'm not sure of the use case for having both ai_tmgmt and ai_translate enabled at the same time (since both send content to the AI to be translated) but if you need to, this MR should stop it causing issues.
OK, I'm going to have to leave this one: code looks good but I clearly don't know enough about the automator chains to properly set up a test.
I've reported that last comment as spam as this is an issue with this module, not a request for alternate modules with similar functionality.
Can you add some steps to recreate the issue; i can't work out what set up I need to carry out to test this patch.
I'm not really the person to make the call, but I can see that we do need two separate buttons: some people may wish to reorder the providers, whilst others may want to save the position they have been manually put into by them. I don't think a single button could suit everybody (which maybe why there wasn't originally any functionality to enable/disable the providers from this listing).
LGTM