🇺🇸United States @justcaldwell

Updated static patch attached.

Updated the MR with a second post-update hook to run after invalid permissions are removed. So the solution now:

1. Implements logic to associate appropriate menu config dependencies with Menu Admin per Menu permissions.
2. Removes any stale/invalid Menu Admin per Menu permissions (i.e. for menus that have been deleted).
3. Re-calculates dependencies and updates roles that have existing Menu Admin per Menu permissions (in light of #1).

Back to NR.

Hmm. Looks like the update hook should also cause an update of any role with any menu_admin_per_menu permission, so the dependencies for existing permissions are applied. Back to NW. :\

Posting a static patch. Also bumping to Major as this can render the Permissions page unusable unless/until you do something like the suggested workaround in #5.

Post update hook added. Ready for review/testing.

Great, thanks @mahyarsbt!

I see the MR was approved, but it seems like it's not actually merged to 1.0.x?

justcaldwell → changed the visibility of the branch 3457584-remove-keyvaluestore to hidden.

Until someone provides an MR for that feature, I recommend the approach in MR 8 for folks who need a resolution to the issue in the meantime. To that end, I'm attaching a static patch based on the MR.

Updated IS

The update hook from a patch for the same issue in entityqueue → looks almost cut-and-paste-able.

MR 20 adds the required dependencies. Ideally, we'd also provide an update hook to clean up any stale invalid permissions hanging around (hence the draft MR). I'll try to take a pass that hook soonish, if no one beats me to it ;).

Just bumped into this again. Updated Steps to Reproduce to more reliably invoke the error.

MR 8 is an alternative solution that makes fewer changes to the module, retains the KeyValueStore approach and the role specific form/route handling. It just discards any stored settings after each use — settings are re-loaded/updated as usual on submitting the filter. I kept the store as expirable in case something unpredictable occurs after the form is submitted, but reduced the time to 5 minutes.

Either approach ensures the permissions page is immediately usable if the user returns to it after attempting to load too many values (or within 5 minutes if something else went wrong).

Hi @nsciacca. Thanks for starting the MR! Yeah, I ran into the same issue this morning. I added the fix to the MR, as well as some additional cleanup of KeyValueStore-related code.

Also, PermissionsRoleSpecificForm.php depends on saveFilterSettings(), so that needed to be addressed. Since a permission filter has been added to Drupal's core form, I don't think it's necessary for filter_perms to override the role-specific form any longer — so I removed it and the route subscriber.

We'll have to see how the module owner feels about all that 🙂. Setting to NR.

@cyu: Some initial testing of this approach worked for me. If we $form_state->setRebuild() in the submit handler, the submitted values will be still be available on subsequent loads, so $filter can just draw from those values and drop the KeyValueStore entirely. The submit handler would become:

public function submitFormFilter(array &$form, FormStateInterface $form_state) {
    $form_state->setRebuild();
}

Then $filter becomes:

$filter = [
    'roles' => $form_state->getValue('roles', []),
    'modules' => $form_state->getValue('modules', []),
];

See: FormState::$rebuild and FormState::setRebuild

The form 1) would no longer "remember" the previous filter if you navigate away from the permissions page AND 2) the render will still fail if too many permissions/modules are selected, BUT the page will be immediately usable if you navigate away and come back to it.

The solution for #2 is user training, unless we add some generic warning text about selecting too many permissions/modules.

Hi @nsciacca - I think remembering the filter settings between submissions is essential to the way the module functions.

@cyu - regarding your question in #5:

Out of curiosity, what kind of numbers for roles/permissions and memory limits are causing this problem?

We have 865 permissions and 54 roles (yikes!).

I just tried 'All modules / All permissions' again on my local with a memory limit = 1024M and it looks like the form actually begins to render for me (after quite a long time). It's the browser (Chrome in this case) that chokes on the results and ends up giving the 'Oh snap, something when wrong' message after partially rendering the page.

On our actual web servers, the memory limit on admin routes is 512M, and the page exhausts the memory and fails quite a bit sooner with no attempt to render.

Noting that the latest release of CKEditor 5 (44.0.0) now includes the Bookmark plugin — CKEditor's native UI for creating anchor links.

Additionally, work on link UI enhancements to ease linking to "on page" anchors/bookmarks is being tracked in this github issue. See the second video on that page for a demo.

