🇳🇱Netherlands @seanB

This seems to do it. Updated the MR.

Would it be possible to move cloneParagraphs, cloneInlineBlocks to a separate service so it can be used in other code/modules? It would also be nice to move the code to clone a specific section to a separate cloneSection method. I'm currently adding a custom controller that duplicates the code to clone a specific section in a layout, and it would be nice to reuse the code we have here.

Tests are all green with the latest changes in the MR. Impact on the amount of queries is the same. From about a 1000 queries to 250 queries for a page with a bunch of layout builder components. 🎉

This caused quite some issues. It probably makes more sense to add the caching to LayoutTempstoreRepository. This solves the issue I experienced with a lot less impact. Updated the MR and IS.

Added a MR. Not sure what the best approach would be to add test coverage. This shouldn't really change anything, except maybe the number of queries to the database. Also would love to get feedback from the framework managers if this is something they would consider adding.

Added a new 3.x release with the changes from the PR https://git.drupalcode.org/project/groupmenu/-/merge_requests/10
Thanks everyone!

We can't simply update the static cache when it is updated, so lets unset it. Sorry about that.

Attached is a patch that adds static caching. This reduced the number of queries in a layout builder edit page from 912 to 227.

Before:

After:

seanB → created an issue.

This seems to make sense! Thanks for the issue. I will check the PR this week and try to get this in.

seanB → created an issue.

Created an MR to fix it. We might want to consider moving the gin_element_info_alter to a different file now it no longer only affects form elements. Did not do that for now though.

seanB → created an issue.

Can we figure out a way to not hardcode the form mode to 'edit'? I'm running into the same issue mentioned in ✨ Use a different form mode when editing block content in an inline block Needs work .

Patch is attached

seanB → created an issue.

seanB → created an issue.

Patch is attached.

seanB → created an issue.

Committed. Thanks all!

This should be fixed by 🐛 Configuration schema for "Enable extra settings" (title,pagination,...) incorrect Fixed

Thanks all! Committed the latest patch.

Thanks for the MR! Also, to weigh in on the discussion about alter hooks vs other ways of changing the parameters: we want the way to alter this to be as lightweight as possible. Cached pages should be served with minimal overhead.

When getting the defined parameters, we can't even rely on everything in the container yet. The module handler works, but I still had to force it to load all .module files. Using the event system could become problematic, same goes for plugins. Dependency injection could easily lead to a circular reference (found that out the hard way while trying to do fancy stuff in my implementation of the hook).

Overall, hooks still have their place in the Drupal universe and I don't see that changing anytime soon.

Patch is attached to implement a hook that allows the parameters to be altered.

Nice work! This was a lot easier to review than the previous PR. So thank you for that.
Added my review to the PR. Hopefully we can find more people to give this a try and thoroughly test it.

After this is fixed I will add this to a new major release. I suggest we create a 3.x branch to follow the versioning of the groups module.

Reroll for latest version.

Attached is an updated patch for Drupal 10.2.

Attached is a patch that seems to solve the issue.

This doesn't seem to fix the following use case:

• Add the Dutch language (default, no prefix) and English language (with a /en prefix).
• Edit a Dutch node, linking to a different Dutch page using the English admin language.
• When a module calls PathValidator::getUrlIfValidWithoutAccessCheck() for the link, you run into the same issue.

Found this while looking at 🐛 processUrl Cannot Find the Correct Entity in Multilingual Site Active .
We could use the language with an empty language prefix as the default langcode. I think that should fix the issue.

This is indeed an issue. A little more context. The following calls lead to the issue:
HtmlLink::parseEntitiesFromText()
EntityUsageBase::findEntityByUrlString()
EntityUsageBase::processUrl()
PathValidator::getUrlIfValidWithoutAccessCheck()
PathValidator::getUrl()
PathValidator::getPathAttributes()
PathProcessorManager::processInbound()
AliasPathProcessor::processInbound()
AliasManager::getPathByAlias()

