🇪🇪Estonia @rang501

Maybe core.extension configuration contains references to removed themes? Recent upgrade changed something and this triggers an error.

Can be checked with drush: drush cget core.extension

Patch in #18 seems to solve the issue. I think it is a major issue because it will cause data loss if you happen to delete one of the nodes.

Updated patch for 3.5.x version

I just updated to 1.15 and it seems to be broken

TypeError: Unsupported operand types: array + null in Drupal\office_hours\Plugin\views\field\FieldBase::viewsFieldData() (line 81 of modules/contrib/office_hours/src/Plugin/views/field/FieldBase.php).

Is this still relevant after 🐛 referencedEntities() causes data loss Needs review ?

Do you have the latest entity_embed module? CKEditor integration is coming from that module.

There is a patch for this already in #3383658-13: Images in media browser have no hover effect and selected effect in media browser → which can be tested. This one will make seven theme as a dependency which is a contrib theme in D10 and is most likely not installed most of the sites anymore.

Unfortunately, Drupal 9 is EOL and 2.0.x is not supported anymore. Please update to the 2.1.x version and if the issue still exists (there were many UI-related changes), feel free to open a new issue.

I found it - someone put a regular config file (which contained langcode) inside the config/schema directory and config schema discovery tried to parse it.

The value of $definition, when the error is triggered is just 'en'.

I also ruled out patches by removing them, still the same issue. Removed config_ui module as well, same issue.

rang501 → created an issue.

There is a bug inside the hash calculation. The $cache_context_keys is not different and the problem appears when there are two themes enabled (in my case, the main theme and sub-theme). The [theme]= context key is wrong, it was gin (admin theme), not the theme that was provided as a function parameter, so the hash was the same and it generated one set of the CSS files and skipped it later because the hash was in cache already.

So I added this before the hash function:

    foreach ($cache_context_keys as $key => $value) {
      if (str_starts_with($value, '[theme]')) {
        $cache_context_keys[$key] = '[theme]=' . $theme;
      }
    }

then it started to work.

I tested on the 1.x version and Drupal 10.1. The patch in #73 fails to apply, #72 works fine, and did not see any issues with aggregation as well.
It seems to work as expected, but I'm not sure if making methods private is a good idea - decorating the service is problematic. I need to duplicate many methods to apply some additional color replacement logic there.

It was quite late and I guess I forgot :D

I made some changes that seem to fix Seven theme support. It just includes the libraries in the required places.

@ytsurk
I think the page size should be larger and should reflect the maximum upload size limit, otherwise, we will have a situation, where the user uploads like 20 files, and some of them are on different pages. I did test it and after selecting 20 files, the field had only 18 added.

The default PHP limit for files is 20 and I think it's something that's changed less frequently than upload size.

This seems to work as expected.

Steps:

1. Apply the patch
2. Add new media (I used image), file name was prefilled as I have the mapping set (default in Drupal install)
3. Change the name mapping value to Skip, and expose the name field (it is still pre-filled if the name field is not exposed on the form)
4. Upload a new image, field is empty.

I looked at it and found out that the configuration form is not saving the checkbox value. I did find the problem and fixed it.
Another thing - the bold text about the manual cache rebuild - it was not necessary and I did remove it. I think that any manual cache rebuild is only for cache debugging. This is something that the users shouldn't do at all. We should add correct cache tags or call specific cache rebuild functions during the settings save. At the moment I do not see there is any need for it.

Overall, it seems to work just fine. I do agree that the option should turn off the dnd functionality completely (both folders and media).

I'm thinking that some configuration is not correctly installed, trace seems to show that it tries to render view. Does the view media_directories_base exists?

@ytsurk I think the issue is that existing media has an existing reference field (I guess field_media_path) and after enabling the module, these are not used.

We can't do anything special here. Finding existing term reference fields is error-prone and I think if someone has such a case, they need to migrate the id-s manually. Unless we want to make the field configurable somehow. Fortunately, the migration is quite simple (with hook_update_N or the SQL query mentioned above, unless two or more references are allowed), so I think we don't need to make it configurable.

As for allowing to assign media to multiple folders - it is possible, but there are UI changes needed (moving files in the browser), and migrating that field to a separate db table needs a migration path.

I found the issue - it seems that the keydown was triggering the form submission, so I added separate keydown event that will prevent enter key from submitting the form.
Works both in Chrome and Firefox.

