🇳🇱Netherlands @seanB

The parameter should probably be FieldableEntityInterface, I think we should just revert commit https://git.drupalcode.org/project/quick_node_clone/-/commit/4b2e8cbbb1f...

seanb → created an issue.

The changes in cloneParagraphs are not entirely unrelated. For example, cloneParagraphs only allowed cloning paragraphs in nodes, while inline blocks can also contain paragraphs. Can you revert your changes to cloneParagraphs?

This makes sense to me! +1 on the approach.

Attached patch should fix the issue.

seanb → created an issue.

This issue might already be fixed since the code has changed in 🐛 password and password_confirm children do not pick up #states or #attributes Active

The previous patch contains an issue when the classList is undefined. Attached patch adds an extra check.

Changed the regex to skip application/json and application/ld+json script tags.

seanb → created an issue.

Added a commit to fix an issue while cloning layout builder content for the default language. It appears InlineBlockEntityOperations::handlePreSave does not run for syncing entities.

Allowing the behavior to be changed using a class might make sense?

Created a MR.

seanb → created an issue.

This looks good to me! The URL discovery is weird and hopefully we can get rid of this completely in the future. But that should probably be another issue.

Since there is no UI, we probably also need a change record to describe how users can enable the URL discovery again (at least until this is moved to contrib).

Thanks for the changes, this looks good! I will try to test it shortly. If other people can check this out and RTBC that would also help!

Whoops! Yes it was. Sorry about that.

Thanks for the changes, this looks good! I will try to test it shortly. If other people can check this out and RTBC that would also help!

There are multiple ways CSS can make the flash less obvious. The problem is it depends on the design of the site what the best way would be to fix it. If we add a solution for this, some sites would have no problem with it, but other ones might have to add CSS to undo/change it. Some sites might not even care about this flash.

I guess in the end it is up to developers and designers how to best solve this, and for that reason I think we could document ways to improve this, but prevent adding stuff developers might have to undo. 🐛 Make lazy loading more like the native browser behavior Active would also be a an improvement to load images earlier for browsers with a good connection, which would also mitigate this problem.

Having a button to delete the generated styles would help! I have no problem adding something like that to the settings page and documenting it. The message could be a nice reminder for people uninstalling via the UI, but it is a nice to have.

Created MRs for the 2.x and 3.x branches.

seanb → created an issue.

That sounds good! We could also consider just rewriting this issue instead. I’m not too keen on adding specific support for other crop modules as it's currently done in the MR, so maybe we can close this and either open a new one or simply revise this issue.

Merged! Thanks!

I agree this should be a global setting. Added this to the config page we have right now is a bit weird, but I guess I can live with that.

The default should be what chrome does. I don't think this is something lots of people think about. I just know at least 1 example of a site that I would like to have a lower default for performance, so that is why I chose 300 in the first place :)

We should have an update hook to set it to our new default, for the example site I would want a lower default for, I can change it manually.

Thanks!

Now ✨ Alter hook to adjust image effect RTBC is in, we at least provide an official API to configure effects on the generated styles.

In an ideal world, we provide an interface like the image styles entity where you can add and configure effects you would like to apply to all generated styles (same as you would apply effects to normal image styles).

Not all effects make sense (like the ones for changing the width/height), but I guess all other effects would be nice. For example, the Image effects → module also provides lots of effects sites might want to add.

What do you think about this idea?

I'm not sure about these workarounds for the flash being added to the module. We probably shouldn't be to opinionated about this. It might make sense to add the CSS example to the README file and point to this issue?

Looking at this again, I guess this is a tradeoff between initial loading times and perceived performance while scrolling.

Could we maybe make this configurable with a link to https://web.dev/articles/browser-level-image-lazy-loading#distance-from-... in the settings page? I would be happy to default to what chrome does, but I can think of some cases for media rich sites where you might want a smaller offset.

Merged, thanks!

Merged, thanks!

This should probably not be the default behavior. The image styles might be used in other config as well, which could become problematic.

Not sure if we could allow users to choose before they uninstall? Maybe we could set a message explaining the generated image styles for the module are not automatically removed, and link to a page where they can remove the generated image styles first?

