Okay, good point. I think it is valid to add the presentation role to make it very obvious about the intent. But I was confused about the issue title and description indicating that alt="" is sometimes not added. If that's happening, I don't think it's an issue with this module. I'll take a look at this MR.
On a clean d11 install using standard profile, I enabled and configured the module for the image field on the Article content type. When I upload an image and mark it as decorative, and view the page, I see that there is an empty "alt" attribute rendered as expected.
You mentioned an image may render without the alt attribute. How? What did you do to reproduce this?
The role=presentation seems to be optional and not all screen readers respect it. In the WAI example page you provided, the examples don't appear to use BOTH an empty alt and a presentation role. Why would we do both?
MR looking better - left more feedback though. Also the code changes should follow Drupal's coding standards. There are missing docblocks, inconsistent indentation, etc.
Indeed, I misunderstood, sorry about that.
I took a brief look at the failing test and it's related to the added cache context for the user authenticated role. It wasn't a simple/obvious fix. I may get to it this coming week.
ah. Thanks. CAn you update the merge request instead? Having patch files uploaded in addition to the merge requests can cause confusion.
Maybe. This one is tricky. JS errors can sometimes cause issues with other JS behaviors. It could be a blocker for some people in some situations. For us it doesn't seem to matter.
Submitted an MR with the short term fix to restore caching behavior that Drupal 10 had, which is that max-age=0 is set for authenticated users. To restate: D10 added max-age=0 to entity forms for authenticated users. D11 doesn't do that anymore. Without that, caching bugs in Webform are revealed (missing cache contexts).
Follow up 🐛 Additional changes needed to fix caching issues in Drupal 11 Active created
How can we safely remove the max-age=0 on entity forms without knowing what all the various custom entity forms in the wild are doing with cache metdata? Catch laid out a bunch of ways this could fail.
Oh, I see 📌 Deprecate not giving cacheability metadata to forms Needs work . Can this issue be closed in favor of it?
Adding 🐛 Additional changes needed to fix caching issues in Drupal 11 Active as a blocker
Okay, I was conflating this a bit with 🐛 Form build caching is broken in Drupal 11 Active which has other circumstances that are leading to cache issues.
First I think we need to define what "cacheable" means here. I think for the purpose of this issue it means that the form doesn't bubble up a max-age of 0. That would make it uncacheble by Dynamic Page Cache, and it would also make it uncacheable by your CDN if you were using the Cache Control Override module which exposes that max-age to the cache header response (Drupal doesn't do this normally).
I agree that prior to core issue 📌 Form tokens are now rendered lazily, allow forms to opt in to be cacheable Needs review , anonymous visits to a webform were cacheable and didn't have max-age=0. Authenticated visits wouldn't have been cacheable (by dynamic page cache) because the CSRF form token got added and had a hardcoded max-age=0 which bubbled up. After that core issue, the webform submission form has a forced max-age=0.
This seems like it's a feature request, not a bug?
In any case, I'm confused. Webforms ARE currently being cached in D11 (which has led to the bug reported in 🐛 Form build caching is broken in Drupal 11 Active ).
Core is only marking the form as uncacheable (max-age = 0) if the form doesn't already have a max-age set.
But webform submissions forms DO have a max-age set as I described in this comment. 🐛 Form build caching is broken in Drupal 11 Active
I looked into exactly why this broke in D11. It is due to the change introduced in
📌
Form tokens are now rendered lazily, allow forms to opt in to be cacheable
Needs review
as suggested in #4. Prior to that change, Drupal's FormBuilder forced all forms have a max-age of 0 (ignoring whatever max-age the form's cacheable data indicated), which prevented Dynamic Page Cache from caching them. After this change, Drupal only forced a max-age of 0 on the form if it didn't already provide its own value for max-age.
Webform does provide its own value for max-age. It sets it to -1 (which indicates "cache permanent"). Webform doesn't do this directly, but in WebformSubmissionForm::buildForm() it has this:
// Add the webform as a cacheable dependency.
$this->renderer->addCacheableDependency($form, $webform);
The Webform entity has a max-age of -1 (I think all config entities do by default).
So the core change suddenly started making webform submissions forms cacheable by Dynamic Page Cache. This wouldn't normally be a problem if the proper cache contexts were also set on the form, but Webform was missing them.
The fix committed in this issue does address the problem, but it doesn't do it completely right. It's only adding the needed cache contexts for webform submission forms loaded via the WebformEntityController::addForm method. Forms shown via a block don't use this code path. So the fix should be implemented instead in the WebformSubmissionForm class instead.
Since this issue was marked closed as fixed, I'll create a follow up for it.
I also verified this. I tried to debug it but didn't get anywhere meaningful. jQuery UI was not updated in Drupal 11, 11.1, or 11.2, so I don't think it's related to jQuery UI changes. It doesn't appear to affect form building in my case.
I imported your form into a local site and walked thru the various pages and submitted it successfully with no issues (note I did have the make the birth date field non-readonly, for some reason it is set that way in your form).
The message you are receiving "a.isFunction is not a function" is almost certainly related to the jQuery 3 to jQuery 4 upgrade in Drupal 11. "isFunction" was removed from jQuery 4. There is no JavaScript in the Webform module that calls "isFunction". You likely have a contrib or custom module on your site that is calling this. Search your codebase for "isFunction" to find the culprit.
I combed through the complete issue queue and I think there's two additional issues that Jacob identified as possible stable blockers. I added them to the related issues list. I also removed one of the issues that is no longer considered a blocker.
I'm not able to reproduce this. Can you please add steps to reproduce on a fresh/clean Drupal 11 site? That's what I tested with, using the latest code from the Webform 6.3.x branch. I edited the existing Contact webform to add a document file field. Then as an anonymous user I submitted the form file a file upload successfully, then submitted it again with no issue. Yes, a session cookie is added after the first upload, but that had no negative impact in my case.
Also note that the current MR is not a valid approach. We cannot just clear out the user's session cookie. I don't doubt that removing the cookie resolved the problem for you, but the root cause is not likely the cookie but something else in the users session on the site you tested with.
RE: #18, I'm a bit confused by your comment, as you're indicating the problem is fixed for you after upgrading to 6.3.x-dev? If that's true then this issue could be closed out as there's nothing to actually fix anymore?
This would have been caught earlier when 🐛 Cannot save unpublished versions of published content with Layout Builder Active was fixed, but the test that was added never tested the scenario where book module was actually enabled for the content type. I updated the test to resolve that.
I submitted MRs for the 2.0.x and 3.0.x branches.
This looks good to me. I merged in the latest from 11.x and fixed a couple minor deprecation issues in the test.
And even just display the workflow state name that will be transitioned to when there's only one choice, but don't output it as a field, just plain text.
Marked #3336702: If user doesn't have access to scheduled state, they are unable to edit the scheduled content → as a duplicate of this.
This is essentially a duplicate of #3274274: Make transition check configurable → . I'll update issue title over there to make it more findable. I think this issue is older but that one has some MR active so makes sense to keep that open.
I think this is a essentially a duplicate of ✨ Allow manual publish before future publish date Active
This has been open for years without any attention from original author, but I think this is essentially a duplicate of #3414656: Scheduler CRON fails when the moderation state is already in the target state → and #3390085: Scheduler-process hooks incorrectly handle failures →
Agreed this is useful. At the very least, a validation error should be shown when someone tries to manually set the state of an entity to the same state that is already published. Right now it just seems to ignore it.
Alternative approach here: Why not just hide the publish or unpublish state fields entirely if there's only one choice? For simple editorial workflows, there will only ever be one choice. Then if a published date is submitted, the module automatically sets publish_state to that single choice.
I think this is essentially a duplicate of #3414656: Scheduler CRON fails when the moderation state is already in the target state → .
So if I understand correctly, the UI already prevents someone from creating this scenario where the scheduled publish or unpublish state would become invalid.
However, we agree that there could be ways that the current state is changed to something that would make the scheduled state invalid by other means not using the normal node UI.
In this case, the problem is then that the publish or unpublish state value gets cleared out and SCMI indicates there was a problem by returning -1. And then it creates an endless loop of revisions being created as SCMI is never able to put the entity in a valid state transition again since it's never valid to transition from null state to something.
That all right? If so I can update the issue summary.
So the maintainers made it clear that it should not be possible to save an entity with a scheduled date without also setting the publish state. The use case outlined in #17 seems to imply they were able to do this though. But, how?
I think the issue summary needs to be cleaned up here to better indicate with exactly the issue is. Because if we are to assume it should NOT be possible to save an entity with a publish date but no state, then the issue title and summary should instead be updated to indicate that the problem is that the error logging is currently not sufficient.
I read this a few times but I'm still not clear on the issue. Can the issue summary be updated with more detail and steps to reproduce? Also it sounds like this would be classified as a bug?
Clarifying the title.
Note the issue in #3 is unrelated to the original report.
I agree that it should not be possible to save a node in an invalid state like this. The module does automatically set the field form widgets when its installed, but I guess that can either not work 100% of the time or someone accidentally reverts it without realizing.
Clarifying the title to indicate that the scope of this issue is related specifically to the view permission of the paragraph and that the issue is present both with forward drafts from content moderation or when reverting host entities (page layouts) to previous revisions. Edit/delete permission should be resolved by this core issue that's hopefully committed soon: 🐛 Layout builder fails to assign inline block access dependencies for the overrides section storage on entities with pending revisions Needs work .
This seems like a duplicate of 🐛 Paragraph access check using incorrect revision of its parent, leading to issues editing and viewing paragraphs when content moderation is involved. Needs work
Excited to see this ready to commit! I verified this resolves the following scenario:
- Content moderation enabled for Page content type
- Layout builder & layout overrides enabled for Page content type
- Content block with a paragraph field on it
- User creates a new Page node, edits layout, and adds the block with paragraph field on it. Saves as published.
- User edits layout and makes changes to the block & paragraph fields and saves a new draft (forward revision).
- User goes to edit layout again, they are not able to edit the paragraph fields at all. RESOLVED with this MR
Note there's another issue here where the paragraph is no longer viewable on the active/default revision of the node once the forward revision is created, but that is a separate issue specifically related to Paragraphs: 🐛 Paragraph access check using incorrect revision of its parent, leading to issues editing and viewing paragraphs when content moderation is involved. Needs work
These patches are not right. You need to be extra careful that the paragraphs on your sites don't have private file fields on them otherwise you risk anyone in the world being able to download those files. Maybe for 99% of you that's not the case, but I can't imagine this patch ever getting committed for that reason.
@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.