Hey @cilefen, thanks for the updates to the issue, and great question. Wanted to first mention that I was only testing this within my project which was on 10.3, so I was certain the issue was there, but I do suspect this is also on 11.x as well.
Yeah, so I had the exact same thought and referenced that test you mentioned as I was trying to figure out what was going on. The best thought I can come up with is that this test is logging a user into Drupal (specifically with "access content" perm which is needed for the `/node` view), whereas my test is just accessing anonymously. I also thought that maybe my test would need to assign the "access content" permission, but I would have expected to see a 403 error if that was the case... instead I get a 200 status with the default /user/login page, so I don't think it's a permission error either. Maybe the logged in user approach is affecting the cache being served, maybe? I think that's probably the right path, but I don't have an exact answer... b/c that ::initFrontPage() method is requesting the homepage as anonymous, and my test is then doing the same thing. Whereas in this existing core test, the setup is still requesting the homepage as anonymous, but the 2nd request to the homepage is with a logged in user.
Did you have a chance to view the test I put up? Would you agree that what I'm doing in that test should be working?
Thanks for the background. Sorry for the title, changing it here. I had a hard time following what the ::updateCachedData() method was doing, I was thinking that maybe it was updating like the list of custom fields and that type of meta data. But it sounds like that's not the case?
This is marked as a major bug with over 6 years of no updates. Should the tagging of this issue be changed at all?
I actually also believe that this is a problem. I am sort of of the opinion that if page_cache cannot handle the Vary header, then maybe it simply should not cache pages that have that header set...? I don't think that the code sending the page, for example a controller, should need to change anything... that is likely sending the correct response already.
I think I saw it's relatively recently introduced in core, and I haven't dove deep into it yet, but couldnt page cache also return the reason why something is not cacheable (just by page_cache).
Here's the CR →
I saw. So maybe it reads X-Drupal-Cache: UNCACHEABLE (Vary cacheability) or something like that?
Sorry @bbrala - this is nearly the same thing you are suggesting as well... but I do think that issue #2430335 would do most of what this issue is trying to do from what I can see.
From #4...
Regrettably this approach clearly violates the ResponsePolicyInterface contract. While not stated explicitly in the documentation, the check() method is certainly not expected to actually change the response.
I don't think this is resolved with the current approaches still, as suggested in #26.
I have solved this in my own project with an event subscriber for KernelEvents::RESPONSE. I think that would be a relatively simple change to make here. Would that address the concerns of #4?
This MR was already committed... I think the status be changed to Fixed? But now this module just needs an updated release.
Assuming this should be \Drupal::config('bynder.settings')->get('cache_lifetime')? I'm unclear what this code is doing though. Is this maybe a cached version of what the custom metaproperties are?
I'm using the patch from #10 on my site. I actually really like the way this works, image attached. Moving to RTBC.
Believe this was opened for one of my projects. We ultimately discovered that there was hook that was hiding this field. I think this can be closed...
Patch form #5 is also working for me... I'm going to set this to RTBC.
I see a lot of talk here and in the linked issues about supporting multiple WEBP modules. Isn't that functionality in core now? I know I'm using the core webp "image_convert" plugin, and this patch is fixing it for me, and is relatively straight forward as well, that's why I chose to use this patch over some of the other open issues.
Ran into this same issue today and was going to create the issue, so I'm +1 on this one. Overall I find this module confusing in that, it provides a media entity source of Bynder, but also provides (in a strange config manner) a single bundle for that config.
Marking this as fixed by https://www.drupal.org/project/memory_limit_policy/issues/3448480 📌 Investigate config validation Active
Just revisiting this one, and my comment in #13... I suspect that for my issue, this was somehow related to using `advagg` module, and when Drupal made the aggregation changes in 10.1, I believe, we stopped using advagg and I have not seen the issue again since we switched.
Also huge thanks to @fjgarlin who helped me out in the Portland General Contribution room. Apologies I should have given that credit as well!
Assigning partial credit to jrockowitz who put up test changes(fixes) in https://www.drupal.org/project/config_override/issues/3023903 ✨ Allow config override directory to be stored outside of Drupal. Needs review that I used to get tests to pass in the new Drupal CI.
This test is not reflective of how the configuration behaves in the real word, because the configuration should never end up like this if it was saved via \Drupal\language\LanguageNegotiator::saveConfiguration
I'm not sure what this is supposed to mean, @larowlan? I have run into the same issue described in this IS, and it seems a few others on this thread have as well... is that not the real world evidence? Why should there be a configuration, language.types, that is NOT overridable, the way every other config in Drupal core is...? How could a developer possibly know that for this one special config, you can't override it, and instead have to call the LanguageNegotiator::saveConfiguration()?
I'm not sure what the opposition to introducing a fix here that allows you to set this with configuration overrides would be? "It wasnt intended to do that" seems like a very poor reason...
I agree with larowlan, that this use case is not covering a real world scenario. It does not use the correct methods. Until now LanguageNegotiator was not intended to be configured by direct configuration changes or in settings.php.
So a user of Drupal is only allowed to override some configuration...?
I don't think that just changing the comment as suggested in the most recent patch/MR actually solves the bug that was described in the issue summary.
I agree completely.
Thanks @sleitner.
Woohoo, thanks @longwave!
I want to set this to "Needs review", maybe even RTBC... I don't know why, but I can't set one of these patches to test against the 2.x branch, so it shows that it fails to apply to 1.x. I am using the 1.x version of the patch in my project, and it is solving this issue for me... I'm curious if this task has some actionable steps now to document?
Yep, I'm with you @capservoogt. This has been a very difficult issue for us to watch Acquia leave us up the creek without a paddle. I am trying to use what code we already have and patch something together for D10 with a plan to move off this tool as soon as is feasible.
I've actually recently run into this issue, and I have some more details that affected me here I wanted to share...
The fix here still solves my problem, and I can explain why as well.
So I ran into this issue with the following scenario...
* I am installing a site that uses a custom cron job to make a call out to an RSS feed, download the file, and store it locally for processing by the feeds module.
* Drupal core runs through the INSTALL steps... but the install_configure_form step actually hardens the simpletest site directory, and sets the folder to 555 (not writeable).
* Drupal core at the end of the installation then triggers a cron. My job calls out to the internet. Drupal core has a custom testing middleware that intercepts that calls, and tries to add testing headers to the call, and ultimately calls drupal_generate_test_ua(), which tries to write the .htkey file to a hardened sites directory, and that fails... so my first cron run fails silently.
I actually came across this b/c by enabling the "locale" module, the call to drupal_generate_test_ua() was made earlier in the stack, before the hardening, and the .htkey file gets placed successfully, and there is no more failure.
The last 2 comments are saying the opposite... could we maybe get more elaboration on #103? I'm using this patch with Claro and it is resolving this issue, so I'm going to put to RTBC, but with the caveat that I'm not testing in Gin and 103/104 are saying it's working/it's not.
What do you mean by that, @smustgrave?
I came across this other core issue regarding these same links, and I think a solution should probably consider both of these issues...?
Interested in this as well... seems related to 2990549, although not exactly the same thing.
When reading through this issue, I think this should ONLY be tagged for branch 9.5.x, which is about to (3 months) be marked unsupported, potentially killing this issue. But yeah, I don't think this is relevant to 10.x or 11.x at all, since they are using the new 2.x asm89/stack-cors library. Does anyone disagree with that?
I'm going to bump this RTBC to get some more eyes on this. I have been using this allowedOriginsPatterns in production for the better part of a year, with a note in my codebase that this is undocumented in Drupal core. I think this should go into core to let people know it's an available option for them.
@nterbogt Could you see if this problem still exists on the latest version of the 2.x branch?
Thanks all. Sorry this took so long to get in, I have a really terrible excuse I won't share here out of embarrassment.
Patch applied to the dev branch, will look at creating a new release soon.
Thanks for your contribution, sorry for the delay, but I hadn't noticed this issue created before now. I am seeing the issue here that this was deprecated in 9 (so still working), but is now breaking in D10.
Confirming this issue is fixed in the latest `dev-5.x` branch. Thank you all!
I'm still seeing failures with this commit. It looks to me like there's an indentation issue in the YML file. I would recommend setting up a very basic automated test in the module that enables a test module that has this config present, and then you should be able to see the schema errors for yourself.
I actually am running into a very similar issue, but I don't know how to recreate. My production site will occasionally have a page that does not load javascript... It has 1 aggregated file that's supposed to be there, but instead my page prints: <script type="text/javascript" src="/"></script> which points to the homepage, and is HTML and not js, so I have no scripts on my page. I do use the advagg module, so not sure if that's related...?
I have also encountered this error (or similar) a few times on my production site. I haven't been able to determine if it's related to this module, though, but I am using this module, still on Drupal 9.
On my site, however, I am seeing that Drupal is loading a script at <script type="text/javascript" src="/"></script> (the homepage).
Just want to say thank you @Berdir and all involved!
A BIG issue I'm seeing here is that `drupal/media_acquiadam` requires `cweagans/php-webdam-client`, which is archived (abandoned), and requires `guzzlehttp/guzzle` < 7.0, but `drupal/core:^10` requires `guzzlehttp/guzzle` ~7.5.0.
The error I get is something along the lines of lockr.lockr_secret._____:info.wrapping_key missing schema. In the code in your screenshot, you are setting `info` to a sequence, but then not defining that sequence. I don't think you want to define this key as a sequence, but instead as a mapping, with the corresponding keys (wrapping_key) then defined.
Did you get the file that I sent over to Chris T? That has the changes that are working for our team, but I don't think you want the patch exactly.
Hey Wim - I see this has been up here for 6+ months, but I hadn't noticed it until now, apologies. I'd be happy to help test out a proposed fix, but I see that this is failing for maybe a PHPUnit rule to make sure that \Drupal\Core namespace does not get called from \Drupal\Components...? Given that rule, a fix like this wouldn't be able to go in, would it? If you'd like, I can still try this fix against my project's problem and see if that solves the issue if it helps move this along...?
My team is currently using a custom version of the work-around Alex suggested, and that has been working for us.
@heddn - they manage externally at https://github.com/pantheon-systems/search_api_pantheon - does that help at all?
This did the fix the issue for my team. Isn't the `skip_schema_check` reference coming from here...?
https://git.drupalcode.org/project/search_api_solr/-/blob/4.x/config/sch...
Suggesting this issue gets closed...? There is an RC release out now for this module...
I came here looking for a roadmap for a stable release, and one that's covered under the Drupal security policy...?
Given the last 3 comments, changing this to RTBC status. My team is using this patch as well and found that it fixed the issue we were facing.
For me, this is an automated testing only issue. When you run PHPUnit tests, part of the testing procedure ensures that all config you are importing has a valid schema, and since the schema being provided by this module is being reported as invalid, when using this module as part of an automated test, the test always fails.
So as an example, if I have a my_module.info.yml, and in my dependencies of the module, I list search_api_pantheon, and then I have a Functional test in my module, my test will fail 100% of the time.
So to reproduce, you can probably just add a FunctionalTest to your project, and that should start to fail.
I also don't think you need all of what is included in the latest patch here. The PR that's open on the Github.com repo (github.com/pantheon-systems/search_api_pantheon/pull/169/files) actually solves the problem for me.
Definitely +1 on this issue and following what happens here...
It's a bit challenging to test this with changes needed to the composer.json file, right? You can't apply a patch for this before composer runs...? How are folks testing this out?
My team is also "trapped on Acquia DAM Classic" (to borrow a phrase from a previous comment). We have been advised by Acquia that there will be no more releases for this 1.x branch, even for Drupal 10 compatibility. I'm not entirely sure if that would extend to reviewing and merging a patch request such as this, but just wanted to give the folks who came here for this a heads up.
If Acquia will not review this approach and move it forward, I'm wondering what folks see as the option here to use this module on Drupal 10?
Can this be reviewed for Lockr 5.x if the same config objects are supported in that branch?
This patch (not exactly) is already applied to the `5.x` dev branch, but there's no release tied to that branch yet. I have been using 5.x in a lower environment, and everything is working as expected for me.
This patch now no longer works for me...
The recent commit to this project added a schema, but it does not appear to be correct. This commit attempts to define every property of the solr connector, whereas the patch in #7 in this issue was extending the plugin.plugin_configuration.search_api_solr_connector.standard schema.
So now, when I run this project, I get the following error...
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for search_api.server.pantheon_solr8 with the following errors: search_api.server.pantheon_solr8:backend_config.connector_config.solr_version missing schema, search_api.server.pantheon_solr8:backend_config.connector_config.http_method missing schema, search_api.server.pantheon_solr8:backend_config.connector_config.jts missing schema, search_api.server.pantheon_solr8:backend_config.connector_config.solr_install_dir missing schema
In my opinion, the commit that was merged to the project (linked above) should be reverted, and the patch from #7 above should be committed in it's place.
I see that a schema was merged in as part of the v8.1.3 release, but it is not the same as the patch here, and the published fix does not resolve my schema errors, but the patch does.
I was running into this same issue, pulled in this patch, and now my tests are passing again. I think this should be added into this module! Thanks @heddn!
+1 for this patch, it solves the same problem I'm facing... Url's in the "From" field with a special character, like `path/to/a%20file.pdf` won't work.
This issue has been RTBC for almost a year now, is there anything left the maintainers need on this issue? I see that, it appears, the last requested changes by Berdir were completed, so I think this just needs eyes again from an maintainer.
This patch fixes a very narrow use case of a space being encoded to %20, but does not address all encoded characters. For example, a ( or ) get encoded to %28 and %29. That makes me think there should be a more global solution to handle this...?
It looks like this was a duplicate of #3322852, although maybe that issue should have been marked a duplicate of this one. Thank you for your contribution!
Are there any reproduction steps that can be provided here? How can I set up a test to see this fail for myself?
I did see that, but did not try it...
Ok so this is working for me now again. Sorry for all my confusion. I think the issue was the invalid tokens (using Gutenberg, it saves image style tokens to the source HTML) that were causing the styles to not generate.
Ugh, sorry... I thought this was fixed, but I still don't think it is. It was just working locally, but that was still b/c I was visiting the page in a browser, which creates the image styles.
Actually, I don't think this was an issue. My problem, I believe, was somehow (not sure yet how) my pages were referencing image styles with invalid tokens. Since I'm using a static site, I don't really care about the image style token, so I can shut that off in the image.settings configuration.
Seems like this should be removed to me... and a 6.x branch in pre-release seems like a great opportunity to do so. It could also probably be marked as deprecated in the 5.x branch to warn users ahead of a 6.x upgrade. Just my $0.02.
Any chance this can be backported to 5.x?