The AliasPathProcessor::processInbound() doesn't pass a langcode to AliasManager::getPathByAlias(), and for that reason AliasManager::getPathByAlias() uses the current language from the URL. If you link to a page in another language this is an issue, but also if the langcode in the URL is different for other reasons (like working on translations or something).

The current patch has some issues though.

1. +++ b/src/EntityUsageBase.php
   @@ -9,11 +9,15 @@ use Drupal\Core\Entity\EntityRepositoryInterface;
   +use Drupal\path_alias\AliasManagerInterface;
   

   We don't have a hard dependency on the path_alias module, and we should not add it just for this bug.

2. +++ b/src/EntityUsageBase.php
   @@ -363,4 +402,29 @@ abstract class EntityUsageBase extends EntityTrackBase {
   +  protected function processUrlForAllLanguages(string &$url): void {
   +    $languages = $this->languageManager->getLanguages();
   +    $request = Request::create($url);
   +    $resolved_path = $this->pathProcessorManager->processInbound($url, $request);
   +    foreach ($languages as $language) {
   +      $alias = $this->aliasManager->getPathByAlias(
   +        $resolved_path,
   +        $language->getId());
   

   This works around the issue, but duplicates a bunch of code. The real issue is the LanguageNegotiationUrl inbound path processor strips the language prefix from the URL, and the AliasPathProcessor is not able to pass the langcode (when available). It seems there already is a core issue to fix this: #3314941: Language-prefixed alias paths cannot be correctly routed inside a request →

Could you try if #3314941: Language-prefixed alias paths cannot be correctly routed inside a request → fixes the issue?

I think doing something like ✨ Alter hook to adjust image effect Needs review makes a bit more sense. It would reduce the complexity of this module and add more flexibility.
Do you think a hook would also give you what you need?

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

Merged! Thanks.

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

I see no harm in doing this, I'll check it out soon. If anyone else can test and RTBC it before I get to it, this would also help!

Busted 🙈! Thanks for this. Merged.

Could someone create an MR for this? This seems like a reasonable change.

This seems to be a duplicate of 📌 Fix the issues reported by phpcs Needs work , which also addresses some other CS issues.

At a first glance I think the subgroup support being in the same MR as the support for groups v2/v3 makes it a bit harder to review.
Not sure now what is absolutely necessary for the upgrade, and changes made to support the new features.

Most of the times it is probably best to keep MRs as small as possible and create separate issues for new features. As for the next steps, I would recommend we try to get these in first:
📌 Fix the issues reported by phpcs Needs work
📌 Drupal 10 compatibility issues Needs review

After that this PR should already be a bit smaller. If we can then create separate issues for the news features (like subgroup support), and split the MR up, I think we should be able to get everything merged more easily.

Thanks for this. There is 1 thing that I noticed, the entity type manager is now injected when we explicitly didn't do that before to prevent a circular dependency.

kristiaanvandeneynde → credited seanB → .

Has this been fixed by #3032078: Multiple webheads can cause infinite growth of Twig cache → ?

Ah yes, the string|int|null user ID is definitely troublesome. We either cast to int for both values, or change the strict comparison. I don't have a string opinion on it either way. I feel casting to int is slightly nicer, but not sure if that could lead to other unforeseen issues.

So either:
$is_owner = $account->id() && (int) $account->id() === (int) $entity->getOwnerId();

Or simply just do:
$is_owner = $account->id() && $account->id() == $entity->getOwnerId();

The node modules contains this by the way, so maybe to non-strict comparison would be more in line with that:

$uid = $node->getOwnerId();
if (... && $account->isAuthenticated() && $account->id() == $uid)

Tomorrow is my day off, but I'll run the latest version through our tests to see if we can remove the custom cache resets. Thanks for working on this. The content overview was kind of slow taking over 10 sec for some users. With the patch it went down to like 4/5 sec, so pretty significant improvement.

Found a small issue with my latest patch, this should fix it!

Rerolled the patch for 2.x.

Unfortunately, logged-in users were still running into pages where they didn't have the admin toolbar. It would be nice if we could skip Fast404 for authenticated users entirely.

Attached is a patch that would fix this.

seanB → created an issue.

The attached patch seems to solve (at least part) of the issue for us. It's probably not a long-term solution, so just uploading this in case it helps anyone.

Rerolled #21 for the latest dev version.

Patch is attached.

seanB → created an issue.

This should fix it.

seanB → created an issue.

Merged! Thanks.

I would generally advise adding width: 100% to responsive images in CSS. I think that should solve the issue. It seems that in this case the parent of the image does not have a width when the image is empty, while the image is looking at the width of the parent to determine the correct source.

Regarding the "flash", we unset the src for lazy loaded images to prevent them from being loaded by the browser. It prevents duplicate images being loaded in some cases. However, if an image is in the viewport, removing the source can cause this flash. I think we might be able to figure something out to solve this.

Thanks for the quick feedback. Great to hear that the latest release makes a significant performance improvement!
Another tip would be to lower the smallest image size to something like 50px. That should give similar results to something like LQP.

Since the latest release has similar (or maybe even better) performance than the LQP version, I'm inclined to close this issue as won't fix. I do appreciate the work you put into this and helping look for performance improvements.

What is the size (width) of the smallest image src loaded for each image used in your example?

In 🐛 Improve lazy/eager loading Fixed we also added a bunch of improvements to reduce the initial number of loaded bytes. Did you enable lazy loading for images and use the latest release for your test?

Thanks! Nice find! Committed. I'll create a new release now as well.

1. +++ b/js/resizer.js
   @@ -172,7 +160,10 @@
   +            intersectionObserver.unobserve(entry.target);
   

   This makes sense, it optimizes the executed JavaScript. Nice!

2. +++ b/js/resizer.js
   @@ -172,7 +160,10 @@
   +            resizeObserver.observe(entry.target.parentNode);
   

   This is also pretty clever, it allows us to remove the check in updateImage and also optimizes the JavaScript. thanks!

3. +++ b/js/resizer.js
   @@ -187,14 +178,16 @@
   +          intersectionObserver.observe(image);
   

   This seems to be missing the part where we remove the "src" for lazy loaded images. I added that back, since the browser seems to load the original sources before we update the src to the prefered image style. Since the original sources are pretty much immediately replaced, this seems like a nice extra optimization. It needs some extra manual testing though.

I've committed the latest patch with some minor changes. Please help me test the latest commit (dev-1.3.x#94469349953435ec2a948dcffd4507fd0e12f267). If you can confirm everything works I will roll a new release.

Coul you create an interdiff for the patch?

This should already be fixed in the latest dev version. Closing as duplicate of 🐛 data-loaded attribute is sometimes not set and data-multiplier attribute assumes it exist. Fixed

The patch should apply to the latest dev version (you can load that via composer using dev-1.3.x#ced7e669a0e672d5ceb83dc487c13b00952fd7fc).

We definitely need an intersection observer, since images are immediately loaded when you change the src. That is pretty much the bug we are solving here.

What we implemented now is a workaround. I think there is still a bug/issue somewhere. The regular views query, in combination with the group's module query alters, simply causes timeouts. This seems to be caused by the amount of added joins and this wasn't the cause in groups v1.

Not sure yet how to fix it or how to further optimize the query.

First of all, thanks for bringing this to my attention. We are indeed loading all lazy loaded images directly. Attached is a new version of the patch addressing some issues I had while testing it:

• Browser lazy loading fires before our JS. This causes the default source to be loaded by the browser even though we don't always need it.
• I move the intersectionObserver to attach to the image only once so we can do all our calculations (not just the preloading).
• I've added an offset to the intersectionObserver so we start loading the images slightly before it is actually visible. I've added a check to updateImage() to see if an image is visible so we can keep calling this method as often as we like. It will only do things when needed.
• I also added a check to see if an image is already being loaded so the different observer won't update the image simultaneously.
• I have removed the preloading entirely, since it doesn't really seem to make a difference and it caused the image to be loaded twice when browser caching is disabled.

The results are amazing. Even a relatively simple page with only a dozen of images went from 418 kB / 925 kB transferred | 403 kB / 1.1 MB resources to 185 kB / 654 kB transferred | 196 kB / 874 kB resources on first page load.

Please review the patch before I commit it. After this one is in I will create a new release.

Thanks, went ahead and committed this. All major browsers support WebP by now. No need to serve WebP conditionally anymore.

Thanks for this. Fixed it and some other minor PHPstan errors.

Not sure if I'm ready to support this. Responsive Images imply that we should downscale and optimize images that are too big. Small images don't need to be optimized for different devices.

I think developers should encourage editors to upload images that have enough quality for the places they are used on the site. That being said, media can, of course, also be used in WYSIWYG editors, where you should probably allow more freedom. For those cases, a simpler media view mode (not using the Easy Responsive Images formatter) could be used to show images as they are uploaded. For example, a view mode called "Small" (or something similar) using an image style to enforce a maximum width without upscaling.

I'll leave the issue open for more feedback. I might be missing something.

Thanks, sorry I missed that. Committed with a minor change.

In the end we couldn't get the query to run with all the added joins from the query alters. Attached patch fixes a notice to allow us to remove the query alters of the groups module completely so we can implement our own (lightweight) version that works for our specific case.

If anyone else needs this, to remove the group module hooks we do this:

/**
* Implements hook_module_implements_alter().
 */
function mymodule_module_implements_alter(&$implementations, $hook) {
  // We specifically unset the query alters from the groups module because they
  // add a bunch of joins that make the query super slow. Since we add the
  // required filters in our views already, we don't need the ones from the groups
  // module.
  $query_alter_hooks = [
    'query_alter',
    'query_entity_query_alter',
    'views_query_alter',
  ];
  if (isset($implementations['group']) && in_array($hook, $query_alter_hooks, TRUE)) {
    unset($implementations['group']);
  }
}

@seixas for now you could download and apply the patch from 🐛 Large placeholders are not processed RTBC using composer to temp fix it until the next Drupal 10.1.x release.

I would argue that popperJS was convenient while it was in core, but if that is no longer the case, we should probably move to the recommended https://floating-ui.com/.

Better yet, we might want to consider using a CSS only approach. We should try not to use JS for things that can be done using CSS.

Patch is attached.

seanB → created an issue.

🐛 Large placeholders are not processed RTBC fixed the problem for us as well!

Patch is attached.

seanB → created an issue.

Closing the issue for now.

That unfortunately breaks the ajax filtering and pagers. All the settings for the viewsreference field get lost on ajax requests if we do that. I found out the hard way (in production 😱).

Patch is attached.

seanB → created an issue.

Modals can be closed using the ❎ button, and the escape key. I don't think we really use cancel buttons for modals (I'm not even sure we need them). This would be a good question to ask the UX team. If we want to add cancel buttons in modals, we might want to add them everywhere, not just for the media library.

Version 2.x does not need the patch and should already fix the issue :)

We definitely don't want an object to be passed in the URL. We could theoretically also just pass the parent entity type, ID/revision ID and langcode? We could then load the data in the AJAX request when we need it? This could potentially break any alters etc though.

In any case, the compression is quite significant for the AJAX page state.

Thanks! Committed.

I just ran into something similar. The request options we are passing in the URL is causing issue with the total URL length on some hosting providers. My initial thought is to implement something like 📌 Compress ajax_page_state Fixed . We can compress the data in the URL and then decompress it when we need it to optimize this.

A colleague just pointed out there already is 🐛 Drupal 10.1: Revisions tab appears twice on Groups Needs review . Closing this as a duplicate sorry about that.

Patch is attached.

seanB → created an issue.