Merge Requests

More

Recent comments

🇮🇳India prashant.c Dharamshala

Due to the fatal error issue, changing the status to NW.

🇮🇳India prashant.c Dharamshala

Similar to the https://www.drupal.org/project/ai_agents/issues/3516448 🐛 Fatal Error on enabling Active but steps to reproduce are different i.e occurring on Assistants.

🇮🇳India prashant.c Dharamshala

I think it would be better if Marcus could confirm whether the placeholders not implemented for advanced mode are done on purpose or missed.

🇮🇳India prashant.c Dharamshala

This needs to be addressed for all the explorer types available.

🇮🇳India prashant.c Dharamshala

You may try adding WebP to this and see whether the model supports it or not. If it is, then you may add webp to the allowed extensions list.

🇮🇳India prashant.c Dharamshala

Reviewed the MR and left a feedback to be addressed.

Additionally @kanchan bhogade tried to upload a PNG which we is not allowed and should have given a validation message. PNG is also a very common image extension, should we consider allowing it also?

🇮🇳India prashant.c Dharamshala

Added the code to have the "Placeholders available" section for the Advanced mode. Needs to be reviewed.

🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala

Is this related to this https://www.drupal.org/project/ai/issues/3525025 🐛 Missing button on Ai Explorer for Speech to Speech Active or different?

🇮🇳India prashant.c Dharamshala

I suggest not displaying the link to the explorer in the explorers' listing at all if the currently configured provider(s) do not support an operation. If that is already working, then we do not need to do anything in this, I guess.

🇮🇳India prashant.c Dharamshala

@anjaliprasannan

It would be helpful to add more details to the steps to reproduce, possibly with a screenshot. Regarding the allowed extensions, as far as I know, the field extension might be hardcoded.

🇮🇳India prashant.c Dharamshala

Rebased, moving back to NR, in case I missed something.

🇮🇳India prashant.c Dharamshala