Merged ✨ Alter hook to adjust image effect RTBC ! I think the patch in #5 makes sense, but could you create a new issue for that with an MR? I'd be glad to merge that as well.

Regarding this issue, I guess if we could drop special support for the avif, webp and imageapi_optimize_webp modules now image_convert is in core. At least that makes sense.

Maybe we could create a setting in the module to select if we want to convert images to avif or webp automatically, and add the image_convert action to all generated image styles?

I like it. The ability to alter the image style entity is relatively simple, and provides all the flexibility developers need. Thanks!

@avpaderno fair enough. Can the ownership of https://www.drupal.org/blackbird → be transfered from my account to https://www.drupal.org/u/mathijs_blackbird → ?

seanb → created an issue.

Here is a patch to show the fix.

seanb → created an issue.

Merged!

If the default revision still contains the translation, we can still allow older revisions to be loaded.

I just ran into the same issue working with translations and content moderation. Specifically when:

1. The user adds a translation (from EN to NL for example).
2. The user publishes both EN and NL.
3. The user then removes the NL translation.
4. More revisions are made for EN (these revisions no longer have a copy of the NL translation since it has been removed).
5. The the user visits the translation overview, somehow it seems the NL translation still exists, so the user can't create a new translation, and is also not able to delete the NL translation.

When I tried the latest patch I still found two issues:

• The translation overview uses the latest translation affected revision. Which made it seem like the NL translation could not be removed (since the NL langcode had an older translation affected revision). We should probably also check here if the latest translation affected revision is newer than the default.
• The entity repository EntityRepository::getActive() also loads the latest translation affected revision. This prevented creating a new translation from the default revision.

An updated patch is attached.

Patch is attached.

seanB → created an issue.

Attached is a patch to fix this.

seanB → created an issue.

Patch is attached.

seanB → created an issue.

Added a commit to the PR to address #159 and remove any existing usage when a block is made reusable.

Added fixes.

Just committed this since I'm using it successfully in existing projects. It also doesn't really do anything without the AVIF module enabled.

Merged!

Added a commit to the PR to address #158.

Fixed!

seanB → created an issue.

Fixed!

It seems 🐛 Warning: Undefined array key 1 in Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies() after update to Drupal 10.2.2 and when views ajax is enable. Active and 📌 AttachedAssets should validate libraries internally Active already exist to improve this.

Came here via 📌 Compress ajax_page_state Fixed and 🐛 Warning: Undefined array key 1 in Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies() after update to Drupal 10.2.2 and when views ajax is enable. Active . I also got the Warning: Undefined array key 1 in Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies() error because system_js_settings_alter ended up returning an empty string. When this string is compressed and unpacked this ends up adding an empty library which can not be resolved.

Regarding the solution, I think it would be best if system_js_settings_alter simply doesn't add $settings['ajaxPageState']['libraries'] when the list of libraries is empty.

So instead of this:

    // Provide the page with information about the individual asset libraries
    // used, information not otherwise available when aggregation is enabled.
    $minimal_libraries = $library_dependency_resolver->getMinimalRepresentativeSubset(array_unique(array_merge(
      $assets->getLibraries(),
      $assets->getAlreadyLoadedLibraries()
    )));
    sort($minimal_libraries);
    $settings['ajaxPageState']['libraries'] = implode(',', $minimal_libraries);

We do this:

    // Provide the page with information about the individual asset libraries
    // used, information not otherwise available when aggregation is enabled.
    $minimal_libraries = $library_dependency_resolver->getMinimalRepresentativeSubset(array_unique(array_merge(
      $assets->getLibraries(),
      $assets->getAlreadyLoadedLibraries()
    )));
    sort($minimal_libraries);
    if (!empty($minimal_libraries)) {
      $settings['ajaxPageState']['libraries'] = implode(',', $minimal_libraries);
    }

I attached a patch for anyone that needs it. I'm not sure if this is the correct issue to address it, I can create a separate issue if needed.

It would be great if we could provide a more helpful error message to help developers fix the error when it somehow happens.

seanB → created an issue.

Here is a first patch with support for the Avif module → .

I had an issue in AJAX views not picking up button[type=button]:

I needed the following to fix it:

