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
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
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.
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.
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.
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.
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.
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.
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.
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.
Ahh good catch. Fixed
Looks good! Fixed.
Looks good for now. Committed!
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.
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!
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.
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.
smustgrave → credited japerry → .
It does appear the code in AssetUpdateChecker is not needed. Fixed!
Good find. Fixed!
Love it! Fixed.
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".
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?