🇨🇦Canada @dylan donkersgoed

Attaching a version of the patch built for the current release as well.

This patch uses a {% spaceless %} tag which is no longer supported and can cause fatal errors. I've opened an MR with the patch applied and that fixed. Attached is a patch file. I've attached no interdiff as that's handled in the MR.

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

The clean-code-deprecated-dynamic-property-3170043-5.patch patch no longer applies to the latest dev/2.5-rc1 versions because the moduleHandler property name has been fixed. Attaching a new patch which includes only the setValue fix.

That last patch doesn't work with composer, uploading another in diff format.

I've fixed the conflicts in the MR. Also attaching a patch for the 3.0-rc2 version.

Uploading a patch rerolled for the 4.x version. I pushed the changes to the 2471481-integrate-typed-data-4.x branch as well. It's not letting me open it as an MR right now, but I may at some point in the future. In the meantime here's the patch file.

dylan donkersgoed → changed the visibility of the branch 2471481-integrate-typed-data to hidden.

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

I've added a patch for the actual Date filter plugin. I tried to transplant the code tobiberlin provided, but it resulted in fatal errors. The value is a string instead of an array unless it has multiple values, could be related to their custom plugin? The solution I've provided is pretty similar though.

This patch should probably have automated tests to go with it, so I'm marking it as Needs Work. But the patch seems to be working for me locally at least.

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

I merged in the latest 8.x-3.x branch. Rerolled patch for 8.x-3.0-beta4 attached as well.

I opened a new MR with a patch for the 2.x version. Patch is attached as well.

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

I've opened an MR and attached a patch.

Dylan Donkersgoed → created an issue.

@fruizalejos

We encountered further issues with this where sometimes access checks would check the default revision and the default revision wouldn't be referenced in any translation. I'm not sure how/why this happens, but it's possible you've run into the same issue. I worked around it by further expanding the check in a custom module to include the revision ID of the default revision when evaluating access. I didn't add it to the patch because I don't think we addressed the root cause and I'm not sure it's secure in all situations. It technically means someone might be granted edit access to a block revision that they otherwise wouldn't be able to edit.

I've opened an MR. Also attaching a patch file.

Dylan Donkersgoed → created an issue.

I've noticed an issue related to the one described in https://www.drupal.org/project/video_embed_field/issues/3311063#comment-... ✨ Add support for Ckeditor 5 Needs review where videos will render raw JSON. This is also related to the regex, but in this case seems to happen when there is an earlier { character before the preview_thumbnail property. In my case this was due to data-entity-embed-display-settings="{ in a <drupal-entity> tag but it probably could happen in other situations as well. Could be the cause of the issues a couple other users reported.

To fix this I pushed up a small change to the regex to make the opening

tag required. From what I can tell in the code for both the CKE4 and CKE5 plugins videos should always be wrapped in a paragraph so I believe this should be safe, but it would be nice to have some additional review/confirmation.

Updated patch is attached.

This patch worked for me, so moving to RTBC.

I needed to clear the storage for my site using the "Clear site data" button under the "Application" tab in Chrome (may differ depending on your browser) to get this to start working.

The latest version of the group patch this module depends on ( https://www.drupal.org/project/group/issues/2797793#comment-15236297 ✨ Entities identified by strings as group content Closed: won't fix ) uses a dedicated property, entity_id_str, for the ID instead. That results in an issue with this patch never picking up any menus.

I've pushed a change to this MR to accomodate that. It should still work for the old field as well. Patch file is attached.

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

Looking at the code it does appear to be intended that the autoplay setting from the WYSIWYG settings should be applied in the modal. It's trying to load default settings from there but they always turn out blank because it has the wrong plugin key ("video_embed", probably corresponding to the CKE4 plugin, instead of "video_embed_wysiwyg_video_embed"):

        $plugin_settings = NestedArray::getValue($editor_settings, [
          'plugins',
          // This should be video_embed_wysiwyg_video_embed for CKE5.
          'video_embed',
          'defaults',
          'children',
        ]);
      $settings = $plugin_settings ? $plugin_settings : [];

I've updated VideoEmbedDialog.php to check for both in the MR and attached the corresponding patch.

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

Patch attached.

Dylan Donkersgoed → created an issue.

I've merged in mkimmet's fix and it seems to be working for me. It discovers anchor links with the name attribute set and converts them to id anchor links, but never saves links with the deprecated attribute. Please let me know if that's not suitable for your use case/if it's not working for you. It's on the 3.0.x branch currently, but drupal.org hasn't generated the composer release yet.

This does not seem to require the patch from @DamienMcKenna, I think because it's not actually saving the attribute.

Seems sensible. Merged into the 3.0.x branch, thanks. I'll test it and tag it a bit later.

DamienMcKenna → credited Dylan Donkersgoed → .

Try running composer require 'drupal/anchor_link:^3.0@alpha' again? I've noticed that it does not always seem to pull in the library on the first go. I think it's because the composer.libraries.json doesn't exist until after the module is updated. If you don't have the library under [YOUR WEB ROOT]/libraries/ckeditor5-anchor-drupal the module will not work.

It appears I didn't actually have the module enabled on the site where I noticed this. So unable to reproduce the issue so far. If you could provide steps to reproduce with a clean Drupal site I may be able to help.

I tried installing on a clean D10 site and I can see the anchor link button when populating the toolbar in the text format.

However, earlier today I did see a similar issue on another site. I haven't been able to pin down the cause though and may not for a few days (it's on my work computer). I did have some JS errors in the console unrelated to anchor_link which may have been the cause, though other toolbar icons worked fine.

