🇺🇸United States @azinck

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

That's great news! I'm thrilled to hear there's a new asset browser coming. It's much needed.

Actually, I didn't really think this one through much. Why has this been submitted to this project? This plugin seemingly has nothing to do with Search API. It would make sense as a contribution to Views itself.

Patch of the latest state of the MR.

pbonnefoi → credited azinck → .

pbonnefoi → credited azinck → .

I've opened a merge request with these optimizations. The improvement is dramatic. After label, section, and collection data has been cached the initial load of the media library has gone from about 7.5 seconds down to about 1, and every interaction with the filters has gone from a whopping *20 seconds per interaction* to just 1 second per interaction!

Previous to this optimization, every interaction with filters entailed a full 71 requests to the BF api for me (your numbers will be at least partially driven by the number of labels in your BF instance)! The optimization gets this down to 1 request.

I've arbitrarily chosen a 10 minute cache TTL for the label, section, and collection data. I want them to reflect changes in BF in a relatively timely fashion while still benefitting from the caching. I could see exposing this as an option to users, or even letting the user choose to have the cache never expire and forcing the user to do a manual re-load of their BF data if it's stuff that changes extremely seldom. But a 10 minute TTL seemed like a reasonably balanced starting point.

azinck → created an issue.

I just want to add my vote that the change added in 📌 After logout due to inactivity , login redirection is not right Fixed was not appropriate for the scope of this module, and reflective of a very narrowly-considered use-case. It should be removed. There are a million other situations just like this where if a different user logs in they could end up on pages that they don't have access to. That's just intrinsic in the way this works. It's impossible and inappropriate to account for all of it.

I don't even think being able to set some sort of general redirect destination when the user doesn't have access to the destination page should be handled by this module. The `destination` functionality is part of Drupal core and can be set and used by a variety of modules, not just this module. There's no reason this module should modify its behavior. If some other module wants to take on that task then fine, but I think it's clearly outside the scope of this module.

Here's another vote for reconsidering this decision. I agree with the points made above. Config_split is a very non-optimal way to solve this problem.

I've attempted to performance-test the merge request here. I'm having a hard time coming up with reliable results showing it improving things. I've used Siege to test a set of pages, attempting differing amounts of concurrency, as both an authenticated user and not and cannot reliably get it to show a performance difference.

The best I've been able to achieve is using a profiler comparing loading 2 pages, both of which have menus on them:

Pre-patch:
- Page 1: 17.8s
- Page 2: 8.02s

Post-patch:
- Page 1: 17.8s
- Page 2: 6.6s

Understandably the savings is limited to the second page (though, interestingly, I might have thought the use of loadMultiple might have been realized in the first page load but I'm not seeing that). And on the load of the 2nd page I can see virtually all of the savings comes in the processMenuLinkTree method, going from 2.07 s → 290 ms.

However, it's somewhat strange to me that I can't seem to find meaningful improvement when using Siege across a wider array of pages. I'm hitting each page once concurrency of 1 to maximally benefit from caching (though I've experimented with other values), always running it immediately after a cache clear and it's just not consistently showing me a benefit despite repeated tests.

Thanks for the contribution! I'm waiting to hear from at least another person that this works for them before merging it.

Whoops, I forgot to rip out the now-unneeded CSS changes. Let's try that again.

Here's a different approach that retains the modal context and just scrolls the user back to the top of the div, putting the assets in view.

alexpott → credited azinck → .

Well, I went ahead and did it (needed it for my own use): https://www.drupal.org/project/search_api_extras →

