Introduced MediaContent and MediaContentApi modules #27536

sivaschenko · 2020-04-01T16:13:19Z

Depends on:

Description (*)

The Magento_MediaContent module provides implementations for managing relations between content and media files used in that content.

Additionally observers have been added to Cms and Catalog modules to save the relation of corresponding entities to MediaContent storage

Story

magento/adobe-stock-integration#717: User sees pages the image is used in Magento

Related Pull Requests

magento/adobe-stock-integration#1235

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2020-04-01T16:13:22Z

Hi @sivaschenko. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

sivaschenko · 2020-04-02T17:41:48Z

app/code/Magento/MediaContentCatalog/Observer/Product.php

+    {
+        /** @var CatalogProduct $model */
+        $model = $observer->getEvent()->getData('product');
+        if ($model instanceof AbstractModel) {


Suggested change

if ($model instanceof AbstractModel) {

if ($model instanceof \Magento\Catalog\Model\Product) {

@sivaschenko will this work if this preference will be changed <preference for="Magento\Catalog\Api\Data\ProductInterface" type="Magento\Catalog\Model\Product" /> and Magento\Catalog\Api\Data\ProductInterface will have another implementation?

If not can we leave AbstractModel instance identification? I can hardly imagine the case when the Magento\Catalog\Api\Data\ProductInterface can have another implementation but if this possible from the di perspective it can break the process. Or I missed something?

@naydav can you please address this question?

app/code/Magento/MediaContent/etc/db_schema.xml

…media-content

maghamed

Do we expect to expose any of those newly created APIs via WEB API ?

maghamed · 2020-04-02T19:37:19Z

app/code/Magento/MediaContentApi/Api/ExtractAssetFromContentInterface.php

+     * @param string $content
+     * @return AssetInterface[]
+     */
+    public function execute(string $content): array;


what's $content in this case?

A "content" in the scope of the MediaContent domain is an alias to any kind of an object property which possibly contains a media asset. For example, a product object has a description which probably includes a media asset. A category object description and image also can have a media asset as a part of storage data.

How somebody (a developer) knows what to put in this property? Where to find list of supported aliases?

Data, selected for this property, is a developer choice made on next conclusion: if an object has a property which possibly contains a media asset, to get it from this property it needs to be sent to this Interface implementation. For example, currently, we represented a declaration of content properties in di:
https://github.com/magento/magento2/pull/27536/files/b6e79cf870273936cb70ffc05add1fa73a9e3334#diff-bcf67dbc51c19997838fdb60cee182abR9-R16

List of aliases, in that case, is a list of properties declared manually based on data structure and a possibility to have a media asset in properties.

@buskamuza that's a parser that looks for image mentions in any content/text/string. Should we change the naming to make it better understandable or improving comments would be enough?

maghamed · 2020-04-02T19:39:41Z

app/code/Magento/MediaContentApi/Api/GetContentWithAssetInterface.php

+    /**
+     * @param int $assetId
+     *
+     * @return array


what's the format of return result ? Not looking at implementation it's not clear

It's ContentIdentityInterface[] (introduced data interface), the code is updated

app/code/Magento/MediaContent/Model/UnassignAsset.php

app/code/Magento/MediaContent/composer.json

sidolov · 2020-04-02T21:14:28Z

app/code/Magento/MediaContent/etc/di.xml

+    <preference for="Magento\MediaContentApi\Api\UnassignAssetInterface" type="Magento\MediaContent\Model\UnassignAsset"/>
+    <preference for="Magento\MediaContentApi\Api\GetAssetsUsedInContentInterface" type="Magento\MediaContent\Model\GetAssetsUsedInContent"/>
+    <preference for="Magento\MediaContentApi\Api\GetContentWithAssetInterface" type="Magento\MediaContent\Model\GetContentWithAsset"/>
+    <preference for="Magento\MediaContentApi\Api\ExtractAssetFromContentInterface" type="Magento\MediaContent\Model\ExtractAssetFromContent"/>


Why interface and implementation located in one module and marked as API? It does not make sense for me. It would be better to leave the interface only.

The interface is used for injecting the service, the model is used for di configuration

That change was requested by @naydav at the online review

buskamuza · 2020-04-02T22:05:50Z

Looks like domain model has such entity as "content". There are several places with string $contentType, string $contentEntityId, string $contentField interface. It would help to have this entity defined explicitly in a data interface.

Also, I'd suggest to define more clearly purpose of the MediaContent domain and make sure public APIs don't include interfaces/classes which are just details of implementation.

Public API should be useful for developers from unrelated domains. For example, if somebody would like to use MediaContent for order details or for emails, which methods would be needed? Things like "relations" sound too technical (low level) and don't help understand domain model.
I would even think about not exposing any public API for MediaContent, as it looks like necessary for MediaGallery only.

buskamuza · 2020-04-02T21:41:33Z

app/code/Magento/MediaContentApi/Api/AssignAssetInterface.php

+     *
+     * @throws \Magento\Framework\Exception\CouldNotSaveException
+     */
+    public function execute(int $assetId, string $contentType, string $contentEntityId, string $contentField): void;


$contentEntityId - what is "content entity"? In the implementation I see no relation of the ID to anything https://github.com/sivaschenko/magento2/blob/media-content/app/code/Magento/MediaContent/Model/AssignAsset.php#L51 , so it's not clear what does this ID mean.

Is it "CMS", "Product Page", etc? Why $contentType is a string? I'd assume there is limited set of content types supported. Looks like implementation doesn't limit any input as well.

content entity id is an id of an entity which has content where an asset is used. For example, if we store a relation between asset and content of the product entity the catalog_product_entity.entity_id will be used for this.
In a provided implementation this is used here: https://github.com/magento/magento2/pull/27536/files#diff-425fc7fd56cadd0cbbf4f2b600c74771R107

The implementation doesn't limit supported types of object with content to provide extensibility. From the box, the cms_page, cms_block, catalog_product, catalog_category provided as a basic content type. If someone will need to store information about media asset usage in the content of a custom object it will create its own content type and this automatically be used for all other features such as showing where and how an asset is used.

@buskamuza we've introduced ContentIdentityInterface, please see the updated code

buskamuza · 2020-04-02T21:44:26Z

app/code/Magento/MediaContentApi/Api/AssignAssetInterface.php

+namespace Magento\MediaContentApi\Api;
+
+/**
+ * Saving data represents relation between the media asset and media content


"Assign asset to media content" would sound less technical and would explain the domain better, IMO. Or "Assign content to asset", depending on what is happening.

comment updated

buskamuza · 2020-04-02T21:44:59Z

app/code/Magento/MediaContentApi/Api/ExtractAssetFromContentInterface.php

+ * Used for extracting media asset list from a media content by the search pattern.
+ * @api
+ */
+interface ExtractAssetFromContentInterface


Should it be plural then? ExtractAssetsFromContentInterface

buskamuza · 2020-04-02T21:49:08Z

app/code/Magento/MediaContentApi/Api/ExtractAssetFromContentInterface.php

+     * @param string $content
+     * @return AssetInterface[]
+     */
+    public function execute(string $content): array;


How somebody (a developer) knows what to put in this property? Where to find list of supported aliases?

buskamuza · 2020-04-02T21:51:13Z

app/code/Magento/MediaContentApi/Api/GetAssetsUsedInContentInterface.php

+     * @return int[]
+     * @throws \Magento\Framework\Exception\IntegrationException
+     */
+    public function execute(string $contentType, string $contentEntityId = null, string $contentField = null): array;


If there are content aliases (as in the previous interface), why there is also this method? Is it that in some cases alias is not known or doesn't exist? And in some cases an alias is all you know (and don't know about content type and all the input for this method)?

buskamuza · 2020-04-02T21:55:33Z

app/code/Magento/MediaContentApi/Model/ExtractAssetFromContent.php

+ *
+ * This class should be used for DI configuration only, please use the interface in the code
+ *
+ * @api


Probably marking the interface with @api should be enough.

this class should be used for DI configuration

app/code/Magento/MediaContent/Model/DeleteContentAssetLinks.php

app/code/Magento/MediaContentApi/Api/Data/ContentAssetLinkInterface.php

app/code/Magento/MediaContent/Model/Content/ConfigInterface.php

app/code/Magento/MediaContent/Model/Content/Config.php

sivaschenko

Review with Valerii

app/code/Magento/MediaContent/Model/ContentIdentity.php

sivaschenko · 2020-04-14T16:44:34Z

app/code/Magento/MediaContent/Model/ContentIdentity.php

+    /**
+     * @inheritdoc
+     */
+    public function setExtensionAttributes(ContentIdentityExtensionInterface $extensionAttributes): void


Delete all setExtensionAttributes if it works with webapi

app/code/Magento/MediaContent/Model/GetAssetIdsUsedInContent.php

app/code/Magento/MediaContent/Model/GetContentWithAssets.php

app/code/Magento/MediaContent/Model/RemoveRelationsForAssetIds.php

app/code/Magento/MediaContent/Model/UpdateRelations.php

sivaschenko · 2020-04-14T17:06:48Z

app/code/Magento/MediaContent/Plugin/MediaGalleryAssetDeleteByDirectoryPath.php

+     * @param string $path
+     * @return int[]
+     */
+    private function getAssetIdsByDirectoryPath(string $path): array


Update after MediaGallery PR is merged

sivaschenko · 2020-04-14T17:07:50Z

app/code/Magento/MediaContent/Plugin/MediaGalleryAssetDeleteByPath.php

+        \Closure $proceed,
+        string $mediaAssetPath
+    ) : void {
+        $asset = $this->getByPath->execute($mediaAssetPath);


update after merge of MediaGallery PR

app/code/Magento/MediaContentCatalog/Observer/Category.php

sivaschenko · 2020-04-14T17:23:44Z

app/code/Magento/MediaContentCms/Observer/Page.php

+     *
+     * @param Observer $observer
+     */
+    public function execute(Observer $observer): void


Observers appear to be deprecated (but not documented)

lenaorobei · 2020-04-16T19:11:01Z

Performance build: https://bamboo.corp.magento.com/browse/MCCE24-MCCE24405-1

VladimirZaets · 2020-04-16T20:46:53Z

@magento run tests

VladimirZaets · 2020-04-16T20:47:39Z

@magento run all tests

…tent

VladimirZaets · 2020-04-16T22:10:41Z

@magento run all tests

VladimirZaets · 2020-04-17T00:04:20Z

@magento run all tests

sivaschenko · 2020-04-19T16:51:05Z

The pull request was tested with Used In functionality on Adobe Stock Integration

…content

m2-assistant · 2020-04-20T15:48:01Z

Hi @sivaschenko, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

ghost added the Progress: pending review label Apr 1, 2020

magento-engcom-team added Component: Catalog Component: Cms Component: MediaContent Release Line: 2.4 labels Apr 1, 2020

sivaschenko requested review from coderimus, sidolov, slavvka and naydav April 1, 2020 16:13

Introduced MediaContent and MediaContentApi modules

d88ed42

sivaschenko requested review from angelgonzalezi4 and VladimirZaets and removed request for angelgonzalezi4 April 1, 2020 16:23

sivaschenko added 3 commits April 1, 2020 17:54

#27536: Included MediaContent modules in root composer.json

49f254a

#27536: Code review changes

6cd1dac

#27536: Code review changes

b6e79cf

sivaschenko commented Apr 2, 2020

View reviewed changes

ghost assigned sivaschenko Apr 2, 2020

sivaschenko commented Apr 2, 2020

View reviewed changes

app/code/Magento/MediaContent/etc/db_schema.xml Outdated Show resolved Hide resolved

Merge remote-tracking branch 'mainline/2.4-develop' into sivaschenko-…

123d06a

…media-content

maghamed reviewed Apr 2, 2020

View reviewed changes

ghost assigned maghamed Apr 2, 2020

sidolov requested changes Apr 2, 2020

View reviewed changes

ghost assigned sidolov Apr 2, 2020

ghost added Progress: needs update and removed Progress: pending review labels Apr 2, 2020

#27536: Review fixes

23fb914

buskamuza suggested changes Apr 2, 2020

View reviewed changes