I don't think the issue is related to the external library after all. It shouldn't even be loaded on the admin page.

As for why you had to run composer twice I suspect the issue was that before anchor_link was updated the composer.libraries.json file didn't exist or didn't have information about fetching the library, so the composer merge plugin didn't import those dependencies at first. I encountered the same issue when installing from the instructions on the module home page. I'll update them.

I'm going to try to run through the steps as documented with a clean Drupal site when I get the chance (probably won't be until this evening). I wrote them from memory/samples in my project and may have missed something. If you're in a hurry you can probably just grab the library directly from https://registry.npmjs.org/@northernco/ckeditor5-anchor-drupal/-/ckedito... and decompress it in the relevant folder, it just won't auto-update until you set it up the composer way.

@mkimmet Can you verify whether the JS library exists in your libraries directory (e.g. docroot/libraries/ckeditor5-anchor-drupal)?

I've merged in a few fixes that were awaiting review including that one and tagged an alpha release. Thanks all for your testing and improvements.

Thanks for the PR mstrelan and vipul-tulse for reviewing. The latest dev version requires a tag with this PR merged in.

As for bringing this into Drupal I'm not sure bringing it into the module is the way to go. Despite the name of the repo, I don't think it's actually Drupal specific. It works inside the non-Drupal CKE5 sample environment at least.

When I first started work on this I was attempting to adapt the existing plugin to work the way I'd seen other Drupal CKE5 plugins work. I did not realize what I was actually doing was adapting it to be a DLL plugin, which is a recognized method of building a CKE5 plugin that works even outside of Drupal. So I called it ckeditor5-anchor-drupal.

I merged this in a while ago but didn't mark the issue done. Thanks for the PR ok-steve, the latest dev version requires a new tag with your PR merged in.

Thanks for the PR Brian-C. That appears to be working and I've merged it into the dev branch.

A GitHub PR was opened for this at https://github.com/northernco/ckeditor5-anchor-drupal/pull/3. However, I'm not sure it's working. See the PR for details.

I'm working on another D10 upgrade today for a site that uses this module so I plan to take a look at this today. I agree that this is ready for an alpha version, we already have it in use on some sites, but there's a PR for https://www.drupal.org/project/anchor_link/issues/3391107 🐛 Empty anchor links are not editable Active which I'd like to test and probably merge in first.

Rajab Natshah → credited Dylan Donkersgoed → .

That last patch does not work (at least against the 2.0.0 version) better one attached.

Changes are in the MR.

I'm also attaching a patch which includes both this change and the change from https://www.drupal.org/project/ckeditor_bootstrap_tabs/issues/3397894 🐛 Error when list elements are on the page RTBC .

Dylan Donkersgoed → created an issue.

Ran into the same issue. The patch above was in the right place but doesn't quite work, here's an updated one.

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

@matthiasm Are you using the latest version of the module and library? A PR from ok-steve that fixed this issue was merged in at the start of this month.

Some users have reported the node package not being updated on composer update in some situations. Please double check you have the 0.3.0 version of the JS package (or newer, I'll likely tag another version soon).

@vakulrai Are you certain this functionality came from anchor link? The last site I was working on that had it enabled does not have this on the production site still using CKE4. It also doesn't really look relevant for anchor links - they don't have URLs.

Same issue here, also on a multilingual site. I've opened a branch with the fix. It doesn't seem to want to let me open an MR right this second, but I'm attaching a patch with the fix.

This fix basically just switches the generated URL to be relative. I'm not sure what the reason for the original change was so not sure this is an acceptable solution or if an absolute URL is actually needed. But it does solve the issue.

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

Also, attaching a new patch file from the latest MR.

I pushed up a few more fixes for issues we ran into:

• When adding a new tab set it adds paragraphs by default. Without it it's quite tricky to select and edit text inside the tab after first adding tabs. Also, with just raw text CKE5 overwrites this with a paragraph when saving anyway (or at least something about Drupal's full html format does), so this keeps things more consistent.
• I fixed an issue where changing the number of tabs would sometimes add too many new tabs.
• I fixed an issue where the heading plugin (and maybe others, though that was the only one I could find with the issue) was not usable inside tabs. I don't fully understand why, but splitting the tab content and tab pane into multiple elements seems to have fixed it.

