Acquia DAM and Experience builder integration support.

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

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

Updated MR with necessary changes, requesting review.

chandu7929 → changed the visibility of the branch 3519247-acquia-dam-all-media to hidden.

The approach in this MR is incompatible with what I've heard @lauriii describe: support both core's Image MediaType and Acquia DAM images at the same time.

👉 📌 Decouple image shape matching from the `image` MediaType Active paved most of the path for this to work the way that I've heard @lauriii describe it, but you'll first need to do 📌 Support image shape matching from the `image` MediaSource: support both the `image` + `oembed` MediaSources simultaneously Active to make it entirely possible. I've included clear next steps.

More concerning: the way this MR currently works is by introducing a completely separate prop shape, which means that all existing SDCs/code components won't work! Plus, the sole SDC that uses that new prop shape is adding half a dozen or so props that would present concerns to the content author that they should never see.

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.

Implemented 📌 Support image shape matching from the `image` MediaSource: support both the `image` + `oembed` MediaSources simultaneously Active for y'all.

Then, over here:

The existing hook_storage_prop_shape_alter() in this MR was very helpful as a blueprint of knowing what the bundle + field name + prop name to target were! Thanks 🙏 😊
1. Merged in upstream 1.1.x because without it, Acquia DAM does not support Drupal 11.2, which XB requires
2. Removed the SDC this MR was adding: https://git.drupalcode.org/project/acquia_dam/-/merge_requests/147/diffs...
3. Added test coverage to prove that Acquia DAM can successfully build on top of what XB's \Drupal\experience_builder\Hook\ShapeMatchingHooks::mediaLibraryStoragePropShapeAlter() does, by taking whatever that found and adding in Acquia DAM's (hardcoded?!) image media type: https://git.drupalcode.org/project/acquia_dam/-/merge_requests/147/diffs...
4. … then updated Acquia DAM's hook_storage_prop_shape_alter() implementation to actually make the test pass
5. Result:

Note: CI is failing on 11.2 for HEAD of Acquia DAM: https://git.drupalcode.org/project/acquia_dam/-/jobs/5977020. This MR just adds one more failure: https://git.drupalcode.org/issue/acquia_dam-3519247/-/jobs/6071248. I recommend pinning it to Drupal 11.2.2 just like XB does, otherwise you won't have a stable test target.

The crucial new test passes fine locally — well, 3 of the 4 pass:

DDFD                                                                4 / 4 (100%)

Time: 00:53.362, Memory: 16.00 MB

There was 1 failure:

1) Drupal\Tests\experience_builder\Kernel\AcquiaDamHookStoragePropShapeAlterTest::testPropShapesYieldWorkingStaticPropSources
Sample value [{"target_id":"1"},{"target_id":"2"},{"target_id":"3"}] generated by field type entity_reference for type=array&items[$ref]=json-schema-definitions://experience_builder.module/image&items[type]=object is invalid!
Failed asserting that two arrays are identical.
…

The reason? The sample acquia_dam_image_asset media entity that gets randomly generated is invalid. I'll leave that for y'all to solve :)

Per request of @mglaman:

Also, GIF of this in action and showing the ability to seamlessly pick images from the local media library (core's Image extends File media source) and Acquia DAM:

FYI: when 📌 Decouple image (URI) shape matching from specific image file types/extensions Active lands, tests/src/Kernel/AcquiaDamHookStoragePropShapeAlterTest.php() will need to be updated similar to https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....

In fact, I'd recommend doing that now — that's worthwhile hardening already :)

While here, checked where this MR is at. I'm surprised to see:

2) Drupal\Tests\experience_builder\Kernel\AcquiaDamHookStoragePropShapeAlterTest::testUniquePropSchemaDiscovery
Exception: Exception when installing config for module experience_builder, message was: Schema errors for editor.editor.xb_html_block with the following errors: 0 [settings.plugins.ckeditor5_list.properties] 'styles' is a required key.

— https://git.drupalcode.org/issue/acquia_dam-3519247/-/jobs/6072258#L95

I checked my local clone of Drupal 11.2 and the ckeditor5.plugin.ckeditor5_list config schema type definitely does not list a styles key under properties 🤔

The problem: this is currently testing against 11.x, it should be testing against 11.2. Which is the current minor, yet this project's "current" is actually testing against Drupal 10.3. I wonder if this is a problem with https://www.drupal.org/project/gitlab_templates → , or with this project. I haven't touched https://git.drupalcode.org/project/cdn in well over a year, and that is testing against 11.2 🤔

Hope this helps — good luck!

@wim leers, validated this with the latest 1.x-dev version of XB and Drupal 11.2, following the steps outlined, but encountered an error.

