🇺🇸United States @japerry

Received validation that it works in Site Studio, fixed!

In theory we can write a better delete test that will replicate this issue, lets get that test in and then work to make it pass.

While a cursory look at the code looks ok, it'd be good to get validation here that the issue found with site studio is fixed.

japerry → made their first commit to this issue’s fork.

Ahh yes both aren't supposed to have the download key in them. fixed!

japerry → made their first commit to this issue’s fork.

The form is disabled due to the asset_id, version_id, and external_id not being created on new entities. However, when editing an existing one (say, adding a new translation), these values should be there in a disabled format if you enable the field under form display.

Therefore, its likely this warning only needs to be shown when asset_id is null. Otherwise, allow saving the form. To ensure this is okay, the MR would also need a test showing that when you add a new translation, that both the new translation and original media item get updates when the widen asset is updated.

A separate issue should probably be made to support translations in general, which likely do not work out off the box with widen: https://community.acquia.com/acquiadam/s/article/Can-I-translate-web-pag...
This issue does compound that problem, since there is no mapping of translated metadata fields. But since it doesn't work right now, thats a problem for another day.

Huh interesting find! Indeed adding the version/external id speeds up the call. I presume since its otherwise trying to fetch them. Fixed.

japerry → made their first commit to this issue’s fork.

Ahh good catch. Fixed

Looks good! Fixed.

Looks good for now. Committed!

japerry → made their first commit to this issue’s fork.

japerry → made their first commit to this issue’s fork.

\Drupal\Core\Config\ConfigFactory::doGet will invoke \Drupal\Core\Config\ConfigFactory::loadOverrides over each with instances of \Drupal\Core\Config\ConfigFactoryOverrideInterface

When config factory service is created \Drupal\Core\Config\ConfigFactory::addOverride adds each override, cannot guarantee they are re-used

There is a bug here, but the proposed fix is not it. Will look into it further later.

japerry → made their first commit to this issue’s fork.

Talked to rlhawk about 4.0 vs 2.0. With d7 support dropped, we're going to use the 2.0.0 version as the next major. So these deprecation messages are correct.

Fixed! All green across the test board!

japerry → made their first commit to this issue’s fork.

Isn't there a way for modern drush commands to work alongside the old drush versions? Some sites have constraints keeping them on earlier versions of drush, so I'm not sure such a core module like key should be moving away from supporting those quite yet.

japerry → made their first commit to this issue’s fork.

Thanks for keeping the Drupal 10 and 9 discussion separate. For 8.x-1.x, 9.1 is ok for now. I also updated the format so its easier to parse in drupalci testing. While there used to be benefits of doing the <12 format (to test betas before release) the lenient composer and drupalci issues make core_version_requirement: '^9.1 || ^10 || ^11' a better format now.

There is a lot of code here, not sure this makes sense to do in the 8.x-1.x version.

This issue highlights a broken architectural decision, where the underlying setKeyValue method, which IS a string, has a hard-coded override injected into it where it diverts to a plugin if the multivalue setting is enabled. Not great.

However, neither the patch or the MR would work in the 8.x-1.x branch. Key's own tests show that setKeyValue is supposed to accept an array. See https://git.drupalcode.org/project/key/-/blob/8.x-1.x/tests/src/Kernel/K...

So for 8.x-1.x, yes, this does need to accept both a string and array because.. reasons. Its not great but it is needed. But https://git.drupalcode.org/project/key/-/blob/8.x-1.x/src/Entity/Key.php... isn't sufficient, as you could end up calling this method and NOT have multivalue set, then you're storing an array when the actual value ($this->keyValue) can only accept a string.

Personally, I'd rip out the plugin definition logic and and just json_encode on array, removing the deferral to the plugin. (Especially as both MultiValueKeyType and AuthenticationMultiValueKeyType 'serialize' method just calls json_encode anyway)

Done! Fixed.

japerry → made their first commit to this issue’s fork.

smustgrave → credited japerry → .

It does appear the code in AssetUpdateChecker is not needed. Fixed!

japerry → made their first commit to this issue’s fork.

Good find. Fixed!

japerry → made their first commit to this issue’s fork.

Love it! Fixed.

japerry → made their first commit to this issue’s fork.

Made you a co-maintainer. Love the idea of doing a 2.x branch for that part. The 1.x can be legacy and then drop support after Drupal 12 comes out. :-D

Done!

After discussion with mglaman, the original cause of this is fixed in #3524222. Any change to the signature here is a bandaid over the underlying issue, which is $version_id, if set, should actually be a string and not null.

getAsset has had that method signature since the beginning of Acquia DAM, with the problem being this internal method returning an invalid version ID. Any methods calling getAsset should ensure that $version_id is either omitted or is a string.

Will mark the other issue "Closed, works as designed".

japerry → created an issue.

No, the problem is that
$asset = $this->clientFactory->getSiteClient()->getAsset($asset_id, $version_id); is passing an NULL, which it shouldn't be doing -- but it appears that public function getFinalizedVersion(string $asset_id): ?string { is returning NULL or string, which it shouldn't be doing -- it should always return a string.

However, changing that response from NULL|String could cause other issues, so the easiest fix is just change line Asset.php:234 to check null coalesce $asset = $this->clientFactory->getSiteClient()->getAsset($asset_id, $version_id ?? '');

Looks good! Committed.

I'm going to need more detail here. The existing parameter 'version_id' is already optional, so it should not be throwing an error. Is there a backtrace showing where omission of the version_id parameter within getAsset is throwing an error vs using NULL?

japerry → made their first commit to this issue’s fork.

