A CDN fallback alone wouldn't be enough though? We'd need to ensure the JS that this module uses is compatible with BOTH cropper libraries. That need would be clearer if #3305831: [PP-1] JavaScript broken when using cropper.js v1.5.12 → was merged in with this one. Neither issue can be committed alone, they are both essentially the same issue and need to be considered together.
It looks like the JS changes needed in this module aren't that extensive. I suppose it's possible to add some conditionals in there to detect what type of cropper library is loaded if we want to maintain backwards compatibility.
...but I still think the best approach is a new major version that drops support for the unsupported version of cropper being used. Keep the current version marked supported for 6 months or so to give people time to update to the new one and get the new cropper JS library downloaded.
Either way, I suggest we close #3305831: [PP-1] JavaScript broken when using cropper.js v1.5.12 → as a duplicate, update the issue credits in this issue to include people from over there, and update this MR with that work.
My original approach didn't work under all situations. This new approach is even simpler and works reliably because we weren't removing and re-adding every component in the section, only inline blocks, so the order could still be incorrect. New approach sets the weight back to its original value after the component is re-added to the section.
Note the failing test is cpsell and unrelated to the work here.
Also bumping to major as this is a really big annoying issue for those using layout builder.
Okay, I MR !3 is updated and has tests passing.
What's needed to move this forward and get it reviewed and committed? The issue has been open for 8 years. We've been using this patch in production for years.
If it helps, I can put my hat in to be a co-maintainer of this sub-module. We do rely on it heavily at this point.
Confirmed this works well when paired with #3305831: JavaScript broken when using cropper.js v1.5.12 → . Can a maintainer please commit both of these issues?
Confirmed this works well. This should be committed together with 💬 The Cropper library referenced in the README and in the libraries info is deprecated Needs work . They're really the same issue but both are needed.
I merged the latest 8.x-3.x into this to get it up to date. Lots of conflicts. I did my best to get this set right.
I'm also going to open a new 4.0.x branch of this repo with the goal of including this big refactor into that, while also switching to semantic versioning at the same time.
Yeah this would be a nice feature. Instead of sending the email immediately, queue the changes, and then send a single batch email. However the entire module would really need see a major refactoring for that to occur. A big challenge will be that the message body is currently given the token/twig context of the single entity that was updated.
This is a really old issue and I'm sure you've moved on by now, but I think a solution here could be to just create two separate notification entities, isn't it? Then you don't need to use the alter hook.
Agreed, better to use a proper solution for all mail.
I was also confused by this. Thanks for starting the work on this. I took it further in an MR and will add you to issue credits. Thanks!
bkosborne → made their first commit to this issue’s fork.
Closing in favor of ✨ Provide full to/cc/bcc email configuration Needs work
Yes, we need clear steps to reproduce here. But the code in the module today should be sending an email to the OWNER of the entity (the form should be updated to make this clearer, I agree). It adds them to the BCC header. Part of the confusion here could be that the email is never sent if the user has "disable the site email address" clicked, in which case the TO header is empty and many mail exchanges will reject emails without a TO header. That will be resolved in ✨ Provide full to/cc/bcc email configuration Needs work
bkosborne → made their first commit to this issue’s fork.
I closed the Group support issue due to the added complexity and bloat it brings to the module. I think the same is true here, though this is at least a simpler implementation. This is a pretty narrow use case that I don't think would benefit the majority of users of the module.
What might make more sense here is to ensure that this module has the API necessary for other custom modules or contrib modules to extend it and add this type of functionality.
I cleaned it up a bit and added some stuff that was recently added into the README.txt file. Thanks all.
bkosborne → made their first commit to this issue’s fork.
Looks like the commits were made into the main 8.x-3.x branch of the fork and I'm having a really hard time merging upstream into it. I'm going to close this issue and create a new one, fix the issue there, and transfer contribution credits.
bkosborne → changed the visibility of the branch 8.x-3.x to hidden.
bkosborne → changed the visibility of the branch 3328951-fix-the-issues to hidden.
bkosborne → changed the visibility of the branch 3328951-attempt2 to hidden.
bkosborne → made their first commit to this issue’s fork.
We can get this resolved in ✨ Provide full to/cc/bcc email configuration Needs work
I'm not sure I really follow here. You have some other module that's using an alter hook to add attachments to the email, and the attachment you add there is added multiple times to the email? I'm confused why your code would address this problem. It's removing all attachments from the email array. Wouldn't that prevent anything from being attached?
I thought about this and I think it makes sense to make the change. Can the patch be converted to a merge request so tests run? We should also add tests for this scenario.
I agree this would be helpful. Let's postpone this on ✨ Provide full to/cc/bcc email configuration Needs work though, since getting that one in is very important and will change how this one is implemented. We can convert the patch to an MR at that point, too.
The steps to reproduce in #5 are very simple but I cannot reproduce them. It's possible this was fixed elsewhere or there are more specifics about the setup that aren't known. But it's been a few years, I'm not sure smustgrave is still able to help with that?
Bumping to major. I'm not sure if it makes sense to implement this yet OR to just go all in on ✨ Provide full to/cc/bcc email configuration Needs work . Working on the other issue is a better long term approach I think.
Can the issue summary be updated to include more detail and steps to reproduce? Also lets use a merge request for patches instead of patch files so tests are run.
Can this patch be converted to an MR so the tests run? It would also be good to get a test for this.
I don't think a permission is appropriate for this. This should be an option in the notification config entity. I'm also not sure I agree with the implementation here. Why would we skip a notification if the owner if the content matches the current user? The notifications may be important to send out to other people. I think it would make more sense to send the email but remove the current user's email address from the list of recipients. Overall I think this just needs more thought.
This was fixed in 📌 Drupal 11 compatibility Active . I'll update contribution credit in that issue.
Let's handle this in ✨ Provide full to/cc/bcc email configuration Needs work
Since Twig support was already added, the next step here would be to split out more of this work into additional separate issues to make them easier to review and merge in.
Note that there's another issue for adding support for more control of the To, Cc, Bcc, etc. headers: ✨ Provide full to/cc/bcc email configuration Needs work . I haven't compared the implementation details of that issue with this one yet but I plan to.
The other functionality of this (abort support, debugging, tips, etc.) should all be split out into separate issues.
I see Content Moderation Notifications as serving a simpler use case. This patch adds some complexity, and it also assumes that the workflow entity is a node, which is not always the case. For these more advanced use cases, I think the ECA module would be a better fit. Please feel free to re-open if anyone disagrees here. I recently became a co-maintainer and I'm trying to clean up the issue queue and keep the module streamlines to make it more maintainable.
Could this please be converted to a merge request so the tests run? And we may need to add a new test here too.
Hmmm, why not just use the adhoc emails field and enter their email addresses?
Or if that's not suitable, I'd rather see this made more generic. Don't limit the list of users to select based on the roles selected. Just have an autocomplete tag field to list usernames on the site to receive the email. The current patch is way too complex and lacks testing too.
See also ✨ Allow a notification to bypass the content access check Active where this was requested. Adding contrib credit to that patch author.
Sorry I missed this older issue. I created ✨ Don't validate that adhoc emails have access to view content Needs review for this same issue and committed it. In that case, I didn't add the configuration option, I just removed the access check for adhoc emails entirely. I think that's what 99% people expect for adhoc emails. I'll close this out as a duplicate but will update the contribution record in that issue to add khaled.zaidan.
Thank you for turning this feature into a contrib module.
This module is already pretty bloated with the number of options it presents users on who to notify. This would bloat it further. It's a slippery slope adding support for other contrib solutions in here. At some point it makes more sense to use a module like ECA which probably provides the flexibility needed, or use the new contrib module created in #52. I'm conflicted closing this because it's clear a lot of people want this functionality, but at the same time no maintainer has committed the work in many years and no tests have been added.
Thanks. Dup of 🐛 PHP 8.4 - nullable is deprecated warning Fixed but I'll give you an issue credit for it over there.
Thanks all. Some of those twig vars are incorrect though. I updated them before committing.
bkosborne → made their first commit to this issue’s fork.
Yes this certainly seems like it's the same issue as #2949891: Doesn't work on multilingual entities → . Closing in favor of that one which just has more activity. but the solution here maybe is relevant over too.
For those that need this, please follow ✨ Add support for other entity type tokens Active where it's being worked on.
Yea, this makes sense. I updated merged 3.x into the MR and fixed it a bit. It was assuming that there's a set of tokens based on the entity type name, but that's not true. For example, there's no tokens for "taxonomy_term", but they exist under "term". We need to use the EntityTokenMapper service from Token to get the mapping.
bkosborne → made their first commit to this issue’s fork.
No response in months - we do need some clear steps to reproduce.
#13 is unrelated to the problem presented here. #13 is about how "To" can end up empty and create invalid emails. This issue is about how no "Reply-To" header causes issues.
Yes, I think that what was suggested in #2 is the way to go. The fact that the /node/{nid}/latest route doesn't return anything if there's not a forward revisions is a bit annoying, but I think beyond the scope of what this module can address.
I'm also not able to reproduce, and I think the original reporter has moved on. Please comment if that's not the case.
Thanks Ryan. I'm going to close this out as Drupal 9 is no longer supported (despite the module saying it's compatible). But at least others will be able to search for this and include the patch in their projects if needed.
I think it's safe to close it at this point
I don't understand the desire for on-demand views created for this. We already have a view for displaying all entities in the trash. Why can't we just add bulk operations to it?
Looks like this made D10 sites unable to restore trashed content: 🐛 Latest release 3.0.19 breaks restore for Drupal 10 sites Needs work
Note we can remove both the lazy designation and the service tag once we drop D10 support. Uninstall validators can be autoconfigured as of 📌 Use a tagged service iterator for uninstall validators instead of individual lazy proxies Needs review (D11 only). I added a todo for that.
Okay, I fixed this, but I'm not really sure if this will break something else. I couldn't figure out why some of the original code was written as it was. But this update is very explicit about checking if the deleted condition exists for the specific table alias now. The existing views test passes and the new one I added passes, but there wasn't much test coverage here for edge cases. I didn't test this with views based on revisions for example, though I think it should be fine?
Okay I narrowed down the problem area. The views query alter already handles adding deleted conditions for relationships, but there's a logic issue if the relationship is to the same base table (same entity type). The query alter never adds the .deleted IS NULL condition because it thinks it was already added.
The problem is in the hasDeleteCondition code. It incorrectly thinks a delete condition already exists on the relationship because it's seeing that the delete condition was added for the main base table of the view. This method needs to be updated to check for the specific alias of the relationship I think.
I created an MR with what should be a failing test. It includes a view that demonstrates the problem.
I don't really know how to fix this problem yet though.
Note also that in my test I had to manually re-enable trash after executing a view. The Trash module is supposed to do this automatically in the a post render hook for views, but the tests never render a view, so the hook isn't called. This may lead to some hard to reproduce and diagnose bugs on sites that manually execute views without rendering their output. I'll create a follow up for that.
rob holmes granted me access. Thank you!
Okay, submitted an MR that ditches BCC entirely and just puts all the recipients in the "To" address.
I like this approach as it's very SIMPLE. Since this could be considered a breaking change, if we commit this it should go in a new feature release, if not new major. I suggest we go with this and then can turn our attention to ✨ Provide full to/cc/bcc email configuration Needs work to provide more robust handling of the from/to/bcc headers.
My bad, this is very much still a problem. The latest version of the module still allows a situation where the "To" address is empty, leading to all of these issues. I marked #3208996: "Disable site mail" feature not compatible with PHP Mailer library → as a duplicate of this.
Also I agree that the work done in ✨ Provide full to/cc/bcc email configuration Needs work is probably the best path forward, but it's also the most complex to implement.
I think the best short term solution here is to just stop trying to put the notifications in BCC. Just append them all to the "to". The downside here is that everyone will see everyone else's emails, but I suspect that 95% of the people using the module don't mind that and may even prefer it. It's helpful context to know who else got notified.
Bumping this to major.
This has not been addressed elsewhere. However, it is indeed a duplicate of 🐛 "Disable the site email address" breaks the workflow even using Adhoc recipients Needs work , so closing in favor of that and I'll re-open that issue.
I agree this is probably the best approach to resolving the issues, but the scope of this is very large, and we haven't heard from any of the active module maintainers. I'm willing to help do this, but we need buy-in from a maintainer first because it will be a lot of work to update the tests. Marking as needs review for that reason.
MR submitted that just removes the access check for adhoc emails. I'll fix the tests too, but before I spend time on that I'd like to get maintainer buy-in on this path. Thank you.
Since core has been fixed for ~3 weeks (see 📌 Remove skip procedural for modules implementing hook_hook_info Active ). Adjusting the title and criticality of this issue, since as Berdir mentiomed, this is really a task to prepare for D12.
bkosborne → created an issue.
Looks like this was already done in this commit which was released in 1.1.0.
bkosborne → changed the visibility of the branch project-update-bot-only to hidden.
Dup of 📌 Automated Drupal 11 compatibility fixes for bibcite_pubmed Needs review
You should click the "plain diff" link next to the merge request link (top of the issue on this page). It generates a git patch file. You should copy the contents of that into a plain text file in your project and include it with composer patches. Looks like it's already D11 compatible.
The thing holding up this issue is the lack of tests.