🇺🇸United States @japerry

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.

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.

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

Fixes look good, Committed.

Good catch! Patch looks good to me, and passes tests. committed.

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

Made comments in the MR

Looks good! (also rebased against 1.1.. will commit when those tests pass as well.)

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

The new version of Acquia Dam → (1.1) should accomplish this functionality

Rebased and merged in! Fixed.

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

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

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.

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

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.

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

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.

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

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!

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

This seems very similar to 🐛 DAM Asset Download not updating asset versions Active ?

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

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!

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

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

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.

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

Yah I just saw this msg as well. Sounds good to me!

Marking NW per comment #38. Because of the new service parameter, this module should just those values. Note, that option was added in Drupal 10.1.

It'd be preferable to not have additional config. To that end, samesite and lifetime settings do not need to be set. For Drupal 10+, use core's settings. For Drupal 10.0 and lower, just set a default 'Lax' and cookie lifetime to 1 month? 1 year seems long. This maintains compatibility with older versions of Drupal that likely wouldn't need anything but the defaults anyway.

Also, shouldn't the cookie secure setting default to TRUE? Sites should be on https and only need this override in cases like https->http load balancer setups. If you're running on http and forget to set it to FALSE, not quite sure what will happen there, but not sure we should care...

While I like having a link to the log entry, ehh its not really needed. So I'm okay with this.

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

Looks good! Merged

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

Since the MR is changing the default field to download/sync instead of embed, marking NW. Also upping priority. The default functionality of 1.1 shouldn't be any different than 1.0. We're just adding the option to download and sync assets.

Note, the changes proposed in 🐛 Thumbnails missing for media items after enabling on-site asset storage Active should be in this PR. a cursory review shows that its at least part of it is in this MR.

This is being handled in 🐛 Media view display handling not honored during migration Active

Good catch. Fixed!

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

greggles → credited japerry → .

greggles → credited japerry → .

Looks good! Fixed in 1.0. Will cherry pick to 1.1, probably after release.

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

The importer tool does say under the category selection text:

"List of categories in the remote DAM system available for the authorized user account. Please choose which of them the media assets should be imported from. When adding the same category multiple times, only the last row of them will be considered and saved. "

lgtm as well! The addition to the queue worker is perfect. Committed.

This work should be done here: https://www.drupal.org/project/media_acquiadam/issues/3500658 🐛 Media view display handling not honored during migration Active

This might have something to do with an error I see when deleting migrated content as well.

"Warning: Attempt to read property "asset_id" on null in acquia_dam_media_delete() (line 696 of /var/www/html/upstreams/acquia_dam/acquia_dam.module).

acquia_dam_media_delete(Object)"

Won't make a new ticket until we validate against this one.

weird that it didn't mark the commit. Closing!

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

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

This is by design. Authentication tokens are not stored in configuration. Except for a raw db dump (with no security striping, not recommended), users will need to re-authenticate when deploying a new site. On a side note, deploying configuration to a pre-existing, authenticated site, should NOT invalidate credentials.

Fixed! Will roll out an RC3 shortly.

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

The fact that this doesn't include a test, nor does it make existing tests fail, while being a fundamental change of the modules functionality (defaulting redirect from false to true) -- is probably a big enough gap to mark needs work, until a test is added that confirms the new functionality. (or at least confirms the current functionality is broken)

Committed the RC2 code cleanup fixes. Marking this issue fixed, if there are other issues found, a new issue can be made.

the new MR passes tests, committed!

Unfortunately I do not believe such a large change will occur in this module, due to its EOL status. Working to update the documentation so that users will goto the Google Tag module instead. Fortunately the Google Tag module incorporates the goals of this patch, so please try it out!

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

The previous MR incorrectly sets the GAAccounts wrapper with an empty value. new MR incoming.

japerry → changed the visibility of the branch 3353389-typeerror-argument-1 to hidden.