@RobLoach I also added a rudimentary composer.json. Please feel free to push to the MR if you need to,

Apparently I forgot to push so that was just the same patch as before. Here's the rebuilt patch for real.

Forgot to actually build the plugin after the last change, new patch.

Few more fixes. Notably there was an error when trying to edit source with my last few changes and there was a conflict with the core list plugin.

See issues for editing anchor links:

https://www.drupal.org/project/anchor_link/issues/3391107 🐛 Empty anchor links are not editable Active

and the bug where mailto links can be converted to anchor links:

https://www.drupal.org/project/anchor_link/issues/3391107 🐛 Empty anchor links are not editable Active

Dylan Donkersgoed → created an issue.

Dylan Donkersgoed → created an issue.

This library seems to be working for several people now. I've brought in the fixes from ok-steve and randy Tang. I'm going to merge this into the 3.0.x branch, double check that the install from 3.0.x is working, then close this issue and open a new issue for the problem with empty anchors not being editable. Thanks all for your testing and patches.

MR seems to be in a fully functional state now, attaching a patch.

There's probably room for some code cleanup, I'm not sure some of the separate upcast/downcast methods are still necessary, though they work fine.

I've expanded this patch to handle upcasting/downcasting bootstrap tabs. You can now load an existing page with tabs on it, edit the tabs, and save it and have everything work (at least from some cursory testing).

There's still a bit of work to do (e.g. the button isn't working correctly to add/expand tables at the moment and you can't change the active tab) but I hope to have that done within the next couple days.

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

Patch attached.

Dylan Donkersgoed → created an issue.

Patch attached.

Dylan Donkersgoed → created an issue.

Patch attached.

Dylan Donkersgoed → created an issue.

This might be a noob question. When I create a feed type using the LDAP Query fetcher and select my query. None of the LDAP attributes are listed as a source when I'm trying to map to my node fields. Only Feed entity items appear in the source list eg. Feed: Authored by

The fetcher doesn't automatically discover LDAP attributes, you need to manually enter the LDAP attribute name. In the same dropdown with "Feed: Authored by" there should be an option for a custom source. I don't recall off the top of my head exactly what it's called but I believe it's separated from the others toward the bottom of the dropdown.

As for why your feed is locking up it's hard to say without more details. I might be able to take a look if you can attach some configuration (the LDAP server, query, and feeds configuration or just an entire test database) that replicates the issue. You can use a public test LDAP server like the one from https://www.forumsys.com/2022/05/10/online-ldap-test-server/.

mcdruid → credited Dylan Donkersgoed → .

@everhee Nevermind, I see the issue and have pushed up a fix to the MR. New patch attached as well.

@everhee That should be working already. The new CKE5 plugin does store them in a different format, but it's supposed to load the child content of old tags into the value attribute of new ones. Is that not working for you and if so could you provide some sample markup with the issue?

The old footnotes widget supported entering the footnote text in a text area, but it was a single line input in the new one. This made editing long footnotes much harder. I've pushed up another change to switch the footnote text input back to a text area and enlarge it. Patch is attached.

@loopy1492 Please feel free to bring that into my branch. I was originally thinking we could keep it to retain CKE4 support but it probably does make more sense to just have a 3.0.x branch that supports only CKE5 and doesn't bring in the CKE4 dependencies.

As for the other issue with empty anchor links not being editable on click I fixed something similar for Footnotes, leaving it here in case someone else wants to apply a similar fix or for if/when I get around to it: https://git.drupalcode.org/project/footnotes/-/merge_requests/4/diffs?co...

The old version also allowed editing footnotes by clicking them, I've added that to the new version as well. Updated patch attached and it's in the MR.

I've pushed up an MR and attached a patch with a simple fix which uses the raw textContent for script and style tags. There's probably room for improvement (e.g. there might be other tags affected, or maybe a less naive check than just checking the parent tag name should be used and possibly there should be a test), but this fixes the issue for me.

