I'm very confused with the branches here. There is both a 3.x and a 3.0.x branch. The 3.0.x branch is behind 3.x but 3.0.x is the default branch that was chosen when I created the issue fork. Which branch is "main"?
In any case, I created a new branch from 3.x with the changes.
bkosborne → changed the visibility of the branch 3569488-vimeo-providers-regex to hidden.
MR submitted. I noticed the unit tests for ProviderUrlParseTest were already failing, so I fixed those here too. Note that as a result of relaxing the regex, it now allows through some previously invalid inputs for time index like this:
https://vimeo.com/193517656#t= (notice missing time)
But that seems totally fine. The time is still not parsed. We just accept the URL and still parse the vimeo ID.
The patch doesn't appear to work for the input example provided in the original report: https://player.vimeo.com/video/607242810?h=47f19305fd&autoplay=0
This is because the main regex that's used for validating if the Vimeo provider can parse the input doesn't allow query string parameters. This was mentioned in #6 and it wasn't completely addressed. The regex in getIdFromInput needs to be updated to remove the $ at the end.
And yes, the patch should be converted to an MR.
Yes IMO this should 100% be a release blocker
Yes IMO this should 100% be a release blocker
I'm a bit puzzled on the best place to catch this exception and bubble it up as a form error. This seems complicated because the exception occurs when the entity autocomplete field tries to auto-create a contributor entity that doesn't exist yet, and the input is invalid.
I think the simplest method may be to add an #element_validate to the contributor field in a form_alter and manually validate the label of the contributor by passing it thru the name parser and check for an exception. Kind of clunky but not sure there's a better way?
I do wonder a bit how @bkosborne ended up with so many entries in the fpm APCu cache.
I mentioned this in the issue summary, but we have ~600 websites all sharing the same APCu allocation - it's a multisite setup.
I think godot's point makes sense, but want to note the kernel sends the installed list through c3c32 first.
Are you referring to the composer class loader prefix that DrupalKernel sets?
I have no idea why crc32 was used for that, but that also seems inappropriate. @alexpott introduced it many years ago → but no discussion on why that algorithm was used. My understanding is crc32 is for integrity checking.
There is an additional concern though that not only are the cache keys very long, there will be entries in the file cache for the variations based on modules installed. Might be worth a follow up to see if there's a way to garbage collect.
Hmm yeah. APUc at least has garbage collection built-in by setting a global ttl for entries that lack one (like ours). I have this set to 7200, so if an item is not accessed after 2 hours it will be considered invalid and can be purged by the garbage collector.
Thanks for the quick review! Applied that change.
I'd like to get input from @catch and/or @alexpott on why this caching is even necessary for hook discovery? The discovered hooks are written to the container which is then cached on its own. I guess the benefit then only becomes we don't need to redo hook discovery when the container is rebuilt? But how often is the container being rebuilt where you wouldn't also need to do rediscover hooks too? (e.g., new or removed extension?)
Oops, didn't mean to do that, sorry. BTW I chose base system since that's what the related issue used.
Just want to say big +1 to this. I think this is a huge improvement.
Okay introduced hashing, though I think I'd prefer to remove the cache entirely.
I think sha256 is fine here? It's slower than something like xxh3 but I think that's OK here since this is a compiler pass not frequently run. Collision avoidance is the most important here.
This causes a huge increase in APCu usage for multi-site setups that use lots of modules. I created 📌 APCu FileCache can consume a ton of memory due to caching procedural hook data since Drupal 11.1 Active to address it.
bkosborne → created an issue. See original summary → .
Note that #40 is not right. Changing the deployment identifier DOES resolve problems like this. The composer class loader APCu prefix passed thru Settings::getApcuPrefix which adds the deployment identifier.
You mentioned that changing the deployment identifier resolves the issue for you, right? That's likely because the deployment identifier is used in the cache key prefix for things that utilize the APCu cache (like composer's class map) and various FileCache usages (things that scan files on disk). This in effect causes a cache bust of the APCu cache entries (without clearing out everything that's in there).
You should be changing the deployment_identifier when updating the code. I agree it's not well known or documented: #3153335: Document $settings['deployment_identifier'] (and that it fixes moved class autoloader caching) → .
I don't think having a Drupal cache clear also clear the APCu cache would be wise. That would negatively affect multisite installations as they all use APCu. If one site cleared the APCu cache, it would in effect clear it for all sites.
Added a test (same one from Datetime's test). Ready for review.
I think the default timezone should not be in the info data at all since it gets cached. A similar issue was found in the (much more widely used) Datetime element in [#13398554] and it was addressed there by pulling it out of the info data. Let's do the same here. I submitted a quick MR to see what fails, but I think we can add a test for it too.
bkosborne → made their first commit to this issue’s fork.
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
bkosborne → created an issue.
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?