I have a feeling that there is a bug here in how we're returning the class type, but not sure how to reproduce it yet.

Its possible this could be a bug? but without other reports or ways to reproduce, can't really do much with it.

japerry → made their first commit to this issue’s fork.

Looks good. Fixed.

japerry → made their first commit to this issue’s fork.

japerry → created an issue.

Thanks for the test fixes! committed.

Verified locally that indeed a test is failing: https://git.drupalcode.org/issue/media_acquiadam-3515234/-/jobs/5062463

Failed asserting that the text of the element identified by '[data-drupal-selector="edit-media-label"] li:nth-child(2)' equals 'Acquia DAM Image → Media: Acquia DAM Image'.
Failed asserting that two strings are equal.
Expected :'Acquia DAM Image → Media: Acquia DAM Image'
Actual :'Acquia DAM Video → Media: Acquia DAM Image'

My guess is that the order is different on older versions of Drupal? nevertheless, we should have this fixed before committing.

Question--why make a new project and not just make this feature a sub module of key?

Thanks for fixing the test! Fixed.

Looked at this a little more, a few points:

1) Assets should only exist in one media type at a time. This is a one-to-one relationship between DAM and Drupal, and I'm not sure it'd be a good idea to try to change that.

2) You can get the functionality you're looking for in one media type by using the 'Download/Sync' option. Embed codes are part of the Entity View display field formatter, so there isn't a reason why you cannot enable either a new display with the embed code as the field display, or changing the download/sync field to disabled.

For those reasons, marking this as 'won't fix'

The premise of this issue was to restrict users from seeing the Acquia DAM authorize button on their page, not to change access on the admin page. MR148 addresses the OP's issue.

The media types that ship with Acquia DAM are fairly generic, so say you have a site with multiple image types due to permissions, download/sync, or another edge case that requires per media type delineation. You may then want to change the media type labels to reflect what they're used for, including the default one.

Looks good! Committed.

Need to continue reviewing code, but marking NW due to the following:
Enable Acquia DAM and Media: Acquia DAM
1) Add a few items to the Media: Acquia DAM image type
2) Add a few items to the Acquia DAM image type
3) Run the migration tool

You'll notice that it suggests deleting the Acquia DAM: Image type, which contains existing data! Also, the media type labels are greyed out. The ones that come with Acquia DAM should be editable alongside the old Media: Acquia DAM types so you can get a full picture of the all the media types and their names.

Yah, so what needs to occur here is requiring Acquia Dam 1.1 and adding an update hook. We want all customers using media_acquiadam to automatically be also using Acquia Dam, hence the requirement during install (and why we need an update hook as well)

It was intentional to require Acquia DAM for all versions of Media Acquia DAM going forward. However, we do need to add an update hook, and now that 1.1 is released, we can require that version (1.1.1 actually)

Is there a failure occurring for existing installs due to the hard dependency? The migration tool already fails gracefully and shows users an error if acquia_dam isn't installed.

Agreed, adding these errors would break the expected API, thus moving to the next major version.

Talked to rlhawk at Drupalcon about versioning, and its likely a new version is not likely anytime soon due to the downstream dependencies. Thus moving this back.

As of Q2 2025, the minimum version can be safely set to Drupal 9.5 without needing to change the major version.

There needs to be some update hook to ensure users with existing 'administer site configuration' get access to dam, otherwise existing admins may lose access to DAM.

Looks good! Committed.

japerry → made their first commit to this issue’s fork.

Looks good! Pushed to 1.1

japerry → changed the visibility of the branch 3515288-create-media-displays to hidden.

Awesome looks good. Merged.

greggles → credited japerry → .

MR139 overcomplicates the addition of the media library. Honestly, the swaping fields from 1.1.0 is also a little too complex and could also be simplified, but not sure thats needed for this issue.

The MR below is a much simpler approach. You -almost- could put this in the asset and not need it in the derivative plugin, but that'd commit us to never needing a different media library field formatter for different types.

https://git.drupalcode.org/project/acquia_dam/-/merge_requests/146

japerry → made their first commit to this issue’s fork.

Marking fixed since this is in 1.1 now.

Not sure why it wasn't caught (since its wrapped in a try/catch... ) -- however I don't see a downside to adding this check. .so fixed!

japerry → made their first commit to this issue’s fork.

I think this is related.. 🐛 New Media Type with Identical Source as Existing Acquia DAM Media Type Incorrectly Links to Existing Media Type Active

For our use-case, we want to show both the assets, from embedded URL as well as Drupal file system, on the same website hence creating different media types. Both the media types should have its own functionalities.

Each media type has their own source instance with configuration. However, I -think- I see the issue, I'm guessing the same asset ID cannot be put in two different Media Types... which wasn't really a use case we thought about.

Vipin, I'd try seeing if you can get the same Asset to exist in two different media types.

Tests passed, fixed!

While this could be a good enhancement, I feel its more a minor feature enhancement rather than a bug. When making other media types, you're expected to go change these fields, so this isn't much different.

Pending the test outcome, this should be good to go. This patch fixes a few issues:
* infinite loop when adding more than 10 items
* missing image style when going from download/sync to embed (and image style isn't allowed)
* mapping of 'original' / undefined styles.

While the DENY flag makes sense, I'm not sure why we'd otherwise default to ALLOW. Looking at other deny policies, usually NULL will be returned instead.

Also, adding a service needs a post_update hook to flush the service container cache, otherwise the event won't fire.

There is now a clarifying comment, but also closing this as it works as designed.

japerry → made their first commit to this issue’s fork.

japerry → made their first commit to this issue’s fork.