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?
@gausarts reverted libraries change and removed references to the old function. There might be other things to adjust to fix the original issue you were fixing?
Bumped ics-parser to ^3.4 and removed reflection
Sounds great, thanks!
Looks like this was added in 3691c830: "Fixed for unwanted asnavfor clone styling."
I think I figured it out:
var go = function (el, source, dest) {
// This gets captured in scope, so isn't recalculated on each click which means that the current index is always zero.
var index = instance.Components.Controller.getIndex();
$.on(el, 'click.' + NICK, function () {
// If it's moved here instead, it's recalculated on each click and seems to work as intended.
var index = instance.Components.Controller.getIndex();
if (index === source) {
instance.go(dest);
}
});
};
That being said, it looks like this block of code is forcing rewind behavior. I didn't notice because I had rewind enabled in my optionset, but if I turn off rewind this code is still forcing it. Rewind is handled by the Splide library itself, so if that behavior is desired it can be enabled without this code.
@gausarts any thoughts?
@ravi did you change base in splide.libraries.yml to point to js/src/splide.base.js and clear caches after commenting out that line?
KarlShea → created an issue.
Lol yeah I don't know how I totally missed that
KarlShea → created an issue.
Rebased MR!11 on top of the work in 🐛 Computed field should specify the cardinality of the field Fixed
@roxflame I think cardinality was supposed to go in your plugin's getFieldCardinality(), not in getFieldDefinitionSettings
KarlShea → made their first commit to this issue’s fork.
Looks like this interacts with the cardinality patch, some of the media formatters look at the field storage definition cardinality in their isApplicable method.
When that patch is in this will also need to be added to ComputedField::getFieldStorageDefinition():
// Set cardinality
$definition->setCardinality($plugin->getFieldCardinality());
Would definitely appreciate a look at this, seems to have fixed the entity_reference issue for me!
Looks like ComputedFieldSettingsTrait is also conflating storage settings vs field settings as well which I don't think is hurting anything but I'm also not sure it's helping anything.
Nope, it needs those settings there too.
Looks like ComputedFieldSettingsTrait is also conflating storage settings vs field settings as well which I don't think is hurting anything but I'm also not sure it's helping anything.
Drupal\computed_field\Field\FieldStorageDefinition might need a way of talking to the computed field plugin to get field storage settings? Or possibly getFieldStorageDefinition() in the ComputedField entity might do it? hook_entity_bundle_field_info_alter can't help because all you can do is set the settings on the storage definition which is just recreated each time it's accessed.
Yes, looks like it, e.g. EntityReferenceItem::storageSettingsSummary.
$storage_definition is \Drupal\computed_field\Field\FieldStorageDefinition, which in its create() is initializing the defaults for the field type, which is "node" target_type. It doesn't seems like there's any way to alter that either.
I wonder if returning target_type from getFieldDefinitionSettings() is a red herring? It looks like EntityReferenceItem from core is returning target_type from defaultStorageSettings(), maybe there needs to be a way of influencing that from within a computed field plugin?
KarlShea → created an issue.
I didn't know about entity_print_chrome until your comment, and I only found out about Weasyprint sort of by accident the other day and was pleased to find a patch to enable it in entity_print. Its output is significantly better than several of the other PDF engines I've tried!
Rebased to get CI working. Just tried and it worked great!
KarlShea → made their first commit to this issue’s fork.
When I test with a header containing a trailing space I'm not getting any errors:
-
name: title
label: 'Title'
selector: 'title '
Your selector just needs to match what's in the header exactly.
Thank you!
Contacted all maintainers with the contact form enabled: @gcb, @levelos, and @tauno
KarlShea → created an issue.
KarlShea → created an issue.
For at least the menu_links migration that seems to be due to the process settings here:
link/uri:
plugin: link_uri
source:
- path
Passing path like that will create an array, the correct settings would be:
link/uri:
plugin: link_uri
source: path
(Search web/core for plugin: link_uri to see other examples)
The same also seems to be true for the other migrations, however since they seem to be meant more of an example they can't actually be run (or the setup module even enabled—it depends on non-exportable preexisting content e.g. a content block uuid in the database).
With the v4 API patch, enough changes to the example modules to get them enabled, and changing the process setting above I was able to get the menu_links to run without any errors.
KarlShea → created an issue.
Setting back to RTBC since there were no changes and the CI tests passing.
I'm not sure what the process is for removing BC changes (e.g. the route provider check in MenuTreeStorage and test) but that check is still in the 11.x MR.
Created merge requests for 11.x and 10.2.x from patch at #38.
If you look at my last commit I made exactly the same mistake except in the other argument lol
Confirming #31, I only applied the patch from #27 and did not change any Twig or check any render arrays.
I don't think a new alter hook is needed, I just think there's a bug here.
The documentation for $context['source'] on hook_pathauto_alias_alter() states "This can be altered by reference." That is true, altering there will save an alias with a different source. However, re-saves of the same entity will create multiple aliases because the source hasn't been altered when checking for an existing alias.
Moving that documentation to hook_pathauto_pattern_alter() and passing $source by reference to that hook works through multiple saves.
KarlShea → made their first commit to this issue’s fork.
No, it's been brought up multiple times on both Slack and in the CKEditor issue queue and no one seems very concerned.
FYI the render|striptags|trim check may not work correctly if the block renders using a lazy builder, there was an issue in the Gin theme where I ran into the same problem.
Patch #27 is also working for me.
#38 works on 10.2.2 with a taxonomy menu for the first set of terms, but does not work for nested terms.
Rebased issue branch, addressed some comments, renamed commerce_product_variation token to current_variation, added default_variation token.
Works great!
KarlShea → created an issue.
Two issues I can see:
- Constructor signature changes for
WorkflowStateandWorkflowTransition. - I'm not sure whether/how to surface the state description anywhere,
OrderListBuilderdoesn't really seem to be used since that's normally a view so there would have to be something done to add a title attribute to that cell.
This would be very helpful, I needed to add a custom transition to our commerce orders so shipping emails didn't get sent and it would be very helpful to have some description text to remind the users which transition sends the emails and which doesn't.
I actually don't think this will work either, since Element::isEmpty is just checking for child array elements, which will always exist.
Not sure if this is the right way to do this check, but the tasks are showing for me now.
Figured out the actual cause: that block is sometimes a lazy builder, so using striptags on the region won't work since there's no text content until it's rendered later.
Stock Gin:
Removed check:
KarlShea → created an issue.
Property visibility fixed in https://github.com/u01jmg3/ics-parser/issues/329.
The patch could be changed to not use reflection, but it would require the patched version of ics-parser (whenever it's released) to be installed.
Patch attached that uses reflection, it's not a great solution but it's working for me.
This used to work, but ics-parser moved any custom properties to a private $additionalProperties array, making it impossible to get that data into the Feeds data transfer object.
See https://github.com/u01jmg3/ics-parser/issues/318 and https://github.com/u01jmg3/ics-parser/issues/329
My client is also asking for this.
That worked, thank you!
Setting image.style doesn't appear to change anything, it's image.uri that does it.