Awesome work everyone on this issue! I think its 'good enough' to get in! I'm sure this isn't the end, but a great start.
Tested manually against Canvas 1.0-beta2 today. No issues with existing content, although I only have tested with Umami. I'd like to see some Functional Javascript tests, but that can wait pending changes to Canvas.
Committed!
I fixed the issue with css selector not working. However there are several other issues that need to be resolved:
1) On the configuration page, there is an 'authenticate site' button. This shouldn't be here as pressing 'save' with a new domain should automatically redirect you to authenticate the site. If the key module is working correctly, then pressing save should return a "validating domain - ok!' success message.
2) They Key configuration when pressing save is incorrect and will cause a fatal issue because we don't check to see if key is the auth type before fetching the domain.
3) There is an issue with image styles.. not quite sure what yet, but looking at it.
These are blocking before we can merge in the canvas support.
Instead of requiring users to opt-in to using Key via the SearchStax config, how about just using Key by default when the key module exists on the site? The 'opt in' would occur upon detection of the Key module, eliminating another step.
So I'd see the process looking like this:
1) For existing users, if key module is detected, run an update hook that creates a Key entity and moves the credentials there. If key module is not detected, do nothing.
2) For new users, no migration is needed, use Key if it exists, fallback to existing config if it doesn't.
3) If a user enables Key module later on, run a post_install hook that migrates the credentials config to Key automatically.
4) Include a Key entity in the searchstax config under the 'optional' folder, since its only required to be installed if Key exists.
In all cases, have the config form detect whether key module exists and show the appropriate form fields.
Committed the gitlab file, but marking NW to try to fix some of these underlying test failures.
Done! Note, this introduces Key support, but still allows for config based key management as well. In a future release, key will be required, but not yet. If there are issues using Key, feel free to post the issue you're having in here or make a new one!
Moving to active as there isn't an MR on this yet.
Hmm yah that does appear to be a copy/paste oversight. Fixed!
japerry → made their first commit to this issue’s fork.
Looks like there is a legitimate failure in Drupal 9.5. Not sure why its not occurring in Drupal 10...
Looks good, and thanks for the test! Committed.
Sounds good! Fixed.
before pushing this up, we need this to be put into an MR and corresponding tests added.
Committing so we can get this into the next release. However, it'd be good to follow up with some tests to verify its functionality beyond a cursory review.
Can we add a test verifying the fix?
I think you're right here. not closing yet but postponing until after release.
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.
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
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!