Would you like me to add [#33279981] to it?

Thanks for the fix!

@solideogloria Then you get into a situation where you have an inconsistent # of results per page (because some are culled by your PHP code) and other complexities that come out of that.

As a first step (and to achieve something we needed to do), here's an extremely simplistic patch that at least allows custom_field criteria to be added to the configuration via non-UI means and understood by the fetchAssets call. I realize we could extend or override the service with our own implementation to achieve this but it would mean a lot of duplication of code in the fetchAssets call, which is non-ideal.

With this patch in place we can, for instance, add a line like this to our settings.php to limit the results for our brandfolder_image media type to only assets that have a specific school (or schools) in our Brandfolder instance:

$config['media.type.brandfolder_image']['source_configuration']['brandfolder']['bf_entity_criteria']['allowed']['custom_fields']['School/Center'] = ['Annandale HS'];

I took the opportunity to refactor the existing a code a bit to reduce redundancy. I think further redundancy could be eliminated in the label code, too, but I wasn't 100% sure what all it was doing and didn't want to dig into it too much for the time being.

azinck → created an issue.

I haven't tested this with Brandfolder, but I'd expect this module to solve your problems: https://www.drupal.org/project/exif_orientation →

I appreciate you reaching out and your contributions to the project, Eric. For now I'm not going to add more maintainers but keep me posted if we're getting in the way again and I'm open to reconsidering. Cheers!

Thanks for the fix!

Thanks for the fix!

Here's a further change that is possibly a bit more controversial, but I think it's in the spirit of what this method is meant to do. This updates the previous patch to strip ALL of the instances of the given tag from the beginning/end of the markup. This is how I expected the function to work given the fact that "Tags" is plural in "cleanExtraTags" (it's not "cleanExtraTag").

So if you have something like:

<p>Text goes here.<br><br><br><br></p>

Then ALL of the
tags will be stripped when you run that through cleanExtraBrTags with this patch.

azinck → created an issue.

I've found the sidebar tray solution utterly unworkable for any significant content creation. For folks with the same problems I recommend https://www.drupal.org/project/layout_builder_iframe_modal → as it frees up a LOT more space.

Re-rolled 23 for Drupal 9.5.x.

Committed, thanks!

Committed.

azinck → created an issue.

I'm very happy to say that the bug I noted in #12 seems to be resolved by the MR on this issue!

I don't believe this issue is limited to media fields. I'm seeing the same behavior with paragraph fields. I have the following structure:

Content type with these fields:

• field_paragraphs - entity reference revisions field using the Paragraphs (stable) widget
• field_wide_content - entity reference revisions field using the Layout Paragraphs widget

Then I have a paragraph type called a "Summary Box" with the following field:

• field_paragraphs - entity reference revisions field using the Paragraphs (stable) widget

If I add a Summary box paragraph to field_wide_content, and then try to add a paragraph into that paragraph's field_paragraphs field I don't see the new paragraph added there in the modal. But if I close the modal I can see it got added to the field_paragraphs field on the parent node instead. If I then re-open the modal to edit my Summary box paragraph I see the paragraph I added earlier (yes, the same paragraph is now in 2 places). Furthermore, edits to content in the node's field_paragraphs field are reflected in the Summary box paragraph's field_paragraphs field. This is on Layout Paragraphs 2.0.2 and Drupal 9.5.7.

@leisurman: This patch is only relevant to the 1.x branch of the module. Since you're on 2.0-beta4 it's not relevant. Are you seeing this issue on your version of the module? If so, I'm somewhat surprised.

Let's try the patch from #8 again.

Rerolled #6 for the 2.0 branch of the module.

This patch was committed a while ago but I neglected to properly reference the issue in the commit.

I agree this is needed. +1 for the approach in #6.

To reproduce this error you need to have code that calls \Drupal\fast404\Fast404::response() without passing TRUE as an argument. This appears to only happen in fast404_preboot(), so to see this you probably have to be using the advanced installation which entails modifying your settings.php to call fast404_preboot(). If you've done that, then you will hit this error every time fast404 returns a 404.

@choneyse it sounds like you're suffering from this bug: 🐛 Changing an existing embedded media's alignment or alt data attributes does not get saved with CKEditor Needs work

@catch there are quite a few issues that touch on various aspects of the terminology discussion, but the most relevant one I'm aware of is: #2899719: Revision/version language on revision listing page is misleading with content moderation enabled →