@bkosborne It will work. Just look at the MR. It updated the URL to be https://github.com/fengyuanchen/cropperjs/archive/refs/tags/v1.6.2.zip
If you download that file and look inside the dist folder, it has all the files that the code references. So what about it would not work?
Ah, I see that's true for version 1.6.2 of the library. But the zip file for version 2.1.0 doesn't include the dist folder. Shouldn't we just go right for version 2.x of the library? I guess we could do that in a follow up.
I don't think we can make the composer.libraries.json file work with the new library because the github artifacts don't include the compiled JS and CSS files anymore.
Wouldn't it be safer to refuse to log the user in rather than manipulate the username like this?
Actually I think we need to get 🐛 js_cookie module not enabled when updating from CAS 2.x to 3.x Active resolved first.
We've been using 3.0.0-beta4 and 3.0.0-beta1 of CAS Attributes in production for many months without any issues. I think this is ready to make stable. What do you think claudiu?
Agreed this should be supported, especially since MS says it's the recommended and more secure approach. That said, this patch should be converted to a merge request for easier review and collaboration. From the screenshot, looks like the Key module support isn't used either but it should be.
I don't think we need two issues for this, right? Looks like this 8.x branch isn't used anymore. Closing in favor of ✨ Microsoft identity platform application authentication certificate credentials Active
Agreed. Setting this to major as it can be really confusing to people.
The affected code has been refactored it seems, and I cannot find any instance of bitwise operator in the codebase anymore.
Agreed this is confusing. Two bits of feedback: The patch should be converted to a merge request, and the patch should not have commented out code in it (just remove the code)
The MR isn't adding the email as a candidate, it's setting the fallback to use the email.
Agreed with #4. What's the specific use case?
I encountered an issue where just rendering a view on the same page as a separate entity query (not related to the view at all) is causing the entity query to ignore trash status, leading to deleted entities being returned. I think this is because post_render is too late to switch trash back active. My entity query is built in between the time the view is built and the view is post_rendered. Basically the post_render hook only works if we're 100% sure the view is constructed and rendered immediately with nothing in between.
I suppose that using hook_views_post_execute to re-activate trash is not appropriate because there may be renderable components of the view that need to have trash ignored too (e.g., does the view need to load entities from entity storage during its rendering process).
Overall I think this feature is a bit too risky though. I'm personally going to patch it out in my project for now, as I'm not sure how to fix this in a safe way. Disabling trash entirely for a view has a lot of rippling effects on it where you may not want trash disabled too.
Okay, I found the seemingly magical and amazing method \Drupal\views\Plugin\views\Query\QueryPluginBase::getEntityTableInfo() which should simplify our logic even further. This method promises to deliver all of the entity tables used on the views query, which is exactly what we need to ensure we're only dealing with entity tables.
Can you guys check this MR resolves the issue? I tested this with draggable views and it works as expected now. I also tested this with a view based on an revisions table, and I tested on a view that joins on the same entity type.
So if we can avoid a new major version (which brings its own headaches), but just ensure that the cropping continues to work even if you don't have the JS installed in the new, expected spot -- a CDN fallback seems like a fine approach, IMHO -- I'd be comfortable merging it.
Oh, and the module already has a CDN fallback. So this MR will load the newer library from the CDN if it's not locally installed in the libraries folder.
Yeah looks like the simplified views query alter logic introduced in 🐛 Views relationships are missing deleted check, allowing deleted content to show in views listings Active is the problem. The code loops through all the relationships (joins) on the query, tries to determine if the table being joined is for an trash-enabled entity type, and if so, adds the deleted IS NULL condition to it. The problem is that it's not correctly assessing the join table is for an entity or not.
bkosborne → made their first commit to this issue’s fork.
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!