🇺🇸United States @japerry

MR needs to be updated, but I'm hoping that with the other changes made around deleted assets and the 'released_and_not_expired' key - I think we can look everywhere to ensure that it is replaced with the exception code.

Yah, I think if it can be added to the view without messing up the other filters, then lets do that. Unfortunately that also means more review time needed VS just putting up the api calls in the current MR, which really is harmless since you'd intentionally want to use it.

lgtm! Committed.

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

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

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

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

See comments in the MR

Committing to 2.x so development can continue here. Eventually it'll need to be backported to 1.x

Updated the 2.0 plan, I might argue that Input plugins shouldn't exist at all and instead be optional traits for KeyTypes when using the UI.

Closing in Lieu of 📌 Support PHP attributes for KeyProvider plugins Active

japerry → created an issue.

This should be a huge improvement for checking deleted assets. Committed!

Looks like the tests will need to be updated/expanded for this to go in. You'll notice a few commits here but that was a mistake, ahh patches ;).. anywho, once those tests are fixed this looks good to me.

Fixed in D11 compatiblity issue

Fixed in D11 compatiblity issue

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

This was merged with the d11 compatibility issue

Committed, release incoming.

Oh sorry, this totally fell off my radar. I'll get a release out shortly!

Looks good. Fixed!

I think this is on the right path, would like some tests though. Probably should look at how we're hard coding the mappings as well, so you don't need to use a hook_alter.

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

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

Looks good -- It'll be interesting to see if we need more handling on a per plugin basis. Fixed!

Changes look good, fixed!

Looks good, thanks for the test as well. Fixed.

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

Agreed, with no ui this shouldn't appear at all anyway.

This was already fixed back in May. https://git.drupalcode.org/project/acquia_connector/-/commit/ef8c8b07579...

Agreed, I doubt this will throw a WSOD, however, we should add the BC shim or remove the category altogether

japerry → changed the visibility of the branch 3476376-provide-local-file to active.

Fix for 7.x-4.x is committed as well.

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

Marking NW per comments in the MR.

Added comments in the MR.

That data shouldn't be getting passed raw, so I think encoding the return is the right thing here. Fixed!

Yup, looks like #3474018 is the cuprit. Thanks for the hook. Fixed!

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

japerry → created an issue.

Done!

Rajeshreeputra is correct here, we needed to simply adjust the post headers so the authenticate method would work. The client ID is no longer valid, so all of that code that you were working on is not needed anymore.

I've updated the MR to remove all of the oauth authorization token information. Note, for BC (because many of these methods are public), I've left most method signatures alone. In theory, you can run cron still and it'll get a token using the API key/secret provided during the login screen.

quietone → credited japerry → .

accounts.acquia.com is no longer available. Please see https://acquia.my.site.com/s/article/Acquia-ID-Implementation-and-Setup

We did some work back in December (#3491727) in anticipation of the Acquia ID rollout, but unfortunately the error we expected to get from accounts is not what matches what you're seeing here. Therefore, with all customer facing accounts now moved to Acquia ID, we need to remove all of the OpenID connect code, due to the new IDP not accepting static client IDs anymore.

I agree with this logic change. Fixed!

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

Due to the fact that code components are hard coded to image schemas, and that we need to define our own schema to start asset, external, and version IDS.. marking postponed until 📌 Support image shape matching from the `image` MediaSource: support both the `image` + `oembed` MediaSources simultaneously Active lands.

@divyansh.gupta -- credits are given to those who contribute substantially to an issue. If you compare your commit (or mine) to what work was done, it did not rise sufficiently in this case to get credit.

@rajeshreeputra -- Ugh thats unfortunate. Fixed, but will need to go into the next version.

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

Thank you rajeshreeputra for fixing the tests! Looks good, committed.

Updated the code to modern standards (while still supporting our supported versions of Drupal), as well as make it consistent with the other Drush Commands in DAM.

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

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!