Pushed a new PR targetting 2.1.x, there are a bunch of CS stuff that still needs addressing, but that's better left to be handled in a separate issue.
Also introduced a new DaemonBase class since the IP and UNIX daemon scanners have duplicated code.
This should make it easier to apply bugfixes for the daemons going forward.
codebymikey → changed the visibility of the branch 3058018-use-of-max to hidden.
Not working on this at the moment, just thought to document it.
Do you know what specific module also declares the FileValidationEvent::class event, enough for Clamav to be higher priority or stop propagation?
codebymikey → changed the visibility of the branch 3554832-check-flag-status-2.0.x to hidden.
Yeah, it appears this was fixed in a later release. Just added the the previous test case for completeness-sake.
If the field value is an empty value like FALSE or [], then the default is not currently used.
So updating the issue to support that instead, whilst also adding test cases for 0.
Added an initial draft to support the feature, it works well especially with multistep forms as per the "Steps to reproduce":
The code works for me, but needs review (and potentially more extensive tests if @joaopauloc.dev would like to add them as I'm having difficulty running functional tests with my current setup)
Thought I'd already given feedback on this, but yes, that all looks good to me. Thanks for fixing the duplicate value issue (a bit more elegantly) and adding the tests as well!
Marking as RTBC
codebymikey → changed the visibility of the branch 3554220-claro-variables-dependency-10.4.x to hidden.
Also had an issue with autocomplete styles being broken because of the claro/claro.drupal.dialog dependency.
I've created an issue 🐛 Claro's libraries don't enforce the variables.css dependency Active in core to address this.
Ran grep -l 'var(--' . -R | sort | uniq | grep -v 'pcss' on the claro theme, and it shows the following files has having custom properties:
./css/base/elements.css
./css/base/variables.css
./css/components/accordion.css
./css/components/action-link.css
./css/components/ajax-progress.module.css
./css/components/autocomplete-loading.module.css
./css/components/breadcrumb.css
./css/components/button.css
./css/components/card.css
./css/components/ckeditor5.css
./css/components/content-header.css
./css/components/details.css
./css/components/dialog.css
./css/components/divider.css
./css/components/dropbutton.css
./css/components/entity-meta.css
./css/components/fieldset.css
./css/components/file.css
./css/components/form--checkbox-radio.css
./css/components/form--field-multiple.css
./css/components/form--managed-file.css
./css/components/form--password-confirm.css
./css/components/form--select.css
./css/components/form--text.css
./css/components/form.css
./css/components/icon-link.css
./css/components/image-preview.css
./css/components/image.admin.css
./css/components/jquery.ui/theme.css
./css/components/media-library.ui.css
./css/components/menus-and-lists.css
./css/components/messages.css
./css/components/modules-page.css
./css/components/page-title.css
./css/components/pager.css
./css/components/progress.css
./css/components/shortcut.css
./css/components/skip-link.css
./css/components/system-admin--admin-list.css
./css/components/system-admin--modules.css
./css/components/system-admin--panel.css
./css/components/system-status-counter.css
./css/components/system-status-report-general-info.css
./css/components/system-status-report.css
./css/components/table--file-multiple-widget.css
./css/components/tabledrag.css
./css/components/tables.css
./css/components/tableselect.css
./css/components/tablesort-indicator.css
./css/components/tabs.css
./css/components/vertical-tabs.css
./css/components/views-exposed-form.css
./css/components/views-ui.css
./css/components/views_ui.admin.css
./css/layout/breadcrumb.css
./css/layout/card-list.css
./css/layout/form-two-columns.css
./css/layout/local-actions.css
./css/theme/field-ui.admin.css
./css/theme/filter.theme.css
./css/theme/install-page.css
./css/theme/maintenance-page.css
./css/theme/media-library.css
./css/theme/tour.theme.css
./css/theme/views_ui.admin.theme.css
So I've gone through the individual libraries and added them as a dependency.
Hi!
I'm aware that the PR appears a little bit complicated, but the batch part is tightly-coupled with the new composer architecture/hooks, and I'm afraid I don't have the bandwidth to work on it to split it into separate commits.
I feel the new commands and architecture are an overall net positive (since it does away with the manual configuration part)
So feel free to repurpose this issue to one regarding addressing the Drush deprecation, and testing locally and seeing if there are any issues with the behaviour.
I wonder whether we should detect whether the rotate_ie plugin is available before swapping the operation.
Yes, or better yet, check if the rotate plugin exists first, before falling back to rotate_ie (just in case it's been implemented in a specific installation)
Would you say this is RTBC then?
Given '#global_types' already defaults to TRUE, it doesn't need to be explicitly declared. Marking this as RTBC.
It's already been implemented locally, it's heavily based on the existing CustomValue plugin. I was just waiting on ✨ Allow the "Custom value" token property to be added to a specific datasource Active to land before applying those changes to it and pushing up.
The main use case for it is with the AI search module, as I think users would find it a lot easier to add "Contextual content" with Twig rather than tokens or having to write custom code.
It also provides more flexibility outside of just the AI module use case (as Twig makes it easier to index essentially most things without needing to implement custom modules or plugins as long as the necessary twig filters and functions exist for it)
One thing I found is that if a processor is locked and hidden, then the backend config ends up being updated to include it.
So if/once this lands, an update hook might also be necessary to resave the configs so that all the backend config is updated to include it.
Thanks @cherrypj,
On thinking on this a bit more, we might also need to weigh the benefit of having a canonical here in the first place (given the canonical is not much use to anyone), if we're able to remove it without causing issues like requiring an entity update etc, then I'd probably lean towards that over duplicating the edit route.
Thanks @scott_euser!
This would work, the only thing I can think of is if there are too many filters or complex structures, some providers might have a problem setting the tool correctly. Also if you have many indexes, you might get tool overload, but we have UI solutions for that coming up.
Was thinking of that, and ideally shouldn't need a deriver, it should be able to programatically build the normalized schema based on currently selected search_index, but I wasn't sure if that was feasible or not since from a cursory search, the tool was sometimes created without the necessary contextual information.
I'm guessing your idea is to derive one tool per index, so you know filters?
Yes, but if there's a way to somehow use the single ai_search_rag_search function and the context configuration to handle it, and only show the available filters for the currently selected search index, then that'd be perfect.
And if the index doesn't have any Filterable attributes, then the extra filter schema just wouldn't be present.
The second consideration you should take, is if it should be done in a reusable way as in https://www.drupal.org/project/tool → or if it is specific for Function Calling/AI? If you can see a use case for such a derived tool in ECA, we should add it there instead.
I wasn't aware of the tool module, it seems pretty interesting. For my specific use case, it's specifically for improving the quality of the responses received by the Function calling via AI by being able to optimize the query.
If the tool module seems like the better approach with its typed inputs/outputs, and it makes sense to without adding too much complexity, we might be able to leverage it as a way to build the relevant AI function as well.
If we add it in the RagTool we either need to make it conditional on if the backend handles filters. If we have that information it should be easy enough.
I assumed all RAG backends were able to handle filters to some degree (hence the presence of AiVdbProviderSearchApiInterface::prepareFilters())? So I was assuming even if the provider didn't support filters, it either wouldn't implement the interface or just wouldn't do anything with the filter information.
Either way, I'd assume this specific validation issue should've already been addressed before it gets to the RAG tool since if the VDB provider didn't support filters, then it shouldn't allow them to add "Filterable attributes" to it on the Search Index.
We already have the "Property restrictions" option within the AI agent page, so that could be a way for users to hide certain filter properties from being sent to the AI (if it's not supported or the desired behaviour).
Beyond that in terms of Tools/Chatbot, I am not 100% sure AI Search is the right component to actually carry out the work, unless you think there are things that are needing changing here to support what you propose?
I'm raising it within the AI Search tool because that's where the RagTool ai_search_rag_search function is defined, and it requires the additional context for the filters in order for the chat to properly interface with it.
Have not considered it myself, but maybe there is a better way to leverage Search API to not do a custom mapping of Search API filters to Postgres in the VDB Provider, could that maybe instead come from Search API Database to provide better support?
I'm not sure if Search API Database would be the right place for that. I think from reviewing most of the code, most of the providers seem to have lots of duplicated code that could be easily handled as part of the AiVdbProviderClientBase.
Given the ai_search is in charge of queries and interfacing with search_api in general, it makes the most sense for the abstraction layer to be there. The list of supported field types and index operators aren't that extensive, so I think it shouldn't be too difficult to map in the RAG tool schema. And given that most of the providers already have basic Search API filter support, there shouldn't be any issues here (worse case scenario is that it ignores the unsupported operator).
For example, if the user has "created_date" and "is_published" as Filterable attributes in the search index, then we could have a schema along the lines of this sent to the AI model as part of the RAG tool's function definition:
"filters": {
"type": "object",
"description": "Named field filters",
"properties": {
"created_date": {
"type": "object",
"description": "Filter by creation date",
"properties": {
"operator": {
"type": "string",
"enum": [">", ">=", "<", "<=", "=", "!="],
"description": "Comparison operator for date"
},
"value": {
"type": "string",
"format": "date-time",
"description": "Date value in ISO 8601 format (e.g., '2023-09-01T00:00:00Z')"
}
},
"required": ["operator", "value"]
},
"is_published": {
"type": "object",
"description": "Filter by published status",
"properties": {
"value": {
"type": "boolean",
"description": "True for published, false for unpublished"
}
},
"required": ["value"]
}
},
}
Then if it's populated by the AI tool, it can essentially pass that contextual data back into search_api fairly easily using something along the lines of this:
$queries = $this->searchString;
$query->keys($queries);
$filters = $this->getContextValue('filters');
if (is_array($filters)) {
$condition_group = $query->createConditionGroup($this->getContextValue('filter_conjunction') ?? 'AND');
foreach ($filters as $field_name => $filter) {
// As this is pseudocode off the top of my head, some operators/filters might need special handling, but this should essentially work similarly to if the search was made by Views.
$condition_group->addCondition($field_name, $filter['value'], $filter['operator'] ?? '=');
}
$query->addConditionGroup($condition_group);
}
$results = $query->execute();
If you are on Drupal Slack may get more help from #ai-contrib channel as there are more people there involved in Tools/Chatbots which might be better at suggesting where to contribute what you are proposing.
Thanks for the pointer, I'll have a look at it. I just thought it was easier to document here since most people with the same issue are likely to run into this first.
I don't strictly need the Filterable attributes at the moment (I'll probably need it eventually for accuracy/efficiency reasons), but it seems like a loose thread since most users would assume you should be able to interface with it when it's exposed as an AI tool function.
But yes, happy to get input from more core maintainers on this.
I'm using the Postgres VDB Provider → .
Given we already have the ability to take in filters, as well as pass $filters directly into the vectorSearch() method (which I'm guessing is how normal searches would work as well), I don't see why it couldn't be supported when called through the RAG tool function as well - we'd simply need to find a way to map the existing Search API filter conditions into a schema that the AI can interact with.
Then if the specific provider doesn't support filtering properly, then it'd be up to the user to determine if they'd rather switch providers to one that caters for it or not.
Ran into this as well while exploring the AI chat.
Given that the search_api modules provides a very easy abstraction layer for doing searches regardless of the backend, my initial idea regarding this was for us to create a deriver based on each SearchApiAiSearchBackend index, then expose all the Filterable attributes on each search index as properties (the schema for each property will be determined by the type, e.g. what operators are supported etc.) as part of the normalize function. Then the rest of the work will be left to the relevant AI model/prompt to determine how to use the tool.
Is there any reason this approach wouldn't be recommended?
Unsure if we also need to expose interface functions on the ConfigAiAgentInterface to identify when the max loop has been reached.
That way, the other AI modules can determine how to respond when the limit is reached.
It doesn't appear to be merged yet. The existing MR will need to be rebased on the latest dev.
I'll do that in due course, then we can look into pushing out a new release.
Merged the open MR, as well as updated to core_version_requirement: '>=8' → since the module is fairly simple and won't require much changes to support newer versions.
There's an open MR in 🐛 Clearing index data does not delete data/table for multivalue filterable attributes Active which should address this.
Provided a MR addressing the issues with multivalue fields not being created/deleted accordingly (the additional fields are only created when you resave the search api index rather than immediately after clicking "Clear all indexed data").
✨ Add optional RAW vectors in RAG results Active appears to have been merged.
Added better error handling when certain backend plugins are missing. It now logs them rather than crashing the page and giving a WSOD.
Also updated the search api server status page so that it provides a more useful overview of the status of the server, and in the event that the NewServerEventSubscriber event listener was unable to create the collection due to a misconfiguration or server being down. It creates the collection if it doesn't exist so that it can be indexed.
codebymikey → changed the visibility of the branch 3202631-10.4.x to hidden.
codebymikey → made their first commit to this issue’s fork.
Tests are passing and working for multiple sites, so marking this as RTBC.
I want to add the string \.deprecation\-ignore\.txt to the variable $_TEST_ONLY_FILE_PATTERN but if there is already a value in that variable, it needs a pipe | before the additional text
For future reference, this can be achieved using the :+ parameter expansion without using a subshell, so it'd look something like this:
# Add deprecation ignore text file to the extra file pattern.
_TEST_ONLY_FILE_PATTERN="${_TEST_ONLY_FILE_PATTERN:+$_TEST_ONLY_FILE_PATTERN|}\.deprecation\-ignore\.txt"
Example script showcasing this: https://www.onlinegdb.com/9hWM_JbBE
However it might be easier for people not as familiar with bash to review if the if statement version is used instead - entirely up to your discretions.
The output from a subshell is just a single string, so isn't available as an array to loop over.
A couple approaches to parse the output properly is available here: https://onlinegdb.com/6IsOZCVHQ9
codebymikey → created an issue.
Makes sense. I'll create an issue for it
codebymikey → created an issue.
The test-only job you linked ended green, which implies it is not doing what we expected. It has
Ooh, I wasn't actually testing for failures, more that it was now able to pick up the appropriate Test.php file as being changed. The diff between the commits doesn't seem to have any changes that'd trigger an actual failure, hence why it passes.
We need to decide how to treat the various combinations of input of the two variables, because running the job when target branch and baseline SHA are identical is just confusing.
Yup, most users shouldn't use the hash reference as the _TEST_ONLY_TARGET_BRANCH, which is why it initially defaulted to the CI_DEFAULT_BRANCH.
I just erroneously did it as a quick test since 520125b0b wasn't the head of a referenceable branch name and was curious how it'd work.
I think it should work as was originally planned:
1. _TEST_ONLY_TARGET_BRANCH - should be a branch (or a commit hash - it should work, but is not officially supported)
2. _TEST_ONLY_BASELINE - should be an optional commit hash. If it's not declared, it should attempt to resolve it from _TEST_ONLY_TARGET_BRANCH which should be a git ref that shares a common history as the current one.
I have a plan to pass in a 'this is d7' argument to the main script, and then do away with the -d7 file altogether.
That sounds like a good idea!
The composer.json is modified at runtime, and that is the only change detected. So more investigation is needed. Any ideas?
It was because the original git ls-remote --sort="v:refname" origin "$TARGET_BRANCH" | tail -n1 | cut -f 1 call was written to pick up branch/tag references, but wasn't made to handle scenarios where the _TEST_ONLY_TARGET_BRANCH is set to a commit hash.
I've pushed an update to cater for it. The updated pipeline's available here.
codebymikey → created an issue.
Based off the documentation
Using compare_to with merged results pipelines can cause unexpected results, because the comparison base is an internal commit that GitLab creates.
It appears due to how Gitlab checks out the repo, the compare only works for clean commits where the commit hash it's comparing to is within its commit history.
In this case:
1. 3523139-test-only-changes -> 0603fba1 gives you a valid test-only changes pipeline.
2. The latest commit on the d9-basic branch (3cbc82f1) doesn't exist on the 3523139-test-only-changes branch, so it's unable to provide you with a test-only change diff for it.
Was able to trigger it on the 3523139-test-only-changes branch with:
_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-part-2
# The test-only pipeline isn't available because the branches need rebasing so that they share similar commit references.
#_TEST_ONLY_TARGET_BRANCH: d9-basic
# This works because the commit reference exists relative to the current branch.
_TEST_ONLY_TARGET_BRANCH: 0603fba103772e05c2c1357e55af2186aaeb4bd6
# Mainly for debugging purposes.
CI_DEBUG_TRACE: true
tldr; due to how Gitlab internally checks out the code, the branch the pipeline is running from needs to have the target branch's commit ref somewhere in its commit history, so a rebase is most likely needed.
We could make a temporary change to this projects .gitlab-ci.yml page rules so that the job is run in this MR, for verification before merging.
That's a good idea, and I've made the changes so it's a manual pipeline for issue forks. I think it's probably worth keeping for testing future changes to the docs.
Sample doc page with relative images running on the issue fork is available here.
Not a blocker, but how will this impact local testing?
That was actually my initial test of the new variable, and it worked well running under localhost.
The updated trim filter has been added.
The test-only patch pipeline also showcases the behaviour.
codebymikey → created an issue.
This would allow the 'test only' job to be run in pipeines on non-default branches, when triggered by other means
Yup! That seems like a great idea actually.
It is a bit of a fuss, and we have tried various way to get round it, but it is not possible.
Yeah, it does seem like it. I think from memory/personal experience with gitlab, and based on how the current variable precedence work, the best way to work around this is to lazily load the environment variables (similar to the .calculate-gitlab-ref) such that:
1. The variable isn't set on the Project/Group variable since that takes precedence over whatever the project has in its .gitlab-ci.yml file.
2. The _GITLAB_TEMPLATES_REPO and _CURL_TEMPLATES_REF are left empty in the main template, but the documentation references what the default value actually is.
3. The _GITLAB_TEMPLATES_REPO and _CURL_TEMPLATES_REF variables are lazily exported in the gitlab-ci.yml script and set to the default value when empty (that way, the custom overrides should take precedence over the "default" empty value).
Switching this to Needs Work so that it's not automatically closed as Fixed.
The fallback option seems useful, the more flexibility the better.
I'd still personally recommend that users avoid using strtotime() blindly since as per the original issue, it can accidentally convert it into an invalid date, so your feed should ideally have a standardized format to convert from (however having the fallback option still wouldn't hurt for cases where the dates are entered manually).
All that being said, the code and tests looks good! RTBC.
As per the discussion in ✨ Allow users to specify custom files that Test-only changes Active , I've created an issue/MR for loading the relative markdown assets.
codebymikey → created an issue.
This feature is available in the recently_read → and message → modules.
codebymikey → created an issue.
megachriz → credited codebymikey → .
I wasn't aware that you could actually modify the manual pipeline variables before running them, so initially worked on supporting test-only changes via the web pipeline.
This can be tested on the https://git.drupalcode.org/issue/stage_file_proxy-3521812/-/pipelines/new issue fork.
Test case 1: Merge request
Previous behaviour: Tests can't be ran
New behaviour: Test runs and fails gracefully.
Test case 2: Manual pipeline with no tests
Branch: 3.1.x
Variables:
_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-patterns
_TEST_ONLY_FILE_PATTERN: stage_file_proxy\.services\.yml
CI_DEBUG_TRACE: true
Result - doesn't create the "Test-only changes" job, because there are no test file changes.
Test case 2: Manual pipeline with tests
Branch: 3521812-test-web-pipeline
Variables:
_GITLAB_TEMPLATES_REPO: issue/gitlab_templates-3539542
_GITLAB_TEMPLATES_REF: 3539542-test-only-patterns
_TEST_ONLY_FILE_PATTERN: stage_file_proxy\.services\.yml
CI_DEBUG_TRACE: true
Result - creates the "Test-only changes" job since there are actual test file changes before the current branch and the default one.
On a sidenote, even though the 3521812-test-web-pipeline branch has the _GITLAB_TEMPLATES_REPO and _GITLAB_TEMPLATES_REF variables overridden and hardcoded in the .gitlab-ci.yml, new manual pipelines which don't explicitly specify the override end up using:
_GITLAB_TEMPLATES_REPO: project/gitlab_templates
_GITLAB_TEMPLATES_REF: default-ref
Yeah sure, no problem.