1. New Drupal project setup
2. XB UI setup completed
3. XB site installation completed
4. Installed test modules (xb_test_sdc, xb_dev_standard, sdc_test_all_props)
5. Installed Acquia DAM
6. Applied patch from MR!147
7. Edited test article with XB
8. Added XB test SDC with optional image and heading component
9. Clicked on Add media; selected Acquia DAM from the media source options.
10. Selected and inserted media; asset thumbnail appears in the settings tab, but the asset does not render in the body.
11. Error in dblog:

Regarding the CI, in the Acquia DAM module, we need to run tests with Drupal versions 9.5, 10, and 11. Therefore, we have set the current version to 10, allowing us to test 9.5 under the previous major version and 11 under the next major version, while also testing with the current version, Drupal 10.

The problem: this is currently testing against 11.x, it should be testing against 11.2. Which is the current minor, yet this project's "current" is actually testing against Drupal 10.3. I wonder if this is a problem with https://www.drupal.org/project/gitlab_templates → , or with this project. I haven't touched https://git.drupalcode.org/project/cdn in well over a year, and that is testing against 11.2 🤔

Reproduced #18.

#19: you can still easily set up additional CI jobs that test additional versions of Drupal core.

To be able to test this on the latest, I had to match the https://drupal.org/project/experience_builder → https://drupal.org/project/canvas rename. Pushed a commit for that 👍

Why no xdebug? 😅

It looks like nobody here used a debugger? The problem is quite clear quite quickly. It's a genuine JSON Schema violation reported by Drupal core's SDC subsystem:

Culprit: justinrainbow/json-schema 5.x → 6.x

Now, why would that happen for y'all (and for me too today as I said in #20), but it didn't happen for me in #15 — aka a month earlier? Clearly, the same values should be getting passed. So it MUST be related to JSON Schema validation logic.

And … as chance would have it, literally hours before #18, 📌 Support for latest 6.x version of justinrainbow/json-schema package Active landed. 😬 And that's what caused a difference in behavior:

{
   "src": "http://example.com/cat.jpg",
   "alt": null,
   "width": null,
   "height": null
}

was considered valid by justinrainbow/json-schema:5.x, but invalid by justinrainbow/json-schema:6.x. That's it! 😅

Conclusion

Now, 5.x was quite incorrect in terms of validation in many ways. 6.x is far better. So the failure here is legit, because alt, width and height are optional, but that doesn't mean they are allowed to be null. (Try it on https://www.jsonschemavalidator.net/ for example.)

So we have 2 choices:

1. loosen JSON Schema in Canvas:
   

   diff --git a/schema.json b/schema.json
   index 52483d534..f2ef8a7cb 100644
   --- a/schema.json
   +++ b/schema.json
   @@ -267,15 +267,15 @@
            },
            "alt": {
              "title": "Alternative text",
   -          "type": "string"
   +          "type": ["string", "null"]
            },
            "width": {
              "title": "Image width",
   -          "type": "integer"
   +          "type": ["integer", "null"]
            },
            "height": {
              "title": "Image height",
   -          "type": "integer"
   +          "type": ["integer", "null"]
            }
          }
        },
   

2. Improve optional property matching in Canvas: omit them, rather than falling back to NULL

I think the latter is preferable. Issue to make that happen: 🐛 Follow-up for #3530533: `::SYMBOL_OBJECT_MAPPED_OPTIONAL_PROP` should be omitted rather than evaluate to NULL. Active .

🐛 Follow-up for #3530533: `::SYMBOL_OBJECT_MAPPED_OPTIONAL_PROP` should be omitted rather than evaluate to NULL. Active is in! (Two trivial commits: updated test coverage + tweaked evaluation logic.)

Remaining steps here:

1. the testing challenge I originally identified in #17, which @rajeshreeputra partially responded to in #19 and I then responded to in #21 still needs to be addressed. Just set up an additional GitLab CI job. Something like:
   

   composer-canvas-core-version:
     _TARGET_CORE: 11.2.2
   
   phpunit-canvas-core-version:
     needs:
       - composer-canvas-core-version
     variables:
       _TARGET_CORE: 11.2.2
     extends: .phpunit-base
   

   (which is what I recommended all the way back in #13)

2. Per my #14: the new test coverage this MR introduces is failing randomly (!) due to a bug in \Drupal\acquia_dam\Plugin\Field\FieldType\AssetItem: it should override the parent ::generateSampleValue(). (For similar reasons that \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::generateSampleValue() exists.)
3. Surely we actually do want alt + width + height to be retrieved for Acquia DAM-stored images?! 😅 The only logic related to this I seem to be able to find in the Acquia DAM code base is \Drupal\acquia_dam\AssetFileEntityHelper::downloadFile(), but I'm probably looking in the wrong place? That'd suggest this only works if Acquia DAM is configured to "Download and sync", which seems unnecessary?
   (I expected this information to be available via a computed field property?)

wim leers → changed the visibility of the branch 1.1.x to hidden.