@dewalt Updating configuration is complicated, we have two problems here -
1. If we update it, it will override customizations.
2. If we don't update it, users must do it manually which is probably more confusing.

It is hard to find the right balance and detecting if we need to update something is not reliable. We might be able to check if some pager or sort is already added and skip the update, but for more complex changes it is just too difficult to keep track of things.

I could take a look at it over the weekend if I have time.

I suggest something like this:

1. Change the sort order
2. Add normal full pager (to go to specific page easily) as default
3. Show 50(?) items per page

The next question would be how to make the update - just overwriting things we need or trying to detect custom changes.

The issue is with sort I guess. We could add a pager, but because the sort direction seems to be oldest to newest, we can't see the last uploaded media which is most likely the one the user wants to select. It is an easy change.
As a user, I usually scroll down and select media there. Most of the time the media is not moved to the directory for various reasons.

I could take a quick look at how rendering with entity query could look like and give some performance differences. View performance is sometimes problematic, browsers most likely can handle rendering thousands of items just fine. Using entity query will lose some customizability some users need so we could add a setting for backend selection.

This issue seems to be coming up again and again. I have it on one site as well. Views is just so slow when trying to render a lot of entities at once. It needs some POC first to see how different it is.

So we should work on it.
I'm proposing two options:
1. Integrate views_infinite_scroll (either as a dependency or switch if the module is installed on the site)
2. Add a second listing type based on the entity query, which is way faster than Views (in both loading entities and rendering)

How much memory is allowed for PHP to use?
How many media entities do you have?

I assume there are too many media entities. Because the listing is currently rendered with views, it may run out of memory if it tries to load all entities simultaneously. The workaround for this is to add a pager, like views_infinite_scroll (you need to configure the view to use that), related issue 📌 Loading times decrease for directories containing 100+ of medias Needs work

I saw some changes that were done here were added by the 🐛 Not working with Group By feature RTBC . I modified the code so this will apply with the latest patch from that issue.
At the moment this will fix

• Hierarchy functionality - it is broken in the Claro theme, it needs one header class, and rendering the indentation part was not correct, it did remove the SVG and there were no visible parent-child relationship lines available
• custom entities (at the moment it expects 'nid' as entity id key, but others have 'id' usually)

I did test this and did not find any weird behavior related to moving things around. Previously it messed things up, ordering was wrong, indentation incorrect, and was lost.

Hopefully, someone can take a look at this with the other issue and see if it is possible to merge these things to a new release.

I did add changes to make this work with Claro theme in Drupal 10. The structure was not preserved when dragging rows around and it did cause some weird issues where relationships were lost.

Updated patch with latest release

I think seven is not supported at the moment. Because the media directories is using core media module classes and styling, it should work, unless theme does something differently, like alters markup or classes, which does not affect media directories.

Maybe there is a separate js file that does the hover effect? Maybe you need to include that instead? The library definitions are theme specific and I guess seven library is not included.

The hover effect should come from the core media module. We should not add any custom styling, unless absolutely necessary (and then we try to support both claro and gin), because sites may use different admin themes, which most likely will use core media classes for styling.

I remember the hover worked fine when I worked on it, but maybe something have changed and it does not work anymore?

What is the Drupal core version and what is the admin theme?

rang501 → created an issue.

Last change sends back the media ids, but the selected items count on the bottom is wrong.
Maybe some other issues, but not checked at the moment.

rang501 → created an issue.

We can release it.

Thanks for the fixes, everything seem to be fixed now.