diff --git a/core/modules/views/js/ajax_view.js b/core/modules/views/js/ajax_view.js
index c76e7073f5..aafb4aac45 100644
--- a/core/modules/views/js/ajax_view.js
+++ b/core/modules/views/js/ajax_view.js
@@ -101,7 +101,7 @@
 
     // Add the ajax to exposed forms.
     this.$exposed_form = $(
-      `form#views-exposed-form-${settings.view_name.replace(
+      `#views-exposed-form-${settings.view_name.replace(
         /_/g,
         '-',
       )}-${settings.view_display_id.replace(/_/g, '-')}`,
@@ -143,7 +143,7 @@
     // Exclude the reset buttons so no AJAX behaviors are bound. Many things
     // break during the form reset phase if using AJAX.
     $(
-      'input[type=submit], button[type=submit], input[type=image]',
+      'input[type=submit], button[type=submit], button[type=button], input[type=image]',
       this.$exposed_form,
     )
       .not('[data-drupal-selector=edit-reset]')

I was waiting for support in Edge, but apparently this was added earlier this year. I guess we can do this now! Thanks for the reminder I will try to get this in asap.

Merged! Thanks.

I guess that would make sense when the imagecache external module is installed. I would definitely accept a PR to implement this.

I just didn't have the time to implement this yet, but I agree that it would be a nice feature!

Instead of the URL I think we only need an ID. That being said, we should probably not throw and error. We might be able to do the following:

• Harden the code to show better error messages when something goes wrong
• Automatically extract an ID from URLs before we call the API

Would you be able to create a PR for this?

I think you can technically already use ENV variables or external services to fetch values for the settings.
$settings['media.external_provider.unsplash.access_key'] = getenv('UNSPLASH_ACCESS_KEY');

I'm not opposed to adding support for the key module. Does the key module allows overrides for settings as well, or only for configuration?

Patch is attached. Also created a MR. Please review!

seanB → created an issue.

+1 for moving the removal of the library to a followup!

MR!8515 does not solve the issue. If a node or revision is deleted during the batch process (by something or someone outside of the batch process), $node is NULL. This throws an error when trying to call _node_mass_update_helper.

Since I needed something that works in a current project, fixed the issues found in my last review. I agree we should get answers for the questions in #93.

+++ b/core/modules/user/src/Entity/Handler/BatchCancellationHandler.php
@@ -0,0 +1,216 @@
+      $this->updateEntity($entity, $method);

As found in 🐛 _node_mass_update_batch_process fails during user cancel when revision is deleted Needs review the entity could technically be removed between creating the batch and the execution of this method. We should check if $entity is properly loaded.

+++ b/core/modules/user/src/Entity/Handler/DefaultCancellationHandler.php
@@ -0,0 +1,185 @@
+    $this->storage->save($entity);

As found in 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work a new revision is created when content moderation is used. We should probably try to prevent this by checking for SynchronizableInterface and doing something like this:

  $original_syncing = $entity->isSyncing();
  $entity->setSyncing(TRUE);
  $this->storage->save($entity);
  $entity->setSyncing($original_syncing);

Created a PR.

seanB → created an issue.

Entity Browser has entity_browser.view.js that performs some magic. Since we do not properly use the Entity Browser modal it seems parts of it do not work as expected. I agree hook_views_pre_render is a workaround for this, but it seems a bit easier that properly integrating Entity Browser modals.

Jean-Paul Vosmeer → credited seanB → .

Created an MR that passed the parameters. Also added a custom ckeditor5 parameter so we can check when to forward it for views.

seanB → created an issue.

+1 from me!

Merged! Thanks again :)

Fixed in 🐛 There is no display available - while referencing, if the view name contains an opening bracket Fixed

Fixed in 🐛 There is no display available - while referencing, if the view name contains an opening bracket Fixed

Merged! Thanks, the tests really help already!

Merged! Thanks all!

I think the conflict project was added to the composer.json file because the project has been renamed from drupal/viewsreferennce to drupal/viewsreference. Which means you should never have both in your composer.json file. So this is probably not a typo (maybe the other maintainers can confirm this?).

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

Merged! Thanks.

Removed all the Drupal CI scheduling.