Not sure why it was kind of markdown code with ```html , the changes fixes the issue. A rebase was needed which is done. Requesting maintainers to review.

🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala

The following warning seems unrelated to the current MR :

1. Ai Search Setup My Sql (Drupal\Tests\ai_search\Functional\AiSearchSetupMySql)
⚠ Field indexing options
✘ Search view

├ Exception: Warning: Undefined array key "title"
├ Drupal\views\Views::fetchPluginNames()() (Line: 157)

Kindly rebase and move it to NR.

🇮🇳India prashant.c Dharamshala

@anjaliprasannan

You screenshot shows "Translate using gtp-4o" but try to check when neither provider nor model is set.

🇮🇳India prashant.c Dharamshala

@jonnytoomey

I could not replicate this issue. I believe there could be some custom javascript code or library that you might be using which is trying to find the element which is not visible on the page hence throwing this error in browser console.

🇮🇳India prashant.c Dharamshala

Steps to reproduce would help here.

🇮🇳India prashant.c Dharamshala

@ishani

You may try to add a validation message in this case when the provider and model are not set for the translation, and the user tries to translate by clicking the "AI Translations" link/button.

🇮🇳India prashant.c Dharamshala

@anjaliprasannan, if you could address the review feedback points, that would be great.

🇮🇳India prashant.c Dharamshala

@_randy

Could you please add more details to the steps to reproduce, like which version of Drupal core is a dev version, like (11.x) or 11.1 or 11.2, etc, that will help replicate the issue?

🇮🇳India prashant.c Dharamshala

@marcus_johansson I am not able to replicate this atleast on 1.1.x.

🇮🇳India prashant.c Dharamshala

A couple of points that need to be addressed:

1. Ai Search Setup My Sql (Drupal\Tests\ai_search\Functional\AiSearchSetupMySql)
⚠ Field indexing options
✘ Search view

├ Exception: Warning: Undefined array key "title"
├ Drupal\views\Views::fetchPluginNames()() (Line: 157)

2. "Choose Model" should not appear when there is no provider available in the "Default Provider" select list for example in Image classification operation type

🇮🇳India prashant.c Dharamshala

Did not work for me.

1. I have 2 vocabularies Tags and Test
2. Tags has no terms
3. Test has 1 term
4. It is not listing any of these, in addition to that "Use source vocabulary" also disappeared after switching to the MR branch.

It needs work.

🇮🇳India prashant.c Dharamshala

Left some review comments, moving to NW. Kindly address those and move back to NR.

🇮🇳India prashant.c Dharamshala

On the page /admin/config/ai/settings getting the following error:

Fatal error: Class Drupal\ai\OperationType\DocumentToText\DocumentToTextInput contains 3 abstract methods and must therefore be declared abstract or implement the remaining methods (Drupal\ai\OperationType\InputInterface::getDebugData, Drupal\ai\OperationType\InputInterface::setDebugData, Drupal\ai\OperationType\InputInterface::setDebugDataValue) in /var/www/html/modules/contrib/ai/src/OperationType/DocumentToText/DocumentToTextInput.php on line 11

🇮🇳India prashant.c Dharamshala

Thanks for the great work done @joevagyok on this but there is no activity from last 6 months changing the assignee to unassigned.

🇮🇳India prashant.c Dharamshala

@marcus

I have done the following:

🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala

@ishani

I tried it on 1.1.x of ai and without even entering anything in these fields i got some response data. In addition to this also got the validation message but the form still got submitted it seems.

🇮🇳India prashant.c Dharamshala

With the help of an AI code assistant attempted the solution, which does the following:

  • Created a base plugin class (OperationTypePluginBase) that implements the OperationTypeInterface
  • Created a custom discovery class (OperationTypeDiscovery) that:
  • Discovers interfaces in the OperationType directory
  • Creates corresponding plugin class names
  • Maintains the existing interface structure
  • Works with the new attribute discovery system
  • Updates the AiProviderPluginManager to use the discovery system

Possible benefits of this approach:

  • Maintains backward compatibility - existing code using interfaces continues to work
  • Works with the new attribute discovery system
  • Doesn't require moving or restructuring existing interfaces
  • This could be replaced/rewritten when a proper plugin system for operation types is in place going forward
  • Now discover operation types from interfaces

Needs a thorough review of the code.

🇮🇳India prashant.c Dharamshala

Need phpcs fixes to let the pipelines pass and it should be good to go.

🇮🇳India prashant.c Dharamshala

I tried multiple tweaks, but nothing worked. I think the operation types are not getting discovered because they are not plugins.

Can we go with the approach of 1.0.x:where it is working fine

https://git.drupalcode.org/project/ai/-/blob/1.0.x/src/AiProviderPluginM...

🇮🇳India prashant.c Dharamshala

Raised the MR against 1.1.xby fixing the logic to handle the Ajax loading of models per provider. Changes need to be reviewed.

🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala

Different regions on the same page make sense, but what if both blocks are placed on different paths?

In case the response block is not on the same page, there are errors throw,n and the user/admin does not know whether it is working or not

🇮🇳India prashant.c Dharamshala

Thank you, @dcam, maybe I misunderstood the issue, so now hyphens are allowed whereas underscores are not.

🇮🇳India prashant.c Dharamshala

Yes, I was just going through the files in core/lib/Drupal/Core/Config/Checkpoint. You are correct, the underlying API does not provide a way to change the label. Then, in that case, you may close this issue.

Thanks

🇮🇳India prashant.c Dharamshala

Will try to add test coverage, if hyphen(-) is used in the machine name, it will get replaced by underscore(_).

🇮🇳India prashant.c Dharamshala

For this we only need to replace -with _in the replace_patternand replacekeys of the machine_nameelement.
This will allow lowercase letters, numbers, and underscores, and the "replace" key will replace other special characters with _.

Please review.

🇮🇳India prashant.c Dharamshala

Thank you @jrockowitz. This is extremely helpful to have these listed at one place.

🇮🇳India prashant.c Dharamshala

Not sure, but this https://www.drupal.org/project/ai/issues/3519196 🐛 Handle errors for API explorers when a provider not configured Active might be related.

🇮🇳India prashant.c Dharamshala

The search module does not yet have any functional JavaScript tests. So, this is the only file for it. However, the search module has another test type, functional, kernel, and unit, where this won't fit AFAIK.

🇮🇳India prashant.c Dharamshala

The issue mentioned in #7 🐛 Text formats "Embed Media" filter throws error Needs work was related to validation error itself. Added validation to check if the Medie embed filter is enabled.

🇮🇳India prashant.c Dharamshala

No status message appears in the functional test after "Save configuration", which has the text The text format ckeditor5 has been updated.However, this status message is being displayed correctly on manual testing.

Based on this, I removed the code
$assert_session->pageTextContains('The text format ckeditor5 has been updated');

If there is some alternative to this, please suggest.

Thanks

🇮🇳India prashant.c Dharamshala

This issue is from the core search module itself. I have provided a fix for Drupal 11 in this MR: https://git.drupalcode.org/project/drupal/-/merge_requests/11911. An attached patch could be applied to your Drupal 11 core to have it fixed until it gets merged to core.

🇮🇳India prashant.c Dharamshala

Ah, sorry, my bad, I somehow misunderstood The proposal was to only show the module's machine name if it is materially different., thanks for clarifying.

Works well otherwise.

Thanks

🇮🇳India prashant.c Dharamshala

@nicxvan

I am on branch 3250376-uninstall-page-showswith the latest code pulled, and I am still seeing the module names on the "Required by" section like the following ones:

Required by: Text, Block Content, File, Text Editor (editor), CKEditor 5 (ckeditor5), Comment, Datetime, Field UI, Node, History, Image, Link, Custom Menu Links (menu_link_content), Menu UI, Options, Shortcut, Taxonomy
Required by: Custom Menu Links (menu_link_content), Menu UI, Shortcut

🇮🇳India prashant.c Dharamshala

A review of the current code is needed before moving forward with this. After that, we can go ahead with writing tests for this.

🇮🇳India prashant.c Dharamshala
  1. Type "modal" worked fine.
  2. Fixed issues for type "off_canvas"
  3. Handled for the "off_canvas_top"

To quickly test, return the following code in a block or controller:

Open link in a "modal" window:

    $url = Url::fromRoute('entity.node.canonical', ['node' => 1], ['absolute' => TRUE]);
    $link = Link::fromTextAndUrl($this->t('Read more'), $url);
    $link->openInRenderer('modal');
  return [
      'read_more' => $link->toRenderable(),
  ];

Open link in an "off-canvas" window:

    $link->openInRenderer('off_canvas');

Open link in an "off_canvas_top" window:

    $link->openInRenderer('off_canvas', ['position' => 'top']);
🇮🇳India prashant.c Dharamshala

Have just created an MR from the patch submitted in #18 . Going to test these methods locally first.

🇮🇳India prashant.c Dharamshala

Thanks @sdhruvi5142 for testing the changes, code review also needed for this, changing to NR.

🇮🇳India prashant.c Dharamshala

prashant.c changed the visibility of the branch 2944554-allow-easily-creating to hidden.

🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala

Was trying to find occurences of

$url->setOption('query', \Drupal::destination()->getAsArray())

and $url->setOption('query', ['destination' => 'other/path'), could only find one, pushed that change.

Tests were added, so I am removing the tag for now.

🇮🇳India prashant.c Dharamshala

prashant.c changed the visibility of the branch 10.4.x to hidden.

🇮🇳India prashant.c Dharamshala

Needs a review, and then the changes could be backported.

🇮🇳India prashant.c Dharamshala

Great find. Was able to replicate easily on 11.x as well and got the following error, while creating any type of search page (node, user and help):

The website encountered an unexpected error. Try again later.

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "search.view_content" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 214 of core/lib/Drupal/Core/Routing/RouteProvider.php).
Drupal\Core\Routing\UrlGenerator->getRoute() (Line: 290)
Drupal\Core\Routing\UrlGenerator->generateFromRoute() (Line: 105)
Drupal\Core\Render\MetadataBubblingUrlGenerator->generateFromRoute() (Line: 773)
Drupal\Core\Url->toString() (Line: 176)
Drupal\Core\Utility\LinkGenerator->generate() (Line: 104)
Drupal\Core\Render\Element\Link::preRenderLink()
call_user_func_array() (Line: 107)
Drupal\Core\Render\Renderer->doTrustedCallback() (Line: 848)
Drupal\Core\Render\Renderer->doCallback() (Line: 393)
Drupal\Core\Render\Renderer->doRender() (Line: 203)
Drupal\Core\Render\Renderer->render() (Line: 491)
Drupal\Core\Template\TwigExtension->escapeFilter() (Line: 184)
__TwigTemplate_cb3fcbd5c3a204683387ed7f057a01d2->doDisplay() (Line: 388)
Twig\Template->yield() (Line: 344)
Twig\Template->display() (Line: 359)
Twig\Template->render() (Line: 51)
Twig\TemplateWrapper->render() (Line: 34)
twig_render_template() (Line: 369)
Drupal\Core\Theme\ThemeManager->render() (Line: 452)
Drupal\Core\Render\Renderer->doRender() (Line: 465)
Drupal\Core\Render\Renderer->doRender() (Line: 465)
Drupal\Core\Render\Renderer->doRender() (Line: 203)
Drupal\Core\Render\Renderer->render() (Line: 242)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 599)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 235)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare() (Line: 131)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse() (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray() (Line: 246)
Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}() (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners() (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch() (Line: 188)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 715)
Drupal\Core\DrupalKernel->handle() (Line: 19)
🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala
  • Rebased the merge request and resolved minor PHP CodeSniffer issues.
  • The current permission name, "View own account details", is too generic for its actual purpose.
  • This permission specifically controls access to the roles section on the user account edit page.
  • Even without this permission, users can still view and edit their own account details.
  • Ideally, if we retain the current permission name, it should only apply to the "/user/[uid]" page, and not to "/user/[uid]/edit".
  • In my opinion, for better clarity and purpose alignment, a more appropriate permission name would be something like "View user roles" or "View own roles".
🇮🇳India prashant.c Dharamshala
  • Rebased the branch with 11.x
  • Reviewed the changes, was able to search the module by machine name as well as the human-readable name
  • Attaching the GIF
  • Keeping in NW as Accessibility review and change records are pending

🇮🇳India prashant.c Dharamshala

prashant.c made their first commit to this issue’s fork.

🇮🇳India prashant.c Dharamshala

I have struggled with this a lot while uninstalling a module from the UI, it confuses when the name of the module is totally different from its machine name; you mostly go back and double-check it.

Having this added to the core would be very helpful to the site builders.

🇮🇳India prashant.c Dharamshala

Was looking for the generic methods for the same. I have raised an MR with some additional checks, like the path should be internal only. Once this is reviewed, we can go ahead with writing tests also.

Current changes need to be reviewed

🇮🇳India prashant.c Dharamshala

I came across this comment https://www.drupal.org/project/drupal/issues/3028868#comment-13812371 , which highlights that the Drupal image module does not support SVGs for a variety of reasons. Given this, it is unclear whether the current issue is still relevant.

Consequently, it appears that utilizing the SVG Image module might be a more suitable alternative, as it offers a widget and formatter for handling SVG fields.

🇮🇳India prashant.c Dharamshala

@oakulm It would be great if you can remove the draft mode of the MR, however the change looks fine.

Production build 0.71.5 2024