🇮🇳India @vishalkhode

@wimleers: Fixed all your review feedback, hence moving it back to Needs review.

1. There are lots of test failures though:

   The remaining failing PHPUnit tests are being addressed separately, as they are currently failing due to a timeout issue and are expected to be resolved in 📌 Run Tests in Gitlab Parallelly Needs work . Additionally, I noticed that some PHPUnit test is failing as a result of changes introduced in this MR. I will investigate and resolve this.

2. Why is this necessary? It's very unlikely many SDCs will use this, only tests/modules/canvas_test_sdc/components/card-with-stream-wrapper-image/card-with-stream-wrapper-image.component.yml from Canvas' test suite does.

   After going through the change record #3542579: SDC (relative) URL strings props can now define the required target MIME type using contentMediaType → , it reads:
   
   It's recommended to target json-schema-definitions://experience_builder.module/image-uri and json-schema-definitions://experience_builder.module/stream-wrapper-image-uri .
   
   If this is not needed for Acquia DAM, we can skip it.

3. This MR is introducing a whole new layer of new infrastructure: class AssetFieldType extends FieldItemBase and friends.

   A new dynamic computed field type AssetFieldType has been added as this adds a computed field to the Acquia DAM media bundle, with the field name following the format acquia_dam<source_id>_item. This field would have Acquia DAM properties based on the media source. For ex: If the Acquia DAM media source is image, it would have properties such as url, alt, width, height etc.

   The dynamic computed field type was introduced because Canvas requires a field name and prop name. In Acquia DAM, the main field type is AssetItem. While it’s possible to add multiple properties (e.g., src, alt, width) to a single field, I think this approach is not ideal since other media sources (like Video or Audio) do not require these properties.

   Additionally, Acquia DAM media type offers a "Download & sync assets" configuration option. When enabled, assets are downloaded locally and served from the Drupal file system. The business logic in computed field/property ensures that when media is selected in Canvas, it follows this behavior i.e returning Image URL, width, height, etc., based on the selected configuration option.

4. Also: that makes it seem to me that this would need sign-off from whomever architected the Acquia DAM module, to ensure continued maintainability?

   Yes, this will be reviewed by Jakob → / Matt → .

5. Core does not have a ImageFieldItem, only ImageItem.
   And this MR is already adding a new ImageFieldItem!? I'm very, very confused now 😅

   The class ImageFieldItem is different from the Core / Canvas ImageItem. Maybe I need to better re-word the class name 😅. This is the Computed field item list class for Acquia DAM image media item and will return the value for Acquia DAM Image url, height, width etc. properties which we'll be mapping in canvas fieldPropExpression.

Looks good to me. Hence, merged.

@wim-leers: Based on your feedback and the suggestions in #23 💬 Acquia DAM and Experience builder integration support. Active , the following updates have been made:

• A new computed field type has been created and added to the Acquia DAM media type, with all required properties included.
• The `storage_prop_shape_alter()` implementation now retrieves the Acquia DAM field values for the src, alt, width, and height properties.
• A new CI has been added which runs Canvas PHPUnit tests.

Note:

For now, the field name mappings are hardcoded as strings as a temporary workaround due to a bug in Canvas 🐛 FieldPropExpression validation error when multiple bundles share the same field name in prop name mapping. Active .

Question

For Acquia DAM, we also need to update the fieldTypeProp expression for stream-wrapper-image-uri. The challenge is that the Core Image media type uses an Image entity field, where the URI value is extracted from the value property of the uri field. Acquia DAM, however, does not have an entity reference field to extract the URI value. While we can add a uri property to our field, this would require updating the PropExpression similar to how we extract the src value. However, the core ImageFieldItem does not expose a direct property for the URI value.
Should Acquia DAM create a new ImageItemField to add this additional property?
Additionally, due to 🐛 FieldPropExpression validation error when multiple bundles share the same field name in prop name mapping. Active , we cannot use array-based prop names.

The current workaround we've is to create the properties in our field with the exact same name. Then, instead of defining prop names as an array, we are defining the prop name as a string. This avoids the validation error, as the check only applies when the prop name is an array.

Also, attached the patch with Test Scenario → where the PHPUnit test is failing.

vishalkhode → created an issue.

Looks good to me. Hence, merged.

Reviewed changes, looks good to me. Hence, merged.

Reviewed changes, looks good to me. Hence, RTBC.

With Drupal core 11.2, they've added a new key i.e container_rebuild_required → . That needs to be added in module's info.yml.

This is not an issue with Acquia Optimize → module as it's an issue with Services API Key Authentication → module and it's already fixed there: 📌 API error handling in Acquia Optimize extension Active . Hence, closing.

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

@ankitv18: Merged the MR. Awesome work on this. We've made a significant change, we can add all those details in the ticket as well ? Then, will mark this ticket as fixed. Thanks.

For now, this looks good to me. Hence, merged. However, we should create a separate ticket to address adding translated text instead of adding plain English text.

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

Looks good to me.

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

Reviewed changes, looks good to me.

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

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

@ankitv18: Backward compatibility is already taken care of. Please check acquia_cms_audio.module and acquia_cms_audio.install file.

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

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

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