Took some time and made multiple fixes. Should address last comment and additionally adds cache tags (block didn't update when entity was updated).

There were also issues like file upload validators were in wrong format, LoggerChannelFactory in construct was throwing fatals errors (due to redirect module, should use interface instead).

This patch should fix it.

The issue seems to be with the order, how entities are deleted. When default translation is deleted, any translation after it doesn't exist.
How to reproduce (my case):

1. Three languages enabled
2. View lists all languages and they are ordered by changed time (it is different between translations.
3. Select entity with its translations, the default translation should be first or middle and not last.
4. Execute entity delete - error is thrown, because deleting default translation will delete other translations.

My patch sorts the queue by default language, so it is always the last item of translations. It should work even if translation is in different batch (not tested), I assume if the entity can't be loaded, it will be skipped.

The patch is for 4.2.x version.

I tried to extract it from 2.1 version, but should be tested first.

The new patch seem to give same results, dropping from 2900 queries to 2700.

It seems that it improves validation in ParagraphsLibraryItemHasAllowedParagraphsTypeConstraintValidator which caused 200 queries before because it scans all paragraphs.

About #19, doesn't

$item->entity->id()

cause extra separate entity load?

Although, it seems to improve saving time, in my case ~200 less db queries (originally ~3600, #7 ~2900, #19 ~2700).

I can describe one situation, when the problem happens:

I have two configurations, the first is workflow configuration, which depends (enforced) on the workbench_email configuration, which depends on the role configuration. Now, uninstalling one module which provides some permissions for that role (in my case user_shortcut module), it does the following:

• it updates role configuration (correct)
• it updates workbench_email configuration (correct)
• it deletes workflow configuration (wrong)

It does not explain, why it happens on some environments (it is database related, because I was able to reproduce it with dump). The differences are hard to track as there is no config sync, so environments in my case are somewhat independent.

Will be resolved in 2.1 release.

It is like that, but composer update throws an error:

drupal/media_directories dev-2.1.x requires drupal/entity_embed >=1.3 -> found drupal/entity_embed[dev-8.x-1.x] but it conflicts with your root composer.json require (dev-3272732-drupal-10-).

But I was able to bypass it with inline alias:
"drupal/entity_embed": "dev-3272732-drupal-10- as 1.3"

So I guess, its fine then.

Latest change is causing issues - composer doesn't like custom fork of entity_embed module now. Do we need version restrictions at all?

I can add that it seems to work on our projects, no issues so far.

Thanks for feedback.
New patch is here

Patch had path issue.

This fixes (hopefully) the failed test. But the previous test question remains - not sure how to handle it.

Created patch based on case 1

I can say this issue is still happening.

We seem to have an issue during module uninstall, where it pulls some extra configuration in through role configuration and then just deletes multiple views and workflow configurations. Applying the change in case 1 almost solved it (one configuration still was marked as deleted).

Not sure how to reproduce it - it is environment specific (probably depends how dependencies were calculated).

Ok, here is the patch with only my changes.

The issue is with test line (SimplenewsSourceTest):

      // And make sure it won't get encoded twice.
      $this->assertEqual($from, Unicode::mimeHeaderEncode($mail['reply-to']));

Sorry about that, now it should be fine.
There was one test that I didn't know how to change and I left it as-is just in case I mess up something :)

Hopefully these changes help. As I understand, iconv_mime_decode should be used instead.
Related CR: https://www.drupal.org/node/3207439 →

Release 2.1 should come in any day, for testing, you could check dev release and give feedback.

Created issue fork and fixed the patch so it applies to 2.1.x.

Ajax throbber issue should be fixed, the z-index was wrong, caused by small class name change.

I created an issue fork and applied the latest patch.
There were one issue with once(), which is now fixed and the entity browser under Gin theme looks fine.

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

There is a fatal error with sending with Drupal 10:

Error: Call to undefined method Drupal\Component\Utility\Unicode::mimeHeaderEncode() in Drupal\simplenews\Mail\MailEntity->getFromFormatted()

Unicode::mimeHeaderEncode seems to be removed from D10.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

As I understand, removing dependencies from composer.json was the goal of this task and the MR seems to do that, so I will merge it to 2.1.x branch.

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

There was an invalid version constraint. It should be fixed now.

I can't comment on dependency removal, I was not able to reproduce it (I have composer v2.5.2), but maybe this fix helps with that?

Made a new release and "composer info drupal/media_directories 2.0.4 -a" doesn't complain about version constraints.

So I took a look at it and the problem was with keyup event, it triggered in all keyup events.

I commited a fix that checks event itself inside keyup handler. Seems to solve the issue and adding media also works.

Yeah, my patch probably wasn't correct.

I opened MR for the changes that should find media alt text and also default to media name in case alt is not set.

It doesn't fix the issue with overriding alt text during embedding. There we have an issue where entity_embed module doesn't pass other attributes to formatter and we don't have any information about customized alt text.

What is the status of D10? Does it need any testing or feedback?
Is it possible to release dev release with D10 compatibility?

I just ran similar steps on a clean D9 installation (enabled modules on admin UI). Seemed to work just fine. So I guess module itself is just fine.

Maybe its some kind of Drupal core thing? I have seen weird issues (configs not imported, modules not really enabled, etc) and they seem to happen when there are a bit more modules or there are traffic on site (for example enabling on production site).

If re-install is not an option, you could always add a hook_update_N to re-add the field.