I think that modules should ensure compatibility with popular admin themes, so it's probably more likely that this becomes part of (or sub-module of) Moderation Sidebar. That's probably no going to happen, so we'll just settle for the status quo 🙂.

Added 'Administration Tools' -- seemed more accurate than 'Developer Tools', which I removed.

LGTM and mo issues found in manual testing against D10/11.

Merged to 1.x-dev. Thanks for contributing.

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

justcaldwell → created an issue.

justcaldwell → created an issue.

Merged to 2.x, release to follow.

Re #3: Thanks, but the patch only addresses comment #2, not the full issue as described in the summary.

Released in 2.0.0-alpha9.

Released in 2.0.0-alpha9

The signature for the __construct() method of ConfigFromBase has changed in Drupal 11 such that we'll need to drop support for Drupal 9. See https://www.drupal.org/node/3404140 → .

justcaldwell → created an issue.

This issue and MR is 'sub-optimal' at best! Closing.

Merged to dev. New release to follow soon.

Just looking at the examples from the core ckeditor5 module, your example seems close, but maybe it should be something more like:

my_plugin:
  ckeditor5:
    plugins: [ htmlSupport.GeneralHtmlSupport ]
    config:
      htmlSupport:
        allow:
          - name: ~
            classes: [my, whitelisted, classes]
  drupal:
    label: Whitelisted elements
    library: core/ckeditor5.htmlSupport
    elements:
      - <* class="my whitelisted classes">

I'm working on 📌 Add support for new field selection UI Active , and it makes sense to go ahead and fix this in those changes. Closing and adding credit there. Thanks!

Makes sense to fix ✨ Allow underscores in CSS classnames/style key field. Active while we're making these changes.

Released in 1.0.4

Fixed in dev. New release soon.

justcaldwell → created an issue.

Thanks for posting this @gravelpot! Confirming same.

The offending commit was reverted in https://github.com/robrichards/xmlseclibs/releases/tag/3.1.3 -- updating to that release resolves the problem for us.

This is still an issue in 4.x.

The fix proposed in the MR still works in 4.x. The MR will need a re-roll for the update hook.

Thanks for the update. Setting to NW.

Thanks for raising this @alphex! This is now fixed in the 2.0.x-dev branch, or you could apply the patch from the MR.

I hope to tag a new release sometime in the next week or so.

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

Thanks for raising this!

Some steps to reproduce would be helpful here. Also, do we need an update/post update hook to clean up possible improperly stored values?

Previous patch looks like pretty obvious credit mining and @Pracheth never returned to correct the format. No credit.

Makes sense. Merged with a new release coming shortly.

Thanks for contributing!

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

No problems with manual testing. Merged the minimal change and will tag a new release soon.

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

Having a hard time replicating the scenario, but I can see how it happens. I suppose we could query for distinct values and populate the bundle filter with the results. Couldn't this be an issue for other fields as well?

This is a good idea. Are you thinking layout id should be on the current report somewhere, or would this be a new report?

justcaldwell → created an issue.

Did some manual testing on D11. All seems fine. Merged to dev, release to follow.

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

Thanks for the contribution! In general, I like the changes proposed here. I think there are some minor issues that need discussion/work:

1. In the filters, what was labeled 'Block Type' is now 'Inline Block Type' and 'Block Type' now refers to other or reusable blocks, is that correct? This was confusing to me. Maybe 'Other Block Type' or 'Reusable Block Type' would be a better label here? I'm not sure.
2. If I'm correct above, then those two filters are mutually exclusive, right? That can lead to further confusion. Maybe throw an error on validation, or add some help text or additional organization to the filter area?
3. The 'Reset' button doesn't clear Inline Block Type if a value has been set.

Static patch of the current MR.

I opened PR 14 on northernco/ckeditor5-anchor-drupal in support of the MR here.

Just wanted to note that, when last I checked, ckeditor's upcoming 'Bookmarks' plugin will not support editing/creating bookmarks (anchors) on links.

I also don't see a way to resolve the edge case in #25. I think the changes so far are an improvement on stripping ids from links, so setting back to NR.

As I indicate in the MR, this still requires minimal changes in northernco/ckeditor5-anchor-drupal plugin. I can open a PR there if there's any interest in this.

At this point, we're just using a custom build of the plugin that resolves this and a few other issues in the queue to try to ensure a smooth transition from cke4 -> cke5.

MR created as promised 🙂

I haven't actually tried this, but I think a "quick-and-dirty" approach would be to just manually replace the northernco version of the plugin in your /libraries folder:

- Visit https://asset-packagist.org/package/npm-asset/justcaldwell--ckeditor5-an..., and click 'Get Zip'
- Decompress the zip file, and rename the resulting 'package' directory to 'ckeditor5-anchor-drupal'
- Replace /libraries/ckeditor5-anchor-drupal with the directory above

I assume this would get overwritten next time you update anchor_link with composer, so you'd probably want to re-install anchor_link using the modified "Option 2" outlined in #21 and #26 above when possible.

Adding CKEditor Anchor Link → , where it's currently a dependency of the 8.x-2.x branch, and might be added back to the 3.x branch (see: 🐛 Support concurrent use of CKEditor 4 and 5 Active ).

Given no other input, I'm going to close this one. Thanks!

Here to eat my words 😬.

Given that there's still a large number of sites that haven't moved from CKEditor 4 (1.x and 2.x branch usage is still at ~18K), there may be a case for supporting an improved upgrade path that allows testing both CKE4 and CKE5 concurrently (see 🐛 Support concurrent use of CKEditor 4 and 5 Active ).

This would require allowing the name to continue to appear in the final source code. As I wrote in that issue:

The plugin currently upcasts anchors with name attributes, but does not include the name attribute in the resulting page source. As a result, anchor links that have been edited in CKEditor 5 will no longer be present if the text format is subsequently switched back to one using CKEditor 4.

Even though name is deprecated, it might be preferable to continue full support for now, and remove it in a subsequent release or branch.

I'm attaching a patch with the necessary changes. I'll open an issue branch/MR soon.

I opted to not reintroduce the hard dependency on the CKEditor FakeObjects → module now, but to use these changes you would need to include it in your project:

composer require 'drupal/fakeobjects:^1.2'

As I said above, this is probably a good idea anyway if you're updating from 8.x-2.x, as composer may well remove the module (if it's not required elsewhere) without uninstalling it. FakeObjects can be uninstalled and removed when it's no longer needed.

The patch also corrects broken paths to the CKEditor 4 toolbar icons.

justcaldwell → created an issue.

Seems reasonable to add support for other entities, but the current MR adds quite a bit of duplicate code. We should refactor to avoid that.

I'd call it minimally maintained. I somehow missed a couple of the recent issues entirely.

I'll plan to make a Drupal 11 release in the next few days, and probably include the RTBC issue.

@rob230 - this fix is actually included in the current release (3.0.0-beta1). The maintainers just haven't updated the issue status to Fixed.

Drupal.t() will pass strings through \Drupal\Component\Utility\Html::escape(). Not sure if there's an easy way to allow HTML in javascript like this.

justcaldwell → created an issue.

justcaldwell → created an issue.

Agreed. No concerns from me.

justcaldwell → changed the visibility of the branch 3477418-add-project-browser to hidden.

justcaldwell → created an issue.

Manually tested with Drupal 11 — works with all toolbar options.

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

If I remember correctly, the CKEditor 4 version of anchor link adds both an id and name attribute to all anchors. Maybe you could select <a> elements that have both attributes? For example:

a[id][name] {
  /* your styles here */
}

Depending on other styles in play, you might have to use !important to get them to work.

This assumes your text format/filter is allowing both attributes through to the rendered content

Yes, switching to 'Create label before first save' worked for us 🐛 Pattern not being respected Active .

No time to debug right now, but my assumption is this stems from changes in 🐛 Set Label runs two times on node creation Needs work .

We're seeing the same issue. Our pattern uses a single token that points to a text field on the same entity. That field allows some minimal formatting, but the generated labels are as described above (e.g. %AutoEntityLabel: 7a075eef-dde7-4c04-8f99-8f6a3f6e6177%) whether or not the field values contains formatting.

Re-saving the node corrects the generated label. Switching to the new 'Create label before first save' setting also works in our case.

Since updating to 3.3, we're experiencing the issue described in 🐛 Pattern not being respected Active . I haven't had time to confirm, but is suspect it might also be related to the changes made here (?).

A simple fix would be to remove the `id` attributes from the elements in the `icons/anchor.svg` file if they are not required for CSS or JavaScript.

They aren't needed for CSS or js. Absolutely a simple fix, but will have to happen in the plugin at northernco/ckeditor5-anchor-drupal. I opened a PR there:

3426731 remove duplicate ids by justcaldwell · Pull Request #13

justcaldwell → created an issue.