Looks good to me. Merged.

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

vishalkhode → created an issue.

Re-opening this back. As we'll complete the development and merge as part of this ticket.

vishalkhode → created an issue.

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

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

Reviewed changes, looks good to me now. Hence, RTBC.

Reviewed changes, looks good to me now. Hence, RTBC.

Reviewed changes, looks good to me now. Hence, RTBC.

Reviewed changes, looks good to me. Hence, marking RTBC.

Looks good to me. Merged.

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

Reviewed changes, looks good to me. Hence, RTBC.

@japerry I think removing category make more sense here because as per the Drupal, the category can be defined to ensure that the field is appropriately listed under the specified category when created via the UI. However, since our field is configured not to display in the UI, so we can safely remove this option.

@Mikeedfw I was able to replicate the issue on both Acquia DAM → : 1.0.14 → and Acquia DAM → : 1.1.3 → . This appears to be a Drupal Core issue and occurs under the following conditions:

1. Go to the Text formats and editors page and click the Configure button for the text filter you are using. Ex: Full Html. Ex:
2. Scroll to the bottom of the page, and under Filter Settings => Embed media, check the value for the Default View mode option. The value could be something other than "Default" (e.g., "Full Content").
3. Next, go to the Manage Display page for the DAM Image media type (e.g., /admin/structure/media/manage/acquia_dam_image_asset/display) and check the Custom display settings section. The configured default view mode value (e.g., "Full Content") will be unchecked. Ex:

So, basically the issue occurs when the Default View mode is set to a specific value (e.g., "Full Content") in the Embed media filter settings, and that view mode is not enabled for the media type under Custom display settings.

The WSOD error is caused by the LottieFiles Field → module, not due the Acquia DAM module. The acquia_dam_asset field type has no_ui = TRUE, meaning it cannot be created through the UI. Therefore, it is unlikely for this error to occur when creating new field via the UI.

@cb_govcms: I agree that PHP 7 and Drupal 9 are EOL and no longer supported. We could merge this and release a new minor version supporting PHP 8.1 and Drupal 10.3 (and above), while using the 4.0.x branch to maintain backward compatibility with older PHP and Drupal versions.

vishalkhode → changed the visibility of the branch 3236423-4.0.x-do-not-require-unique-characters to hidden.

Update MR !70 and reverted changes from @COBadger. I think we shouldn't create a new password policy constraint for zero password validation as this should be treated normally because user can enter other single digit numeric / string values like 1 or 2 or 3 or a or b etc. The policy validator should be more of generic validations. I've added strlen condition to fix this issue and added PHPUnit tests as well.

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

It seems your configurations do not include password_policy under dependencies.enforced.module, which ensures these configurations are removed when the module is uninstalled.

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

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

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

Reviewed changes, looks good to me. Hence, RTBC.

Reviewed changes, looks good to me. Hence, RTBC.

vishalkhode → changed the visibility of the branch 3519247-approach-1 to hidden.

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

Reviewed changes, looks good to me. Hence, RTBC.

Not able to reproduce issue as well. Verified that getMapping() method is called multiple times, but, due to the condition i.e if (!$this->mapping) {, it only executes once. Please add steps to reproduce and re-open, if we still see this issue. Hence, closing.

Reviewed changes, overall looks good to me. A PHUnit test covering above scenario would be good or we should handle separately ? As we can also then remove the $reset parameter from getKeyValue() and getKeyValues() methods as cache is now being updated / deleted in CRUD operations and ensuring it always returns the correct value. Hence, RTBC.

Requested some changes in the MR. One question here, should this provider function similar like the Environment Key provider, meaning should only read values from the state rather than saving, reading and deleting value as well from the state ?

Reviewed changes, looks good to me. Hence, RTBC.
Note: Core Version requirement has been updated and Drupal Core 8 support is dropped. Probably this requires a new minor release.

vishalkhode → created an issue.

In !MR 6 Looks like there's a complete new feature has been introduced i.e Provide support for external file url for key. I'm not sure if this should be supported or not. In any case, this should be implemented and discussed separately. We should limit this ticket to fix the bug only.

I've to verified and not seeing any error with 1.1.x. Looks like this is already fixed & merged as part of 🐛 TypeError on EmbedCodeUrlBuilder on new revision update for an existing asset/image on Acquia DAM Active . Hence, closing.

To fix the issue, I think we can do the following:

1. Update the method to also update the static cache. For example:
   

     $key_values = &drupal_static('getKeyValue', []);
     $key_values[$this->id()] = $key_value;
   

2. Update the method to remove the value from the static cache as well. For ex:
   

     $key_values = &drupal_static('getKeyValue');
     unset($key_values[$this->id()]);
   

3. Lastly, we should consider removing the $reset argument from the method and the related code, since the static cache should now always reflect the correct and updated value. Are there any edge cases where we might still need the flag ?
4. Add / Update the PHPUnit tests to cover all above scenarios.

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

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

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

rajeshreeputra → credited vishalkhode → .

ankitv18 → credited vishalkhode → .

vishalkhode → created an issue.

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

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

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

Thanks @rajeshreeputra for working on this. Changes looks good. Hence, merged.

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