🇺🇸United States @agarzola

Added info re: overriding component libraries.

Include fragment in theme region inheritance link.

agarzola → created an issue.

agarzola → created an issue.

Assuming you’ve removed the version key from the library definitions in your modules/themes’ *.libraries.yml files, then I suggest looking into some of the issues linked in the comments from earlier this year.

I’m interested in this feature. Is this issue in active development, or is it up for grabs?

Did the core really need to manage this case ?

Since loading content dynamically via Ajax is functionality offered by core and it is a reasonable expectation for dynamically-loaded content to itself contain dynamic links within, then it stands to reason that core should handle that use case.

Using version is certainly an option. I didn’t mean to dissuade you from doing that if it works for your team and the specifics of your project and workflow. In our case, we found that it was A) too easy to miss during development, and B) too difficult to catch during review. This was especially problematic for us because our PR preview environments did not exhibit the kind of behavior you get from production when a library is updated but the version key is not (since the PR preview is essentially a fresh build with no old assets in the cache). So after a handful of deployments to production that served stale assets (requiring quick follow-ups to bump the relevant version numbers), we decided on going with the safer option and just remove version altogether.

The alternative is to update the version key on any libraries that you know for sure changed in a given release. That is likely too onerous a requirement in your case, so the best option is to remove the key entirely. When Drupal encounters a library without a version key, it will hash the contents of each file to use as the file’s “version”. It is my understanding that this will result in only the files that have actually changed getting cache busted.

@yonailo Can you share details on why removing version is impractical, unfeasible, or otherwise does not fix the problem in your scenario?

Providing a new patch for 10.1.x that uses MutationObserver, since MutationEvent is deprecated.

That is a valid concern and one that I agree with, which is why I updated both the MR for cl_server and the PR for storybook-drupal-addon to deprecate (as opposed to replacing) _storyFileName in favor of _componentFileName. I think the mix up is due to the fact that I had not updated the PR title and description in GitHub, both of which still referenced replacing _storyFileName. I have updated the title and description to more accurately represent the changes proposed here.

Let me know if deprecating the old parameter is not enough to ease the burden of maintenance you referenced in the PR.

I pushed a commit that deprecates _storyFileName instead of outright removing it. I also updated the storybook-drupal-addon PR to include both parameters. This issue is again ready for review.

That is a good feature request on its own, but I think being able to use cl_server in production would still be helpful for anyone who wants to be able to render components with arbitrary data (e.g. à la Storybook controls) in production.

agarzola → created an issue.

Would it be better to deprecate _storyFileName instead of outright replacing it?

Merge request is ready for review, as is PR #41 of storybook-drupal-addon. These should be released together and it may be prudent to bump the Storybook addon’s major version, since the PR introduces a breaking change.

agarzola → created an issue.

There’s an argument to be made that CR 3301716 → should be updated to include information about how to opt out of the new behavior (namely: by removing version keys from any library definitions that should be re-aggregated on cache clear/site deploy).

For anyone looking for a definitive answer to this as I was: I received confirmation in Drupal Slack that removing the version key from libraries, regardless of where they are defined (core/contrib/custom). I agree with @ras-ben in #95 that this should be documented somewhere and even warrants a change record.

Just to confirm: Is the way to make sure that Drupal always regenerates aggregated assets on cache clear without having to bump the version number on each library that has changed to remove version altogether from our library declarations in *.libraries.yml files?

Many thanks, Spokje! I was not sure whether the 10.0.x dates applied to 10.1.x.

Hi! We just ran into this issue and our client is asking about a fix. Before we go and apply this patch ourselves, is there a way to find out when a 10.1.2 release might be published containing this fix?

Thanks!

When a library doesn't specify a version, file_get_contents() the files, hash the raw contents and use that in place of version. We'd need to do that in AssetGroupSetHashTrait so it would run both when building the main page, and also when files are generated.

This approach is most attractive to me as a front-end developer. Perhaps instead of no version specified (opt-in by implication), this could be explicitly opt-in via keyword (à la VERSION). Maybe HASH, FILEHASH, or similar?

Our team ran into this issue earlier this week. Drupal was serving stale aggregated assets on a large multisite project serving ~20 sites, each with its own theme. After finding 🐛 Ensure that edge caches are busted on deployments for css/js aggregates Fixed , we decided to update the THEME.libraries.yml file in each site’s theme to either add or change the value of version for the libraries that have been updated since we updated the project to Drupal 10.1. This caused Drupal to serve updated aggregated assets and all sites now receive the expected styles in production.

