🇳🇱Netherlands @askibinski

Sure!

The main other issue I had (beside the PR) is that in SearchApiAiSearchBackend the search methods have tied $collection_name (and database) params to the server config as for example in #7 above. For more flexibility it would be nice to be able to override this in the plugin if you want to (which isnt possible now). Only the filter param can be set in the plugin.
But I think that could be a separate issue?

Just adding a patch file here for reference so I can link to it from mysql vdb provider → which requires it. It's similar to the MR but also patches the backend server to fallback to index name for collection (if not given in config) and server name as database name (if not given).

@scott Thankt for the update, that makes sense, I'm perfectly fine with postponing this for a next version because there are other areas too which would need attention, for example doSearch which uses the backend configuration collection name and passes it to vectorSearch without any index context.

  /**
   * Run the search until enough items are found.
   */
  protected function doSearch(QueryInterface $query, $params, $bypass_access, &$results, $start_limit, $start_offset, $iteration = 0) {
    $params['database'] = $this->configuration['database_settings']['database_name'];
    $params['collection_name'] = $this->configuration['database_settings']['collection'];

Well, I wanted a more fine-grained control over collections. Currently the collection name is tied to the backend server configuration and all indexes use the same collection name. In my case, I wanted to be able to create a separate collection for each index belonging to the vector db server.

askibinski → created an issue.

Came across this since I got penalized by lighthouse for not having a title or aria-label on the dialog:

I think you should be able to set a title but not show it. However looking at the code upstream that is currently not possible. If !showNoticeTitle then no title is shown (link). This is covered in this issue upstream: https://github.com/klaro-org/klaro-js/issues/521

I also added a reference to this issue at 🌱 [Meta] Improve the accessibility of the Klaro module Active .

Jquery is loaded on every page when this module is active because it is attached on every page using google_tag_page_attachments:

  $attachments['#attached']['library'][] = 'google_tag/gtag.ajax';

And that library in turn has a dependency on core/drupal.ajax which in turn has a dependency on jquery.

Related core issue for removing jquery dependecy in drupal core ajax:
📌 Remove jQuery Form dependency from misc/ajax.js Needs work

It pushes data for events to the datalayer on ajaxrequests. But if you don't care about that you could use https://www.drupal.org/project/gtm → or implement the tag through custom code.

I tested a vanilla setup with 10.3.6 and 2.0.0 and can't reproduce any issues with the dialogs.

That code actually exists to disable the LinkUI close method for the ckeditor popup when a drupal modal is open. When the modal is closed, it re-binds the original so you can close the first (ckeditor) link ui popup.

Having said that, my original code for that looks pretty brittle, with a hardcoded class:

    linkUI._hideUI = () => {
      const modal = document.querySelectorAll('.ui-dialog-content');
      if (modal.length === 0) {
        _hideUIOriginal.bind(linkUI).call();
      }
    };

And maybe in your case that class does not exist? In that case the re-bind wouldnt happen explaining your issue. I would welcome a patch/MR to improve this.

@remco gitlab didn't like changing the MR target branch to 2.0.x (gave me a unhelpfull "An error occurred while merging. Try again" message) so I committed the changes myself to the new branch. Will give you credit oc.

Created a new 2.0.0 version for D10/D11 and marked the new 1.0.4 version for D9 only (which is unsupported anyway).

Right. This got in here for 11 but apparantly also got backported to 10 (although that last comment states otherwise).

https://www.drupal.org/project/drupal/issues/3098201 🐛 \Drupal\editor\Ajax\EditorDialogSave::__construct() should typehint as an array instead of a string Fixed

Patch looks fine. I guesst the nice thing to do is to create a 2.x version for D10/D11 and drop D9 support and create a new 1.x release for D9 users. Will look into that this week.

The whole array_combine is a bit quirky, we do not really need to check config like that, simplified it a bit.

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

Sure, sounds like an improvement.

Note, that you can currently alter the labels using the provided hook.

Yeah we use `Drupal.ckeditor5.openDialog` and not the `Modal` or `iFrame` provided by entity browser.

There was a reason for using openDialog the way it is now, I think it was to simplify communication between ckeditor dialogs instead of the awkward way using iframes and also because entity browser threw in a lot of other (js?) stuff which screwed something else up, not sure.

Looking at the code from entity browser, they indeed also add `original_path` so I agree we should add that to the request, I'm just not sure that `hook_views_pre_render` is also needed to add it to `ajaxViews`? I don't see entity_browser doing that either so if someone would need it, it could be implemented using that hook since it is not needed for normal functionality.

Looks good! Thanks Remco!

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

Done!

Thanks! A nice improvement, merged!

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

Yes, I'm sure it's doable. Then again you could argue that linking to media entities is a better way in combination with a module like `media_entity_download`. That way, when a file is replaced in a media item with a newer version, the link keeps working.

But I'm open to review MR/patches if somebody wants to work on this. also referenced this issue from the project page.

👍

Indeed annoying, I think I only tested on a site which already had that overridden so didnt notice it.

Basically people can override this easily when needed:
https://github.com/justafish/drupal-example-custom-css

But I agree the default should be wider than that, and since Claro is the default too, that should work out of the box.

I added the class to the dialogSettings so it actually works, and renamed to css.

Thanks!

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

Thanks! Merged! This was indeed something that bothered me too, didn't now about the cardinality setting!

Reviewed and tested this. It keeps working with checkboxes even if you forgt to set that option, so that's nice. Updated the README and project page to include this information.

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

Fixed, good bot.

@bhaveshdas merged the MR for the PHP fix.

Note however that JS support in phpcs has been removed for a while now (for that eslinters are recommeded) and cause lots of false positives. Like the issues in the template literal in the patch at #6.

larowlan → credited askibinski → .

This module was moved into core in D10.1. See #3227824: Move the linkset functionality from the decoupled menus contributed module to core's system module → .

It's not that clearly stated on the project page though.

This seems an issue with layout builder which indeed can occur, and one way to address this is being worked on in 🐛 Layout Builder cannot be uninstalled while overrides exist; no easy way to revert all overrides Needs work .

Or you can indeed manually clear the tempstore entries like above.

I created a separate module which integrates entity_browser with ckeditor5:
https://www.drupal.org/project/ckeditor5_entity_browser →

It works on the link UI level, so you open a modal for a configured entity browser and insert the link in the link UI. Siince embedding entire entities is something else.

I created a separate module which integrates entity browser with ckeditor 5, without any linkit dependency:
https://www.drupal.org/project/ckeditor5_entity_browser →

askibinski → created an issue. See original summary → .

For starters, here is a cleanup of the hook_form_alter which is hard to read in the current state and does a lot of different things. The messages are split up in their own specific form_id or base_form_id alters and can be removed in 3.x since they only show a message.

The only thing remaining in the form alter is the redirect which checks on content entity interface which will prevent excuting unnecessary code on many other forms.

But this will make further changes depending on choice easier.

askibinski → created an issue.

I tested this on CKEditor 5 in two ways:

Please note that the tag needs to be allowed otherwise blocking isn't necesseary anyway (like the project page of this module states).

1: Copy an image (not the address) from any webbrowser and paste it into ckeditor 5.

This would create an but with the src being the http:// address of the image.
This is hotlinking an image, but not something this module is addressing.

2: Drag and drop an image from your local system into ckeditor 5.
This would create the base64 encoded image in ckeditor 4, but it seems to be blocked by default in ckeditor 5.

I think the default ckeditor build without additional plugins doesn't allow this out of the box.
There is a demo at https://ckeditor.com/docs/ckeditor5/latest/features/images/image-upload/... which does provide this functionality but it is differently configured.

So, it seems that in a standard configuration as described, this module is not necessary anymore. I'm not sure how to reproduce the steps above which state otherwise.

Not maintainer, but after testing the 3.x branch my biggest issue currently is 🐛 Empty anchor links are not editable Active .

This seems relevant: 📌 Optimize joins and table selection in SQL entity query implementation Needs work

Here is a re-rolls as in #20 but on top of #76 in 🐛 Render using theme input and select instead of lists with links for checkboxes and dropdown Needs review for D10 compatibility.
It does not take into account feedback from @Brolad above.

What about making it configurable under admin/config/development/logging?

Can anybody else confirm that this module is not needed anymore in ckeditor 5?

askibinski → created an issue.

askibinski → created an issue.

askibinski → created an issue.

I would say this is a duplicate of 🐛 Use Batch API to avoid timeouts when updating large number of references Needs work since both use batch api as a solution. Marking this one as duplicate since the other one has more recent activity, code is very similar.

Cleanup some similar issues and renaming them for sanity's sake, see 🐛 Use Batch API to avoid timeouts when updating large number of references Needs work for a similar issue but using batch api.

Created an MR against 2.x with the patch from #18 + minor changes. The difference with #20 is mainly a separate batch method in the manager and config for the batch size. The tests still use the non-batch method.

However, since this is only an api module, I would consider all this code to be in the wrong place. The term merge module is where the UI is and the batch_set should be implemented over there. This does require a bit of splitting up of `migrateReference` in the service.
Any functional javascripts for that should also be added in term merge.

Here is a reroll of above patch against 2.x. Also updating the other issues.

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

This shouldn't be too hard using web standards like the draggable HTML attribute and the HTML Drag and Drop API. Specifically the dragstart, drag and dragend events.

Duplicate
📌 Add access control to /filter/tips Needs work

askibinski → created an issue.

As commented here: #2862910: Add template suggestions to twig debug → the normal views suggestions do work. However, it's true that twig debug does not actually show these in the html comments.

That might be related to this core issue though.
📌 [PP-1] Use hook_theme_suggestions in views Postponed

Needs test I guess.

Needs test though. See entity usage for a similar test. And we already have entity_track_test.module.

Stumbled upon this issue when having the same problem. Can confirm setting the proper dependency fixes the issue, so this issue is probably works as designed.

See the 3.x branch of media_revisions_ui → for an example of this integration.

Here is simple patch, for testing this with tracking default revisions only.

Also, isn't it a bit weird that tracking of revisions is only done during a batch update?

@marcoscano
I think you might be right to do this in a hook. There are just too many variables on how people might have structured their content to do this reliably through 1 setting.

For example, we would have to figure out for each entity whether its a "middle-layer" non-canonical content entity (like a paragraph) and then find out what the active revision is, which for a paragraph means getting the entity reference revision field of the top level entity because there is no way to reliably get this info from the paragraph itself (as far as I know).

But that's only a paragraph example.

Good points, I will change the title since I actually meant the default (active) revision.

I discussed this with SeanB yesterday and regarding content moderation: getting the default is a bit tricker but doable. But indeed: I'm not completely sure if messing around with the revisions will trigger entity track. Something to check.

Regarding workspaces: I have 0 experience with workspaces and haven't seen a project with it in real life, so personally I would be completely fine with checking whether workspaces module is enabled and disabling this option for incompatibility.

The hook is a good alternative. The only thing bothering me about it is that it would require coding on the user part. And I think that the scenario where you are only interested in the active revision is the 80% usecase which "normal" users would like to have?

I'll give it a stab with an option to see if I encounter any dragons.

askibinski → created an issue.

askibinski → created an issue. See original summary → .