KVUO
Account created on 11 January 2006, over 19 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States japerry KVUO

\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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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!

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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)

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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".

🇺🇸United States japerry KVUO

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 ?? '');

🇺🇸United States japerry KVUO

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?

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

Thanks for the test fixes! committed.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

Thanks for fixing the test! Fixed.

🇺🇸United States japerry KVUO

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'

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

Looks good! Committed.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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)

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

Marking fixed since this is in 1.1 now.

🇺🇸United States japerry KVUO

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!

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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.

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

🇺🇸United States japerry KVUO

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

Production build 0.71.5 2024