Thanks for your reply, @apaderno. If I understand correctly, it sounds like @energee —the only maintainer of this module at the moment— to decide whether to add me as a co-maintainer. I cannot assign this ticket to him, so what are next steps here?

I can confirm that the error described here appears to be solved by the latest patch in the issue linked by @DamienMcKenna.

Thanks, @Kristen Pol, @apaderno, and @florianmuellerCH.

Since this project is covered by the security advisory policy, drupal.org site moderators will not add as co-maintainers users who cannot opt projects into security advisory policy.

We are trying to pair up people so they have a mentor with security rights.

It sounds like policy would dictate that I cannot be added as a co-maintainer, the reasons for which I think are entirely understandable. But Kristen’s comment sounds like an exception might be made for folks seeking mentorship? If that’s not the case, then should I close this issue?

Kristen Pol → credited agarzola → .

Kristen Pol → credited agarzola → .

This is still active, as I'm working getting my test to pass.

I did some digging in an attempt to find alternative ways to limit the selector’s scope, but I found that what is proposed in the MR is not only adequate, but the correct solution.

My original concern (and I believe it’s @wimleers’s concern as well) was that perhaps other text which may potentially include one or more legitimate anchor links might get included inside of .form-textarea-wrapper (as a sibling of the textarea element and WYSIWYG editor). I can only think of one such element that might be included and that is helper text, so I did a test locally and it turns out that helper text is not injected into the .form-textarea-wrapper container element, so it is not at risk of getting left out of form.js’s purview with the query selector in my merge request. I have attached a new video showing this in action. The video shows my fix preventing the anchor link within the CKEditor UI from working (as intended). The field has helper text that includes an anchor link. Clicking on the anchor link correctly navigates to the anchor, as intended.

Respectfully, I am marking this back as and marking the conversation as resolved, as I believe that the solution in the MR is adequate. Please let me know if this is not proper procedure to get more eyes on this.

Thanks!

Oh wow — so it's a regression on the Drupal core side simply because CKEditor 4 used

s, and CKEditor 5 doesn't! 🤯

What a magnificently obscure bug, and epic work shepherding this in the right direction, @agarzola! 👏

Thank you for the review @sonam.chaturvedi.

Reached out to @zaporylie about this issue today.

agarzola → created an issue.

Contacted @energee about this issue today.

agarzola → created an issue.

Kind friends showed me the way. I’m closing my MR, changing the target version on this issue, and submitting a new MR that targets 10.1.x.

I could use some guidance on the proper way to submit MRs against multiple branches. Should I create a child issue for the 10.1.x-dev branch and create an MR from there?

I found the culprit. It actually does not have to do with the CKEditor module at all, but rather form.js. This file (found at core/misc/form.js) adds a click event listener to all anchor links on the page which will trigger a fragment interaction event, causing the browser to navigate to the anchor target. It targets all anchor links indiscriminately, without regard to where it might be located:

  /**
   * Binds a listener to handle clicks on fragment links and absolute URL links
   * containing a fragment, this is needed next to the hash change listener
   * because clicking such links doesn't trigger a hash change when the fragment
   * is already in the URL.
   */
  $(document).on(
    'click.form-fragment',
    'a[href*="#"]',
    debouncedHandleFragmentLinkClickOrHashChange,
  );

I believe the solution here is to amend the selector passed in the second argument to $().on() so that links found inside a WYSIWYG editor are excluded from the query. I will be submitting merge requests for Drupal 9.5, 10.1, and 11 shortly.

Since this doesn’t actually have anything to do with the CKEditor core module, I changed the Component field on this issue to “javascript”, though perhaps there is a more appropriate selection?

Removing the [upstream] badge until we can confirm that this is reproducible upstream.

I have not been able to reproduce the issue in any of their demos, actually. You can try it for yourself: https://ckeditor.com/ckeditor-5/demo/feature-rich/

That is the reason I have not reported it upstream and instead reported it here. Happy to open a ticket, though, if we’re able to reproduce it in a non-Drupal setting.

agarzola → created an issue.

Something caused the previous 9.x patch not to pass tests in 9.5. Tooling in 9.5 causes the non-ES6 version of the file to _not_ get blank lines around the conditional.

This new patch should apply cleanly to 9.5.x.

Re-rolled against the 10.1.x branch.