@rajeshreeputra @japerry , Wouldn't we need key integration for update endpoint as well as read&write token? At this point, it only serves the purpose for analytics.
Since the endpoint is variable per customer/account (environment?) then yes, this needs to be included with the key as well.
I’m just not familiar with the Key module so would need a thorough review for my code afterwards.
There is also a 2.x branch that changes quite a bit of the plugin structure for key, although most of the features should be backported to 1.x for compatibility (with 2.x dropping the deprecated plugins). This work is being done as we speak, with better support for multiple values (as right now the way things work making two keys would be needed instead of one key with two values).
Since the form is not 'final' the constructor signature is different. While its a minor change, we cannot guarantee that no one has extended this form, without making a major change.
Also, unit tests are.. ehh not the greatest. Converted it into a Kernel test. It'd be even better if it was doing request based kernel tests, but this at least confirms the values entered should be returning correctly when submitted.
This commit created some BC issues and needs to be partially reverted. Setting to NW.
Since SMTP doesn't use the methods that 7.0.0 breaks, I think its safe to work with both. Added to composer.json!
Tested locally, error is gone and archive media type is here now. Fixed!
Which key provider should we use for this key entity—file, environment variable, or another option?
By default, it'd just be in config. We shouldn't expect a file, env, or other provider to exist at the moment. It does highlight some of the issues we have with the current version of key.
Thanks for reaching out! I've added you as a co-maintainer to commit code and shepherd issues. Once those issues are committed, I can review the release and make a new cut.
While not specifically caused by this issue, it is easier to see in this issue -- if you put a url schema for the domain in the form, it says the domain passes validation, but redirects improperly. Rolling a fix momentarily to fix that.
Updated issue summary to encapsulate both PHP and Drupal 11 fixes by the bot.
Ugh, that seems like a breaking change in a minor. https://www.drupal.org/node/3496788 →
Merged!
A little delay because it is mixing the image plugin derivative with the File entity helper, but thats a problem for another day.
Merged!
There are some more tests that need updating, moving to NW.
It appears the changes were not tested. Marking NW, It'd be good to verify an SVG is getting the correct embed code.
Exactly what I was looking for! thanks @rajeshreeputra! Will rebase and run tests, assuming they're good, I'll get it merged.
Got it, thanks for the clarification. Merged!
This I think is related...
https://www.drupal.org/project/acquia_dam/issues/3550116
🐛
TypeError: Drupal\acquia_dam\EmbedCodeFactory::getImageDimensions(): Argument #1 ($image_properties) must be of type array, null given, called in /var/www/html/docroot/modules/contrib/acquia_dam/src/EmbedCodeFactory.php
Active
If you end up using original though, you can just not specify a width and height. The code now looks like it'd return null as values on there, which I think isn't what we want.. unless the template or something else interprets that and unsets the key to be included in the embed code.
I changed the title to reflect a few more UI changes that should occur here:
1) The image styles menu should be dependent on being connected to the DAM. If you're not authenticated, don't show the image styles tab.
2) The 'authenticate site' and 'save dam configuration' buttons seem redundant. With the image styles moved, the main page should be for authentication details only, with 'Authenticate Site' and 'Disconnect Site' being the two buttons options (depending on the state of the site)
3) While authentication type is greyed out, domain is not. We have some logic that disconnects the site.. but instead we should just grey all of this out and require someone to 'disconnect' before changing any of the credential details.
Oh yes, I already created an internal JIRA ticket to track that, and if product decides its worth pursuing, we'll make an issue on d.o. Optionally it gets linked here.. but since its a test and not functionality someone coming to this issue is looking for, not sure that needs to occur.
Looks good. If the rebased tests pass then this should be good to go.
The kernel tests with Canvas is failing in the next minor release. Lets look at that before it gets merged?
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.