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.
Looks good. Fixed.
japerry → made their first commit to this issue’s fork.
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.
Looks good! Pushed to 1.1
japerry → changed the visibility of the branch 3515288-create-media-displays to hidden.
Awesome looks good. Merged.
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.
Made a few modifications- looks good!
japerry → made their first commit to this issue’s fork.
Fair enough. Fixed!
I think you're onto something here with how the handle4xxerrors method is returning something different than expected, especially when throwDamException is expecting some 400 errors as well (which never get seen).. However, we probably don't want to throw out that code fully.
Fixes look good, Committed.
Good catch! Patch looks good to me, and passes tests. committed.
Made comments in the MR
Looks good! (also rebased against 1.1.. will commit when those tests pass as well.)
The new version of Acquia Dam → (1.1) should accomplish this functionality
Rebased and merged in! Fixed.
Tested well, looks good. Added an update hook for the new logger channel service and committed.
No because other things, like config imports would hit hook_insert, causing unintended issues.
The core issue is here:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/media...
The problem is that we already have a defined field for the source plugin, so this will never be null. Instead, we could look at doing what media library does, hooking into the the submit handler for entity type creation. When creating a new media type via config, its assumed they have a corresponding view config, we don't want to auto create it there.
See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/media...
With our config, we want to ensure only the embed code or download/sync field appear in the view display. If helper methods are needed, use AssetMediaSourceManager to store those methods.
This issue is no longer needed. any minor changes should be in their own issues.
A 'generic' file handler exists in 1.1. You can probably (?) use that one, but I noticed the ft key is different in your MR (compressed_archive) -- so if it can be re-rolled into the new plugin format, we could include it in a subsequent 1.1 release.
Fixed. I don't like it either, but eh its fine. Needs to be brought into 1.1.
Looks good! fixed.
I need to understand from the maintainers the reasoning behind the use of "final" on these classes, and what the correct path to allow for extending is in the minds of the maintainers.
Over time, we've found that customers will override classes throughout modules, if they can, which causes headache in the future when even simple changes are made. This requires new major versions to be created, which has a knock-on effect to downstream modules, making upgrading more difficult, especially custom code on customer sites.
That is why, except where we explicitly want to expose the API at a class level, we mark classes as Final. That way, when a customer request, like this one, comes in, we can make informed architectural decisions.
Luckily, as part of our download/sync work, we noticed that the hard coded derivatives were not sufficient. Instead of expanding their use within a single, non extensible class, a new plugin framework was introduced that allows us (and the original author of this post!) to add functionality without needing to change the signature of the underlying Asset and AssetDeriver classes.
Asset Media Source Plugins
Under Drupal\acquia_dam\Plugin\media\acquia_dam you'll find that all of our Asset types are separated into new plugins.
You can create a new Annotation like the following:
* @AssetMediaSource(
* id = "audio",
* label = @Translation("Acquia DAM: Audio"),
* default_thumbnail_filename = "generic.png",
* asset_search_key = "ft",
* asset_search_value = "audio",
* )
The plugins we use extend MediaSourceTypeBase -- which is public for customers to extend as well. In it are 3 variables:
- $embedCodeAssetField -> This is the default embedcode that goes in all Asset types, and required even for Download/Sync as the Media Library uses this field for thumbnail display.
- $localFileAssetField -> Used for local file assets. There is an example in the image plugin where the default managed file field is replaced with the imagefield.
- $assetFieldFormatterConfiguration -> An array of the field formatter configuration keyed by the field names for a particular Asset type. Each plugin can override this configuration for their own format. Even though the image plugin adds a new field, it'll get called as the active one because $localFileAssetField is overridden
The two main MediaSourceTypeBase class methods you'll be interested in is getActiveFieldName and getFormatter.
getActiveFieldName: This method gets the active field based on whether 'download assets' is checked or not. If you have a different workflow for your plugin, you could override this method to return a field regardless of the Asset configuration.
getFormatter: Gets the specific formatter settings from the assetFieldFormatterConfiguration dataset.
Hopefully this achieves what you're looking for here. Marking 'needs review' to get validation what we've built here meets your needs.
Closing as the work is captured in https://git.drupalcode.org/project/acquia_dam/-/merge_requests/113
The issue is due to the default media source plugin field being returned and not created during Media Type creation.
Finally passed tests, committed!
Verified this is an issue even with MR113.... Will commit that MR and work on the fix here though. Not sure why, since we now hook into entity_view_display's presave hook.
That other issue is too big so its being closed, thus reopening this and hopefully can just merge it.
Just turned 19 years in Jan! The first oscon was great, and I think I have some pics from the OSCMS summit in Vancouver as well!
Both this and 🐛 Update to 1.1 removes metadata some mappings Active were reported by ibullock -- can you confirm these are two different issues? They seem somewhat related. I'll try to replicate this issue now that the first one has been committed to 1.1
Looks good! fixed.
The MR incorporates some of esteban's suggestions in the patch and removes the version specific caching, which does appear to be breaking the updated versions. Fixed!
This seems very similar to 🐛 DAM Asset Download not updating asset versions Active ?
Left comments in the MR.
This needs a kernel test to validate functionality.
Yup, I think this looks good. Ran a few manual tests and it was okay. Fixed!
Made comments in the MR. it needs a bit of work.
Looks great! Merged.
Left msg on the MR: tldr: update the test to check for the option being checked.