I have also encountered this issue. It is not a CKEditor 5 base issue, or at least does not always happen with base CKEditor 5. A minimal CKEditor 5 configuration with source editing does not have the issue, but a minimal Drupal CKEditor 5 configuration does. I'm attaching minimal editor/format configs with the issue and an equivalent minimal vanilla CKEditor 5 setup.

To reproduce the issue in Drupal:

1. Import the attached editor.editor.source_editing.yml and filter.format.source_editing.yml config files to a Drupal site with CKE5 enabled
2. Visit a page with WYSIWYG editing and switch to the filter
3. Toggle the source editing mode
4. Enter content like: <style>span > a { }</style>
5. Toggle the source editing mode off and then on again
6. You will see that the editor has converted the > to >

To see the issue does not happen in vanilla CKEditor 5:

1. Extract the attached cke5_minimal_source_editing.tar.gz archive
2. Visit the index.html page contained within
3. Toggle the source editing mode
4. Enter content like: <style>span > a { }</style>
5. Toggle the source editing mode off and then on again
6. You will see that the editor retains the > instead of converting it to >

The CKE4 version of this plugin used to display footnotes as [fn] rather than showing them inline. I've updated the CKE5 plugin to behave in a similar way and pushed it to the MR. I've attached a patch as well.

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

This issue does occur in D10. You can see it if you install a fresh D10 site with the standard profile and try to add this filter to the "Full HTML" format you run into the error.

While there are similarities with the linked issue (similar steps to reproduce, the error occurs in the same function) I don't think it's quite the same problem. This is a validation error when saving the format, not a fatal error. It occurs in D10 even though the fix is already applied in D10. And while I'm not 100% sure I don't think it requires other filters of the same type to be enabled.

I'm also not sure it's actually unintended behaviour in core. I'm not too familiar with the internals of core's CKEditor handling but it does kind of make sense to me that a plugin that declares it restricts HTML but then provides no information about what tags it restricts would encounter validation errors.

Here's a patch.

Dylan Donkersgoed → created an issue.

No plans to work on this in the immediate future so unassigning myself for now.

Dylan Donkersgoed → created an issue.

I've pushed some preliminary code to an MR for this. It still needs some work. Right now this just splits the configuration related to the IdP metadata into a config entity. However, as roderick noted above there are some other attributes that almost certainly should be configurable. E.g. the unique ID, username and e-mail attributes. There are some other attributes arguably could be (e.g. maybe you want to require signed assertions for one IdP but not another?). And the submodules should be integrated with it as well.

This MR will also need an update hook to move the existing IdP config to the config entities.

Also, right now this MR doesn't expose any mechanism to actually select which IdP you're using through the UI besides setting a default one nor does it provide, e.g., separate login links for separate IdPs. So it's kind of useless unless you're planning to supplement it with overrides outside the module (which I am). So for some possible enhancements:

• The module could provide login links per-IdP
• The module could provide some mechanism for configuring when to use which IdP, e.g. selecting based on domain.

I'd appreciate anyone laying out their use case, so I can be sure I'm not misunderstanding it / it gets documented properly.

(Per #3211419-3: Consider merging with the saml_sp module / differences between modules, the saml_sp maintainer lays out his reasons, which is the only actual use case I've seen written down. My interpretation of this is: he's not _actually_ using multiple IdPs at once -- but having multiple configurations saved as config entities makes it a lot easier to switch / there are situations where some form of config override would be too hard.

I'm assuming most people are using some form of config override for making things work in dev/test/live environments.)

I can think of two other use cases besides per-environment configuration.

The one I'm dealing with right now is having different configuration for different site domains. I have a site with two different domains, one English and one French, so they have a different ACS URL etc. which means multiple SPs. The way the IdP owner's system works there's only one IdP per SP so that means multiple IdPs as well. I can kind of work around this with settings.php overrides/custom code, but it would be much more convenient if it was in the module.

Another case (and one that's harder to work around) would be allowing logins from multiple different IdPs. I haven't worked on any sites that have done this with SAML, but I have personally worked on sites that have done it with OpenID and seen sites that do it with SAML in the wild.

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

Forgot to mention when I created the issue but I think the person who added these changes to the original patch was Austin Mitchell → , please credit him as well if this is merged in.

I've attached a couple MRs. One applies the part from the original patch that adds support for this, one is for the 2.x version of group and would need to be used in combination with https://www.drupal.org/project/gcontent_moderation/issues/3309542 💬 Is group content moderation module compatible with Groups 2.0.0-beta3? Needs work .

Dylan Donkersgoed → created an issue.

If you're using wikimedia's composer merge plugin you shouldn't need to manually change the JSON (should probably add something about that to the README) but if you have that set in your composer.json that's correct.