Works for me!
I also need a separate patch on top for the facets range summary to work, I'm not sure if that should go into the MR or not?
This is the patch for DefaultFacetsSummaryManager:
// @todo Keep non-facet related get params.
- $url = Url::fromUserInput($facets_summary->getFacetSource()->getPath());
+ $path = \Drupal::service('path.current')->getPath();
+ /** @var \Drupal\path_alias\AliasManager $pathAliasManager */
+ $pathAliasManager = \Drupal::service('path_alias.manager');
+ $path = $pathAliasManager->getAliasByPath($path);
+ try {
+ $url = Url::fromUserInput($path);
+ }
+ catch (InvalidArgumentException $e) {
+ $url = Url::fromUri($path);
+ }
Rebased !162 on 2.0.x. It doesn't apply cleanly to 2.0.9 right now because there's been a change to facets-views-ajax.js since that was tagged, but that's the only change right now.
Yes, that's why I rebased the MRs.
Rebased on 2.3.0/3.3.0, fixed phpcs/phpstan issues.
Also fixed a bunch of stuff from phpcs/phpstan. Will have to live with the deprecated interface warning, using the static methods didn't work when the tests ran (was throwing an error about a missing chained service).
karlshea → created an issue.
karlshea → created an issue.
Does the fix in the MR work for you?
karlshea → made their first commit to this issue’s fork.
Is node one of the available types of content in one of your groups? Tried clearing caches? Otherwise you might have to debug yourself or wait for someone running Group 3.x to chime in, I'm not sure.
You should be able to use [node:group] tokens. I'm still using Group 2.x (MR 157), if you're on 3.x (MR 156) the MR might be wrong, but I haven't been able to test it.
Sure! The project I'm using this on has 17 migrations so I'm not hitting rate limits, but I can take a look at whatever you've got!
Oh it was totally the missing context_mapping! I had to fix that on the actual site I was working on *facepalm*
MR created against 2.0.x with most of #95.
I'm not sure where some of the tests for Drupal 8 should be (if anywhere at all), so that didn't make it in.
There also seems to be an issue in testBookNavigationBlockWithTopLevelPageTitle. I'm not sure what's going on there, the patch works on my local copy but when the test runs it can't find the block by ID.
Seconding needing more page margins. Having the header menu run into the side of the window when it has rounded corners looks weird, and the text on the left of most of the block types feels way too close to the edge of the screen. Similar issues with the images on the right side of the blocks with rounded corners hitting the scrollbar.
@jonmcl yes, that's correct! My computed media field has exactly the same code.
karlshea → created an issue.
My production settings is setting $config['entity_print.print_engine.phpwkhtmltopdf']['settings']['binary_location'] because I needed to install it with pyenv.
Did a cache clear help?
Using same approach as mercury_editor in 🐛 Mercury Editor dialogs won't open after upgrading to Drupal core 10.3 Fixed
It's also broken Bootstrap Modal, looking for event.dialog where instead the fix is to look for $event.dialog.
The MR can't be fixed until it's targeting the right branch, that's why it shows hundreds of commits.
The MR needs to target 11.x, not 11.0.x. I rebased it on 11.x but I can't edit the MR.
KarlShea → made their first commit to this issue’s fork.
Lots of core stuff hitting me recently too!
That makes way more sense lol it's been a long day!
Do you mean $settings['blazies']?->set('unlazy', FALSE);? If I do that I get a "Undefined array key "blazies"" warning.
KarlShea → created an issue.
Updated the description as well to reflect the actual issue
Sort of figured it out, I think it's a race. If I set a breakpoint when that extend happens instance.options is totally empty. But when it's logged it's not empty so it's getting updated somewhere else as this is running.
Actually that isn't working either, I'm still not getting all of the properties of both coming through.
KarlShea → created an issue.
Ahhh right, commit pushed with a static table definition.
Cool. Doesn't apply to 10.3 now. Love these MR patches.
Rebased on 11.x, I added most of #145 except for removing image_path_flush() in Media, that will affect every type not just OEmbed.
KarlShea → made their first commit to this issue’s fork.
Might just be tired but I'm failing to understand what is going on/what the request is for the @todo referencing this issue in MenuAccessTest::testSystemAdminMenuBlockAccessCheck(), could someone take a look?
@quietone, @wells I was also confused by #72 but removed the fallback code and rebased both MRs.
Oh good idea! I tested this with sql_generate_invisible_primary_key enabled and it worked just fine.
/user/password?name[foo][0]=foobar
First request after cache clear:
HTTP/2 301
retry-after: 0
cache-control: max-age=86400
location: https://xxxx/user/password?name[foo][0]=foobar
accept-ranges: bytes
date: Mon, 24 Jun 2024 19:07:31 GMT
x-served-by: cache-chi-klot8100127-CHI
x-cache: HIT
x-cache-hits: 0
x-timer: S1719256052.788341,VS0,VE0
strict-transport-security: max-age=300
content-length: 0
HTTP/2 400
content-type: text/html; charset=UTF-8
x-drupal-dynamic-cache: UNCACHEABLE
content-language: en
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
expires: Sun, 19 Nov 1978 05:00:00 GMT
accept-ranges: bytes
date: Mon, 24 Jun 2024 19:07:32 GMT
x-served-by: cache-dfw-kdfw8210068-DFW, cache-chi-klot8100138-CHI
x-cache: MISS, MISS
x-cache-hits: 0, 0
x-timer: S1719256052.863280,VS0,VE366
cache-control: no-store, no-cache, must-revalidate, max-age=0
strict-transport-security: max-age=300
alt-svc: h3=":443";ma=86400,h3-29=":443";ma=86400,h3-27=":443";ma=86400
(This is not cached, hitting /user/password works fine)
/user/password?name[%23post_render][0]=passthru
First request after cache clear:
HTTP/2 301
content-type: text/html; charset=utf-8
location: https://xxxx/user/password
x-drupal-route-normalizer: 1
content-language: en
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
accept-ranges: bytes
age: 0
date: Mon, 24 Jun 2024 19:11:16 GMT
x-served-by: cache-dfw-kdfw8210117-DFW, cache-chi-klot8100046-CHI
x-cache: MISS, MISS
x-cache-hits: 0, 0
x-timer: S1719256276.102347,VS0,VE88
cache-control: no-store, no-cache, must-revalidate, max-age=0
strict-transport-security: max-age=300
alt-svc: h3=":443";ma=86400,h3-29=":443";ma=86400,h3-27=":443";ma=86400
content-length: 386
HTTP/2 400
content-type: text/html; charset=UTF-8
x-drupal-dynamic-cache: UNCACHEABLE
content-language: en
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
expires: Sun, 19 Nov 1978 05:00:00 GMT
accept-ranges: bytes
date: Mon, 24 Jun 2024 19:11:16 GMT
x-served-by: cache-dfw-kdfw8210044-DFW, cache-chi-klot8100046-CHI
x-cache: MISS, MISS
x-cache-hits: 0, 0
x-timer: S1719256276.201582,VS0,VE385
cache-control: no-store, no-cache, must-revalidate, max-age=0
strict-transport-security: max-age=300
alt-svc: h3=":443";ma=86400,h3-29=":443";ma=86400,h3-27=":443";ma=86400
Request to /user/password afterwards:
HTTP/2 400
content-type: text/html; charset=UTF-8
x-drupal-dynamic-cache: HIT
content-language: en
x-content-type-options: nosniff
x-frame-options: SAMEORIGIN
expires: Sun, 19 Nov 1978 05:00:00 GMT
accept-ranges: bytes
date: Mon, 24 Jun 2024 19:12:33 GMT
x-served-by: cache-dfw-kdfw8210044-DFW, cache-chi-klot8100170-CHI
x-cache: MISS, MISS
x-cache-hits: 0, 0
x-timer: S1719256353.239181,VS0,VE129
cache-control: no-store, no-cache, must-revalidate, max-age=0
strict-transport-security: max-age=300
alt-svc: h3=":443";ma=86400,h3-29=":443";ma=86400,h3-27=":443";ma=86400
Notice the Location header differences, that might be why one is getting cached.
I was able to reproduce behind Fastly, response is a 400 both from Fastly and hitting the app server directly.
Here's the maybe interesting part: /user/password?name[foo][0]=foobar does not trigger the page getting cached, but /user/password?name[%23post_render][0]=passthru does. Maybe because ?name[foo][0]=foobar doesn't redirect? The latter redirects back to the page without any query parameters so I wonder if the redirect is triggering the cache.
Response headers from Fastly when cached:
Accept-Ranges: bytes
Alt-Svc: h3=":443";ma=86400,h3-29=":443";ma=86400,h3-27=":443";ma=86400
Cache-Control: no-store, no-cache, must-revalidate, max-age=0
Content-Language: en
Content-Type: text/html; charset=UTF-8
Date: Mon, 24 Jun 2024 18:40:41 GMT
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Strict-Transport-Security: max-age=300
X-Cache: MISS, MISS
X-Cache-Hits: 0, 0
X-Content-Type-Options: nosniff
X-Drupal-Dynamic-Cache: HIT
X-Frame-Options: SAMEORIGIN
X-Served-By: cache-dfw-kdfw8210044-DFW, cache-chi-klot8100063-CHI
X-Timer: S1719254442.751705,VS0,VE95
Response headers directly from app server:
Cache-Control: must-revalidate, no-cache, private
Connection: keep-alive
Content-Language: en
Content-Type: text/html; charset=UTF-8
Date: Mon, 24 Jun 2024 18:40:19 GMT
Expires: Sun, 19 Nov 1978 05:00:00 GMT
Fastly-Drupal-Html: YES
Server: nginx
Surrogate-Control: no-store, content="BigPipe/1.0"
Surrogate-Key: eNqK igJ3 2tce mWTg juT3 F8oS PGx9 3F+3 grmK ojvY axoF eNI2 bpNa XtKW C6oq +QS4 pCAc
Transfer-Encoding: chunked
X-Content-Type-Options: nosniff
X-Drupal-Dynamic-Cache: HIT
X-Frame-Options: SAMEORIGIN
X-Generator: Drupal 10 (https://www.drupal.org)
I got this on a site behind Fastly with the fastly module enabled if that might help, and clearing the caches fixed it.
I won't be able to really try and reproduce the issue for a couple of days but when I'm back if I find anything else I'll update the issue.
Unfortunately the web user would need the SESSION_VARIABLES_ADMIN privilege to change the sql_generate_invisible_primary_key value.
Tested, it doesn't work. I don't think having sql_generate_invisible_primary_key enabled is compatible with what we have available to modify the schema in Drupal.
Some searching seems like it might work to do this inside a transaction? Untested MR created.
KarlShea → made their first commit to this issue’s fork.
Hmm my understanding is the only way to change a primary key is to drop it first and then recreate it. I asked in #contribute on Slack, let's see if anything comes from that.
Changes makes sense, thanks!
We did fork the project and added Media integration here: https://www.drupal.org/project/canto →
The devs at Canto aren't checking the issue queue and are also not replying to direct emails.
Thank you! It all appears to be working for me as well.
Do you have the cache lifetime implemented in your plugin? If you open a MR I can take a look, I've seen the same myself when hitting through the UI but so far no issues just running with Drush.
KarlShea → made their first commit to this issue’s fork.
KarlShea → created an issue.
@MegaChriz I added a test but I'm not sure that it's in the right spot, or that I'm putting a test for feeds_log outside of the submodule. I can make any changes here you'd like, just let me know the direction you're thinking.
When I remove the schema change for feeds_import_log_entry, the assertion fails checking for the log entry count, and when I remove the schema change for feeds_clean_list the test fails with an "Incorrect integer value for column" error.
Alright, let's see how it goes. Do you want to add me as a co-maintainer over there and I'll start taking a look?
I talked with my client and they think this whole thing is kind of clunky anyways and they're more in favor of just removing the integration altogether. But I would be open to co-maintaining to at least get it working, if we do get a D10/Media solution working I can keep it enabled on my client's site.
However I did ask on Slack and one of the Drupal moderators said that if you do create a fork don't use the trademark in the module's short name, they suggested "cdam" as the module name but the description can be "Canto DAM" in the title on the page: https://drupal.slack.com/archives/C1BMUQ9U6/p1715288857811109
Rebased!
I'm not sure that core issue is a good one for reference because it's talking about a case where the id should be an int for those entity types but isn't, e.g. the database column is an int but due to some circumstance a string is returned ("1" instead of 1).
However EntityInterface does not limit the id to an int, the typehint is string|int|null so the statement "an entity id should be an int" is not true for all Drupal Entity types.
This issue is trying to fix the case where the entity id is intentionally a string (and the database column backing it is a varchar). Think about a table of US states where the id is the two-letter acronym—we're not trying to coerce "1" to 1, but instead trying to store the value "NY" as an int which is impossible.
I don't think EntityInterface supporting this is widely known so it's been catching out some other module assumptions (like
Dynamic Entity Reference →
which has some weird release versions to fix the same issue).
Check out the EntityTestStringId content entity type in core which tests this, and the issue that created it:
#2205215: {comment} and {comment_entity_statistics} only support integer entity ids →
, or the issue also using that same type to test core's Entity Reference field type making the same assumption:
#2107249: Don't assume that content entities have numeric IDs in EntityReferenceItem →
.
Maybe that isn't true, I think you need to move 💬 Offering to maintain Canto Connector Active back to the project ownership queue to request moderation assistance. Or I can also create a co-maintainer offer issue for myself and try to move it through.
I don't think taking over the namespace will happen if the current maintainer has been making releases (see the recent 10.2 version).
@awm from previous experience working with DAM companies (including a company acquired by Canto), I don't think we're going to get any engagement from @flightdev. Take a look at what just happened with the 10.2 tag, they renamed the module machine name which will prevent upgrades.
I have a site that needs media entity integration, I'm thinking of forking the module and basing it off of this branch with any fixes from 10.2 pulled in. Any thoughts? Would you want to co-maintain?
KarlShea → created an issue.
Rerolled from patch #10 and added fix for date fields in the field formatter.
Created MRs for both 3.x and 2.x from the latest patches.
KarlShea → made their first commit to this issue’s fork.
Created MRs for both 3.x and 2.x from the latest patches.
Unfortunately this breaks min/max aggregation for datetime sorting.
Awesome, thank you so much! I'll give it a shot.
KarlShea → created an issue.
Ahhh if that was not configurable previously this all makes way more sense! Yes, that module has a UI for those settings.
The issue I'm hitting is that I changed background opacity in the Photoswipe module settings (/admin/config/media/photoswipe) to 0.6. So the $options array starts with bgOpacity of 0.6 from the blazy settings call. Then the array_merge overwrites that with bgOpacity 0.97 from a hard-coded list (and the same would be true of any configurable setting like animation duration, type, etc).
It is the case I could overwrite it again using the blazy_photoswipe_js_options alter hook, but why overwrite what the user has configured? I agree that it makes sense to have defaults in case they're not set up yet but I would have expected my site-wide Photoswipe settings to have also applied here.
KarlShea → created an issue.
It is definitely an adjustment, after working with the new MR flow awhile I feel like I understand what's going on better but it still feels pretty awkward how things are spread out all over.
The other thing I wish they'd make way easier is syncing the base branch into the issue branch, it's always a juggle getting origin/2.x and nnn-my-issue-branch/2.x up to date and rebasing everything.
#26 just means that it's the commit author, because I cherry-picked the squashed merge commit which had Ravi as the author. It doesn't influence issue credits.
This issue is showing up as credited on my user account, I think if you merge from the issue after picking users in the credits table that's all that matters now.
From what I've been reading, the commit author doesn't matter with regards to issue credits, that's why they removed the author radio buttons in the credits table.
I'm not really concerned about the issue credit, but if you're trying to figure it out I found this on slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1704365359171499?thread_ts=...
If you use the Merge button within the issue, it’ll. use the values set in the credit table, including the author.
Apparently it's a button within the issue below the credit table?
Unrelated to this issue, but after 10.1 Drupal can minify itself: https://www.drupal.org/node/3305725 →
Do you think it makes sense to remove the minifed JS from this module and let Drupal handle it?
@gausarts I'm not too worried about it, but yeah that does seem weird. Maybe merging the MR will automatically do it? I'm not sure.
Although you probably need to build the minified JS anyways so the MR flow probably doesn't work with this module that well?