🇳🇱Netherlands @seanB

seanb → created an issue.

seanb → created an issue.

Patch is attached. The performance seems to improve quite significantly for layout builder pages.

Before:

After:

seanb → created an issue.

Apparently we already have snippets in GitLab (thanks @kevinquillen for pointing me towards that!). This might be something we can leverage. Looking at the existing snippets though, they seem to be used for all kinds of things, not necessarily for working code examples. We would at least need a way to flag or categorise the snippets in some way.

https://git.drupalcode.org/explore/snippets

Patch is attached.

seanb → created an issue.

Updated the issue summary.

Moved the issue to the Term Reference Change module since the migration is done here.

The modal currently already display all selected assets. For each asset we could show the message, allowing the user to set the existing media ID in the hidden selection field and skip adding a new media item.
I guess we could also implement both, allowing site admins to make adding duplicates optional.

Thanks for this. I guess it makes sense to have this. Not sure if we should force it though. We can do 2 things:

1. Make it a global setting, and just always either reuse existing assets, or create new ones.
2. Add a message to the form when we already found the media item. Something with a message like: The chosen item has already been imported. Would you like to use the existing item or create a new one?. When the users chooses to use an existing item, we skip saving the media and just set the media ID that already exists.

I kind of like option 2, which gives more clarity and flexibility to editors. What do you think?

Here is a new patch as well for anyone that needs it.

When editing paragraphs on the latest draft, we found that the parent entity for paragraphs was not correctly loaded. It always load the default revision for the parent entity. This is probably caused by 📌 Store information about a paragraph's parent revision Active .
Updated the MR to add a helper method that loads the latest translation for parent entities.

In multilingual sites the patch doesn't load the correct translation for the parent entity before we save it. Updated the MR to fix that.

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

Fixed an issue for when a paragraph somehow doesn't have a parent.

seanb → created an issue.

Added a change to fix an issue with the option to revert back the latest revision. This didn't properly get the correct translation for the latest revision, so the revert ended up being for the wrong language. Updated the PR and here is a patch for 2.7.

seanb → created an issue.

Also create a MR. Please review :)

seanb → created an issue.

seanb → created an issue.

Here is a first simple patch that should do it.

seanb → created an issue.

Since we now should have unique tokens for cached and uncached pages, we should be able to delete the identifier after validation.
I'll start a new MR as well.

Added some more changes to fix issues with URL encoding of tokens.

Implemented a special controller to generate new identifiers using AJAX.

I think this feedback from #58 seems valid:

The very original approach by @awolfey did something subtly different whereby there was an AJAX request made on page load to the server that would fetch in a newly signed timestamp into the form, the client wasn't trusted.

We should probably have an AJAX request to fetch a valid new identifier. We should probably also make sure each identifier can only be used once.

Reroll for version 1.12.

Reroll for version 1.12.

This might be a duplicate of #2452267: Entities can be created programatically without titles or with bogus bundle names →

greggles → credited seanb → .

The limits are there to guide users to sane settings. Adding a lot of steps and insane min/max values could potentially kill the site. Each step needs to have a URL generated, an image style, etc etc
Not sure if we should do this. You can always alter the form?

Now we have 🐛 There is no way to make a button element that can use the AJAX functionality Needs review in core, we could use this to actually create a button type="button" instead of using JS to change the button type.

Yes, the changes correctly add the nonce, which then makes sure the included matomo script is allowed. The only thing I can think of is that the error is caused by a different script on the page? I noticed the console log messages are not always clear about the cause.

Patch is attached.

seanb → created an issue.

In version 2 of the CSP module support has been added for render elements. I think this might fix the issue: ✨ Allow CSP to be added by render elements Fixed

Fixed issue credits.

Fixed issue credits.

I think this was definitely fixed by https://git.drupalcode.org/project/easy_responsive_images/-/commit/ab618...

Please test the new release 1.5.0.

Merged!

Merged, thanks!

Since we have 🐛 Flash of missing image when lazy loading Active to handle the flash, and the image size needs to be done using CSS, I think this is a won't fix issue.

Merged into the new 1.5.x release.

✨ Do not load the smallest version of an image for media previews in CkEditor Active contains an improvement for media in WYSIWYG.

Regarding the CSS added in this issue, how about this:
I think we might be able to add it as a configurable option. We could add a select field to the settings "Image loading animation", with the options "None" and "Fade-in" or something like this (naming can be discussed).
We can then conditionally load the CSS when a user configures this. This would also allow us to add more loading animations later.

I can not reproduce this using the latest version of the module with the latest version of Drupal core.

Attached is a patch to fix this.

seanb → created an issue.

Thanks @tstoeckler! I haven't tried that, the site was still running D10.

As berdir correctly points out, there are a lot of cases that this patch currently doesn't handle. Before I continue, I think it is important to highlight that most editors will not know there is a difference between files and media. I've seen cases where a users accidentally uploads an excel file containing passwords (which also ended up in google). When this user deletes the media item, it is extremely important that the associated file is also deleted. I think this is the main use case we should be trying to solve in this issue.

If I'm not mistaken, we have (at least) the following cases.

Deletes:
1. A user deletes a media item without revisions
This is what the patch in this issue solves. It is a relatively simple use case, since we have a clear indication that the users wants a file removed (as long as it does not have other usages).
2. A user deletes a media item and all its revisions
This becomes a bit more tricky, but in this case I think we can safely delete all files related to revisions of the media item (as long as they do not have other usages).
3. A user deletes a single revision of a media item
This is even more tricky, the file can still be referenced by other revisions of the media item. And example of what this can lead to is 🐛 File usage is not tracked by revision, leading to private files embedded in text fields in old revisions being accessible when they shouldn't be Active . If we want to properly clean up these files, we need to track file usage per revision. We should probably handle this in a followup (if there is not an issue for this already.

Updates:
4. A user updates a media item without revisions
This should be relatively simple. If the original file is not used in other places than the media item, we can simply remove the old file.
5. A user creates a new revision for a media item
This is just as tricky as deleting a single revision. We basically need usage tracking per revision to solve this properly.

If possible, I think we should focus our efforts for this issue on 1. and 2. We should create a followup for issue 4. We should create different followups for case 3. and 5. and block those on tracking file usage per revision.

It might be good to mention I found 🐛 Allowed memory size exhausted when using revisions caching Active while using this patch. There could be contrib/custom code loading lots of revisions. When we suddenly start caching these, the cache could easily go over the memory limit.
I wonder if we could automatically release items from the cache when it starts to reach the memory limit? That might be a different issue though.

Attached patch fixes the issue for us.

seanb → created an issue.

I understand the need to remove files related to media (it can even be considered a security issue that the related file is not removed). I add a hook for this in basically every project I do. So +1 for adding this to core. I also guess it is good for BC to make this optional.
Do we really need the disable_delete_control option though?

I would suggest that if the configuration specifies files should be deleted when media is deleted, then that should happen automatically, without presenting additional choices or options that might confuse editors. The distinction between files and media isn’t always clear, and adding more options could increase the potential for misunderstanding. We'd have to explain when files can be deleted, when they should be kept, and what happens if someone chooses not to delete a file but changes their mind later. We can avoid this complexity by keeping the configuration straightforward and not allowing individual overrides when deleting a media item.

Attached patch seems to fix it for us.

seanb → created an issue.

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.