🇳🇱Netherlands @seanB

We should probably use hook_form_BASE_FORM_ID_alter() to target all media library add forms.

seanb → created an issue.

It might also be good to add the current user as a cacheable dependency. Anonymous users have different caching needs from authenticated users. Leaving to needs work to make this also depend on the $settings['cache_ttl_4xx'] configuration.

Here is a first patch to added support. I will also create a MR.

seanb → created an issue.

The page cache renders the same HTML for all users. Varying the page cache by the Accept header can lead to a lot of different version of the page cache, making it less efficient. Maybe we could add a very specific custom cache context for specifically the AVIF. I thought about that, but maybe this solution is a bit easier and user friendly. Since it is a 302 temporary redirect this shouldn't be cached.

Drupal supports caching 4xx responses, so we can make it depend on the setting:

/**
 * Cache TTL for client error (4xx) responses.
 *
 * Items cached per-URL tend to result in a large number of cache items, and
 * this can be problematic on 404 pages which by their nature are unbounded. A
 * fixed TTL can be set for these items, defaulting to one hour, so that cache
 * backends which do not support LRU can purge older entries. To disable caching
 * of client error responses set the value to 0. Currently applies only to
 * page_cache module.
 */
# $settings['cache_ttl_4xx'] = 3600;

Created a MR. Feedback is welcome :)

seanb → created an issue.

Looks good to me! Thanks.

Thanks, I'll try that! The only case this doesn't work is when the user aborts after the file is uploaded. The entity the file is being attached to is never saved and the file will remain temporary as long as defined in system.file:temporary_maximum_age.

For now I will test the response subscriber and probably add something custom to remove the files for anonymous users after a short period of time. Hopefully someone can think of a better way to clean the sessions for those cases!

I'll also close the MR for now, the branch was wrong and the approach is probably to complex.

Personally I kind of like the idea of having a permission for this. Added an MR for review.
This definitely needs more tests though if this is the way we want to go.

seanb → created an issue.

I suggest leveraging the AI module as an optional dependency for Search API Solr to facilitate embeddings and/or some generic tagged service that can collect libraries that aren't providers. But being that the AI module is popular, its a good place to start.

I think the new plugin system for transformers would allow the AI module to ship with a nice plugin. Or we could add the integration here, since we have a isEnabled() method where we can check if the AI module is installed. But maybe that would be better as a followup?

Created a MR. Please review!

seanb → created an issue.

We should be able to filter the unknown parameters from the mail like this:

        $reflection = new \ReflectionMethod($builder, 'createParams');
        $valid_params = array_intersect_key($params, array_flip(array_map(
          static fn($param) => $param->getName(),
          // Skip the first parameter, which is the email object itself.
          array_slice($reflection->getParameters(), 1)
        )));
        $builder->createParams($email, ...$valid_params);

I think this might best be fixed in Drupal\symfony_mailer\EmailFactory.

Setting back to needs review, functionally it works fine for me :)

It seems my issues were caused by an AJAX element in the form, that did not have proper arguments to make it unique. Which caused issues updating the form and caused the states of the wrong form to be detached. So it seemed related but the actual cause was something else. Sorry I didn't report back earlier, kinda forgot about this comment.

@alexpott @marcoscano A lot of great work to improve entity usage. Nice!
This particular change seems to cause some issues for my current project though. If the batch somehow gets stuck or crashes, you have to start all over again. Yesterday it happened 3 times, after about 1,5 hours of running (we have a lot of plugins).

Do you have some more background on the choice to remove the queue options? Is it for performance/maintenance only? Or is there a chance of the outcome just being wrong or inconsistent?

Before we start creating our own drush command or write a patch it would be good to have some more background info.

1. +++ b/src/EntityInformationManager.php
   @@ -16,11 +16,11 @@ use Drupal\Core\Routing\UrlGeneratorInterface;
   -   * The url generator service.
   

   The URL generator was not used. So removed that.

2. +++ b/src/EntityInformationManager.php
   @@ -34,21 +34,14 @@ class EntityInformationManager extends DefaultPluginManager {
   +  protected array $entityTypes = [];
   

   This initializes the variable to fix the error.

3. +++ b/src/EntityInformationManager.php
   @@ -34,21 +34,14 @@ class EntityInformationManager extends DefaultPluginManager {
   +  protected array $entityBundles = [];
   

   Same here.

Patch is attached.

seanb → created an issue.

I took the code from https://www.drupal.org/sandbox/eojthebrave/3444194 → to try and implement support in the module. Just created a MR. Also see the notes from eojthebrave in #3397950-4: Dense Vector Search → .

I've already added a plugin manager to allow plugins to handle the code to generate vectors, plus added some example plugins for:

• Hugging face API (https://huggingface.co/)
• TransformersPHP (https://github.com/CodeWithKyrian/transformers-php)

I've chosen a model that allows 1024 vectors by default. I believe that is the maximum Solr supports. If you want to use a different model, the DenseVector Field type config needs to be updated:

field_type:
  name: knn_vector
  class: solr.DenseVectorField
  vectorDimension: 1024
  similarityFunction: cosine

Some things that might be nice to further improve:

• Make the knn query added in DenseVectorQuerySubscriber play nice with other ways of boosting specific fields.
• Allow configuring the topK parameter for the knn query.
• Allow plugin configuration for the SolrDenseVectorTransformer plugins (for example the cache dir for TranformersPHP).

Feedback would be appreciated!

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

Is this duplicated by #289240: Allow #type 'button' that aren't form submits ? It's a bit of a different approach judging from the code changes. Will need a reroll anyway because there is a merge conflict.

I don't think this is the same issue. Allowing input type "button" is not the same as switching input elements to button elements.

Merged, thanks! 🥳

Merged!

nod_ → credited seanb → .

Just found an issue using this patch. When you have 2 forms on the same page, the states no longer seem to work.
When I remove the patch everything works as expected.
When I remove one of the forms, everything also works as expected.

I was not yet able to pinpoint the exact issue, but it seems to be caused by the patch I created from the MR.

Fixed!

Looks good to me!

I don't remember why we thought contextual links did not make sense. I think it is because the use cases are relatively complex. Not having contextual links for the simple use cases in core was ok for the moment. When embedding a document link, the contextual links seem like a bit of overkill, but when embedding a slideshow, or something complex like that, it totally makes sense.

My first feeling is it depends on the view mode / media type whether contextual links make sense. At least per media type. Can we change the configuration of the filter to allow which media types/view modes should have contextual links?

Maybe we should even move the configuration to the media type / view mode config so we can also use this for entity references for example?

Reroll for the 2.x branch.

seanb → created an issue.

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.