At first I wrote this about images, but it applies to all media bundles; video and files too.
Much better!
On my M2 Mac the install took 12min (plus the 4mins that I didn't realize that it was asking me a question about which recipes to install).
I'll close this up.
I ran into this but it was only happening on the live site (Pantheon, but I don't think that matters). If I cleared the cache, some of the domains worked but not others. If I cleared the cache again, then a different set of domains started working. I attempted to deploy the attached patch to get more info. But simply the act of deploying seemed to kick it into gear and it started working for all domains.
Ack, we just hit this problem again. Just like @mqanneh the site in question is using v3.0 and patch #12.
I think we'll have to uninstall the module until this is resolved.
Also adding attribution.
I attempted to give a few words of inspiration at the end.
I'm mentoring @joelstedman@gmail.com on their first contribution at Portland2024. We'll work on this issue for 1hr.
dalin → created an issue.
I'm mentoring @aronm on how to contribute to their first d.o issue.
I'm at Portland2024 with @tderego and @ptkoroma and we're going to test this merge request within the next hour.
@kopeboy We try to keep issues as small and self-contained as possible. I suggest creating a new issue.
I've updated the ticket description with what I think are the remaining tasks. Including a new one: Rewrite the tests to not use an .sql.gz file and remove it from the patch.
@mrshowerman
There are a few places in the codebase that could throw an exception. Here's some examples:
- https://git.drupalcode.org/project/entity_reference_purger/-/blob/1.0.x/...
- https://git.drupalcode.org/project/entity_reference_purger/-/blob/1.0.x/...
- https://git.drupalcode.org/project/entity_reference_purger/-/blob/1.0.x/...
If I brought this up in my IDE it would probably tell me there are even more uncaught exceptions.
Let's say that our code in hook_node_delete is running. We're processing the deletion of a node that is referenced by 50 other nodes. An exception is thrown when processing the first node. Currently code execution will stop, and the other 49 nodes won't be processed. What should we do instead? Maybe just skip it and continue on through the list of nodes.
@mrshowerman
I think this issue should stay open. I don't see any try/catch statements in the codebase.
We are migrating a D7 site to D10 that has 1000s of webforms. It is causing severe scalability issues where a full cache rebuild takes >1 min to complete. I was hoping to come here and find some tips. We'll report back if we find anything.
Hi @gnikolovski
Writing good issue descriptions takes time. Could you share an issue credit with me?
Upping to critical since the site basically goes down when this happens.
I'm not sure if this is a thing, but I've seen the error on two high traffic sites. We also run this module on smaller sites, and they haven't seen an issue.
One idea: Since the 4.x branch is largely around CKEditor 5, the new format, and the migration path, and since paste-from-word seems to have stalled out, could we move forward with a 4.0 release and add paste-from-word in some future release?
While this patch makes things better, I'm experiencing some significant UX issues:
1. Create an OL
2. Use Entity Embed button to insert a document
3. Try to add another LI. There's no way to put your cursor after the embedded document and press . The CK5 "type around" buttons are misleading since they only add soft line breaks (i.e. keep you within the current LI)
4. You might eventually figure out that you have to put an empty line break after the UL, and then click the OL toolbar button.
I don't know enough about the inner workings here, but I suspect that this should be a follow-up issue.
Since the related Drupal Core issue was critical (because it results in data loss when upgrading from CK4), this issue should also be.
Added info on how to Migrate from Video Embed WYSIWYG to Drupal Core Media.
dalin → created an issue.
Getting this on a different site too, though here thankfully it isn't an empty fieldset
IMO we should still show something for menu items that the user doesn't have access to. They should be included in the total count, but in the list we should show "access denied" or something similar.
I've discovered that this also includes a menu entity delete. Updating the description accordingly. I updated the remaining tasks.
IMO we don't need to do anything special if there are lots (a hundred?) direct children. The list will be long. So be it. But this will almost never happen in real life.
The code was added a few months ago in this commit:
https://git.drupalcode.org/project/smart_date/-/commit/011d5ddc3d3f4ccf1...
Which is from this issue:
https://www.drupal.org/project/smart_date/issues/3322456
🐛
The "Add another item" button does not work with a single click
Fixed
It sounds like the solution for that problem needs to be more specific.
Upping to Major since this is a major WTF for content editors.
Follow Mozilla's guidelines and support inline images with “data:image/*” unless it’s “data:image/svg+xml”.
I recommend we go the other way and use a whitelist of known safe mimetypes. Who knows what the future brings. Or if I intentionally mangle the format to be something like "data:image/_svg" can I get the web browser to still render this as an svg?
It's probably worthwhile to take a look and see what Gin's preprocess hook is doing. Is it important? Should we instead switch to a different (less likely to be overridden) implementation like maybe hook_process()?
@joaopauloc.dev
I like how you moved to ContentEntityDeleteForm.php so that this will apply to all content, not just nodes. But I'm not sure that getQuestion() is the right place for this. There's several returns in that function, and you've just added one more. The original MR used buildForm(), and that might be a better choice. Especially since we want to also use a list.
First, the warning message will be displayed only for a node that has a menu item as a parent? or should we warn users since the content has any kind of reference in menus?
Only warn when an entity is being deleted, and that entity has a menu item, and that menu item has child menu items.
In Drupal 11 the menu link is also deleted if the content is deleted. We need to update the message, I thought something like: This page has 3 menu children. These menu links will be deleted too if you continue.
Yes, the menu item being deleted is what causes the problem that this ticket is trying to solve. But are you saying that in D11 the _children_ menu items are also blindly deleted??? I can't get https://simplytest.me/ to build D11 so that I can confirm. But if that's the case, then that's a separate critical issue of content destruction. I'm guessing that's _not_ happening.
I've updated the ticket description to have clearer messaging. Can you take a look?
Setting to "Needs Work" because
* the latest patch seems to have lost the tests from #7
* the latest patch doesn't apply on the 1.x branch, but should probably just go on the 2.x branch anyway.
Upping to "normal" because the module is not doing what it says that it's going to do.
dalin → created an issue.
dalin → created an issue.
Just noting since it's possibly related: When I ran
composer create-project fourkitchens/sous-drupal-project [PROJECT-NAME]
Then I too see the big warning in that you shouldn’t use https://deb.nodesource.com/setup_X to install Node.js anymore. but a little further after that there's an error without any info:
Unpacking nodejs (16.20.2-deb-1nodesource1) ...
Setting up nodejs (16.20.2-deb-1nodesource1) ...
ERROR ==>
And then it continues on with some Lando stuff.
If the work on this ticket fixes that, then great, otherwise let's spin up a new issue.
Also, this patch still needs a functional review.
Here's a workaround to ensure that we don't alter the same query twice, but figuring out the root of the problem is beyond my skillset.
The problem is that \Drupal\group\QueryAccess\EntityQueryAlter::doAlter() is run twice on the same query, for each entity in the View.
I don't think the MR can work yet. We still needs some changes mentioned in #7:
- remove the included files from these libraries in
jquery_once/lib - the files will then be at
/libraries/[library]sojquery_once.libraries.ymlwill need to be adjusted - add some documentation (a README.md?) pointing people to https://asset-packagist.org/ because you need to add a few snippets to your root
composer.json - bonus: add a
hook_requirements()to check if the library is where we expect.
Gave a better test query that also shows status and last login.
Why not require "npm-asset/jquery-once": "^2.2" rather than jQuery? It lists jQuery as a dependency, so it'll sort it out (especially if you have a specific version of jQuery required in your root composer.json).
Also, this commit would need to remove the included files from these libraries, and add some documentation pointing to https://asset-packagist.org/
@Utkarsh_33
This isn't about when menu items are deleted. It's about when nodes (or I guess other entities too) within the menu are deleted.
I spun this up on https://simplytest.me Mostly, this is great! Better than what I had mocked up as an interim back-end solution by just reordering some things. Here's some screenshots from using just Drupal Core:
If I switch the moderation widget to "text field" then it's okay, but the field is too big:
And if I adjust my screensize we get some problems with things bumping together, or blocking the title. It's not too terrible, but should be cleaned up before we commit this.
Code looks awesome. I don't have a chance to do functional testing right now. Once someone can do that, then this can be RTBC.
Thanks for picking this up @GuillaumeG ! This is going to be great. Just needs a few tweaks.
Before I found this d.o issue I did a mockup of a better state. I'm not sure how much overlap this has with what's already been done on this issue.
Another place this pops up:
1. Use getParentEntity() somewhere.
2. View a non-default revision of the node. `getParentEntity()` is always returning the default revision of the parent, so you see something broken.
If the parent is a node there's awkward ways to work around this by getting the route object. But if the parent is another paragraph, you're screwed.
Is this problem when using Core's support for WebP, and/or with the WebP module?
https://www.drupal.org/project/webp →
If you could cut a new release of the module, that'd be great! Unfortunately it's not possible to use a patch to make a module's composer.json file to be compatible with D10.
@johnle
> When applying a composer patch, it did not install the require preserve_change change module, likely due to it running the composer install on the module first, and then later on patching it with the composer changes.
Yes, that's a limitation with Composer Patches that can't be avoided.
https://docs.cweagans.net/composer-patches/troubleshooting/non-patchable...
But I wonder if you could get around that by using an issue fork (see the info at the top of this page) rather than a .patch file.
-
+++ b/entity_reference_purger.module @@ -9,8 +9,16 @@ use Drupal\Core\Entity\EntityInterface; +function preserve_changed_field_info_alter(array &$info): void {This should be `entity_reference_purger_field_info_alter()`
-
+++ b/entity_reference_purger.module @@ -9,8 +9,16 @@ use Drupal\Core\Entity\EntityInterface; + $info['changed']['class'] = PreservedChangedItem::class;What happens if both entity_reference_purger and preserve_changes modules are installed? Should ERP just require PC as a dependency?
-
+++ b/entity_reference_purger.module @@ -105,8 +113,7 @@ function entity_reference_purger_entity_delete(EntityInterface $entity) { + \Drupal::service('entity_reference_purger.process_entity_reference_items')->processentities($parent_entity, $field_name, $delta, $field_item);Should be `processEntities()`
-
+++ b/src/Plugin/QueueWorker/EntityReferencePurgerWorker.php @@ -64,12 +74,12 @@ class EntityReferencePurgerWorker extends QueueWorkerBase implements ContainerFa + $this->entityReferencePurgerService->processentities($parent_entity, $field_name, $delta, $field_item);Here too
-
+++ b/src/ProcessEntityReferenceItems.php @@ -0,0 +1,106 @@ + // Move the current revision last on the array so that it does not break + // the not stop revision from being able to stop saving due to no longer + // being the main revision active.This comment doesn't quite make sense. Maybe:
The current revision can't be deleted until the rest have, so move it to the end of the array.
-
+++ b/src/ProcessEntityReferenceItems.php @@ -0,0 +1,106 @@ + $terms = $translated_entity->get($field_name)->getValue(); + foreach ($terms as $key => $tid) { + if ($tid['target_id'] === $field_item->target_id) { + $translated_entity->get($field_name)->removeItem($key); + $translated_entity->setSyncing(TRUE); + $translated_entity->changed->preserve = TRUE; + $translated_entity->save(); + } + }This duplicated code should be moved into a protected method.
The error message comes from content_moderation_workspace_access() and a code comment says "Check if any revisions that are about to be published are in a non-default revision moderation state."
dalin → created an issue.
@MacSim,
Sometimes we talk about how Drupal Core should handle 80% of common use cases. What you're describing sounds like an edge case, so contrib is probably the better place for something like that to live.
No, not still desired. If you are doing something that requires such high database performance, you are not going to use Drupal for.
Here's a screenshot of the out-of-the-box experience.
If the module were to only upload the files in the first step, then have one or more subsequent steps to add the field data, that would also solve the alt-text problem that ✨ Provide a way to add alt text when doing bulk upload Needs review tries to fix. It would also solve the more general use case of required fields on media items.
Actually, I think it's better to keep this issue just about catching exceptions, and providing a useful error message in Watchdog. Fixing the race condition should be in 🐛 Race condition when using queue Needs review which is a better place to start since it has tests.
If the deltas can/will get out of order from what we initially expected, why work with deltas at all? Shouldn't we remove the ID regardless of its delta? We could use
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...
I posted a similar comment on 🐛 Race condition when using queue Needs review
Setting to critical since this results in unexpected data loss.
Question about the patch: why skip if the entity ID is no longer is in the expected position? Shouldn't we search for the expected ID and remove it regardless of its position? We could use
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...
Well since other people are going to look at this, I'll clean up the formatting.