Netherlands
Account created on 6 July 2009, over 14 years ago
  • Contractor at iO 
#

Merge Requests

More

Recent comments

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands

Rerolled the patch for 2.x.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

Attached is a patch that would fix this.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

This should fix it.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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?

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands
  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.

🇳🇱Netherlands seanB Netherlands

Coul you create an interdiff for the patch?

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands

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']);
  }
}
🇳🇱Netherlands seanB Netherlands

@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.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands

Patch is attached.

🇳🇱Netherlands seanB Netherlands

seanB created an issue.

🇳🇱Netherlands seanB Netherlands

Closing the issue for now.

🇳🇱Netherlands seanB Netherlands

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 😱).

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

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.

🇳🇱Netherlands seanB Netherlands

Patch is attached.

🇳🇱Netherlands seanB Netherlands

seanB created an issue.

🇳🇱Netherlands seanB Netherlands

We extended this class in our custom code. Not sure if this is still needed and if we should just implement our own version.

🇳🇱Netherlands seanB Netherlands

This probably needs to be configurable, at least per media type (maybe even per media item). I can imagine you don't want to provide downloads for all media types. Images are debatable, but external media like youtube/vimeo video's are problematic.

We could provide a base field like promote / sticky with a default value configurable in the media type, including permissions for the field. It feels like this feature is a bit similar. We should then probably also allow media sources to opt-in to usage of the field (file based media should have that, but oEmbed media probably not).

🇳🇱Netherlands seanB Netherlands

Attached is a patch for the 2.x branch.

🇳🇱Netherlands seanB Netherlands

I think fetching the published key from the entity type is a more generic solution.

🇳🇱Netherlands seanB Netherlands

You can hack around this by doing this in composer.json:
"drupal/entity_clone": "2.0.0-beta4 as 1.0"

It would be great to have this committed though, this is quite a nasty workaround.

🇳🇱Netherlands seanB Netherlands

The latest patch in Drupal 10 readiness Needs review has this incorporated, and more D10 related changes (like 💬 Cannot install Entity Clone 2.0.0 because this module requires ^1.0 in composer.json Active ). Closing this as duplicate.

🇳🇱Netherlands seanB Netherlands

Crap, forgot to add the patch.

🇳🇱Netherlands seanB Netherlands

Here is a patch that combines the changes in this issue, 💬 Cannot install Entity Clone 2.0.0 because this module requires ^1.0 in composer.json Active , 📌 Automated Drupal 10 compatibility fixes Needs review and 🐛 TypeError when Cloning Menu Needs review to make everything work in D10.

🇳🇱Netherlands seanB Netherlands

Ah, if this is no longer an issue in 2.x I think we should close this. We should probably migrate people over to 2.x at some point. Most active development is going on in that branch. I think we are almost ready for a stable 2.0 release.

🇳🇱Netherlands seanB Netherlands

I needed to update the method signature for the normaliser to allow the update to Drupal. Patch is attached for anyone that needs it.

🇳🇱Netherlands seanB Netherlands

Thank you for testing! This at least solves the issues for the existing formatter currently broken in the latest release (for some sites). We can continue the development and debugging of the lazy builder in new issues. If someone else could also check and RTBC this I will try to commit this asap and create a new release.

🇳🇱Netherlands seanB Netherlands

Updated the IS, guess I made a copy-paste error.

🇳🇱Netherlands seanB Netherlands

Hmm, 2 seconds seemed to short after all. Sorry about that. Checking if the file exists again seems like a good idea though. No need to add another lock in that case.

🇳🇱Netherlands seanB Netherlands

If another thread is generating the image, we might no longer need the lock to generate the image. We should probably check if the file exists after a timeout. Also, 5 seconds seems quite long. Changed it to 2 seconds.

🇳🇱Netherlands seanB Netherlands

Since the media_entity module has moved to core a long time ago it is no longer used, so we should either move this issue or just close it. I personally think that a contrib module like https://www.drupal.org/project/media_entity_download is a great solution. Not 100% convinced that core should provide this out-of-the-box. But I guess it's fine to move it to the core issue queue to get more opinions.

🇳🇱Netherlands seanB Netherlands

Well, the list of issues in #31 is a pretty big indicator that people are struggling with problems because of the fact that it is impossible to create an <input type="button"> at the moment. Of course, we could do this in contrib, but that would mean all modules that have AJAX buttons would need a dependency on this new module to solve the issue.

Adding a flag like #submit_button to the existing button element would probably be the easiest and most developer friendly way to solve this. So I'm hoping this is a thing we could add to core.

🇳🇱Netherlands seanB Netherlands

Sorry about that, some changes were missing in my previous patch (forgot to git add before creating it). Could you try this one?

🇳🇱Netherlands seanB Netherlands

I don't think #9 fixes the issue. The problem is in the browser. When pressing the enter key, the first input element with type "submit" is triggered, instead of the actual form submit button. We should really stop using type "submit" for AJAX buttons, since they are not the primary form submit button. This is caused by the fact that Drupal\Core\Render\Element\Button always uses "submit" as the button type.

So we either need to add a new setting to allow changing the button type, maybe a key like #submit_button?

 $form['ajax_example'] = [
   '#type' => 'button',
   '#submit_button' => FALSE,
   '#value' => $this->t('My AJAX button'),
 ];

Or another render element that always uses the button type. Something like #type = 'ajax_button' (although it is not necessarily always going to be used for AJAX stuff).

🇳🇱Netherlands seanB Netherlands

Let's start by getting this RTBC first, and then try to ping the maintainers for a new release.

🇳🇱Netherlands seanB Netherlands

Yes, #3380691-16: Issue 2919092 breaks BC and causes issues for existing sites contains a patch to revert the changes and implement the lazy builder as a separate formatted. It would be great if you could help test that!

🇳🇱Netherlands seanB Netherlands

The latest version of the MR added changes to allow us to add the alter hook in MediaSourceBase::getMetadata. That still needs to be implemented in the MR though.

If we want to allow dynamic attributes to be mapped in the interface we also need to change getMetadataAttributes. The idea was to move the metadata attributes to the plugin annotation, and have getMetadataAttributes return whatever attributes are configured in the annotation. The annotations can then be altered via hook_media_source_info_alter.

That being said, technically you can already override the media source plugin class using hook_media_source_info_alter and override getMetadataAttributes and getMetadata for a specific media source. Which kind of makes me wonder if we need to implement this new hook at all. Although I know that lots of people probably find hooks much easier to implement than overriding a plugin class 🤔.

🇳🇱Netherlands seanB Netherlands

Added a new setting to the formatter to allow it to use the height of the container to select the best image. If you are not using object-fit, it won't make sense to do that. This makes it configurable.

🇳🇱Netherlands seanB Netherlands

What was the size (pixels) of the smallest image compared to the LQP (compared in bytes)?

🇳🇱Netherlands seanB Netherlands

Sorry for the late response. This just got on my radar again. Can we reroll this now 🐛 Update viewsreference_update_8103 fails if table name is too long and shortened by Drupal Fixed is in?

🇳🇱Netherlands seanB Netherlands

This is an issue for the views_ajax_history module. See 📌 AJAX GET in core: Alter ajax url query parameters Needs review for a way to fix this.

🇳🇱Netherlands seanB Netherlands

A schema has been added in 🐛 Update viewsreference_update_8103 fails if table name is too long and shortened by Drupal Fixed which has just been committed. Closing this one.

🇳🇱Netherlands seanB Netherlands

Finally got around to commit it. Thanks everyone for your hard work! ❤️
This is a great step towards getting 2.x stable!

🇳🇱Netherlands seanB Netherlands

Attached patch should work with the latest version. Could you please check if that fixes the issue?

🇳🇱Netherlands seanB Netherlands

I think 🐛 Viewsreference embeds Views without cache, breaking Dynamic page cache Fixed broke BC and a lot of custom code. For that reason the issue is probably valid. Renamed the title.
I'm going to revert the changes in 🐛 Viewsreference embeds Views without cache, breaking Dynamic page cache Fixed and introduce the lazy builder option as a new formatter for people that want/need it.

🇳🇱Netherlands seanB Netherlands

This has probably been caused by 🐛 Viewsreference embeds Views without cache, breaking Dynamic page cache Fixed . I'm going to revert that commit since this also caused issues for others. See 🐛 Issue 2919092 breaks BC and causes issues for existing sites Needs review for example. Closing this as a duplicate of 🐛 Issue 2919092 breaks BC and causes issues for existing sites Needs review .

🇳🇱Netherlands seanB Netherlands

Great to hear this had been helpful! To extend the metadata options, we have been exploring a way to do this generically in Add a hook to modify oEmbed resource data Needs work .

The MR is closest to what I think it should be, not sure what is happening with the patches. When we start using getRawMetadata and getMetadata, the core getMetadata method should add a hook to allow altering the metadata and return whatever you want. We should then probably do something similar for getMetadataAttributes and getRawMetadataAttributes.

🇳🇱Netherlands seanB Netherlands

This seems to be caused by the same issues as 🐛 Issue 2919092 breaks BC and causes issues for existing sites Needs review . Since the other issue has more information, I'm closing this as a duplicate.

Production build https://api.contrib.social 0.61.6-2-g546bc20