EZP-29104: Impl. ImageAsset field type #2403

adamwojs · 2018-07-25T08:39:08Z

Question	Answer
JIRA issue	EZP-29104
Bug/Improvement	no
New feature	yes
Target version	`master`
BC breaks	no
Tests pass	yes
Doc needed	yes

ImageAsset Field Type allows to store images in independent content items of a generic Image content type, in the media library, instead of storing them within the original content item to make then reusable accross system.

From the kernel POV its works as same as a ezobjetrelation field type.

ImageAsset Field Type configuration

ImageAsset Field Type allows to configure the following options:

Name	Description	Default value
`content_type_identifier`	Content type used to store assets	`image`
`content_field_identifier`	Field identifier used to asset data	`image`
`name_field_identifier`	Field identifier used to asset name	`name`
`parent_location_id`	Location where the asset are created	`51`

Example configuration:

ezpublish:
    system:
       default:
            fieldtypes:
                ezimageasset:
                    content_type_identifier: photo
                    content_field_identifier: image
                    name_field_identifier: title
                    parent_location_id: 106

Customizing ImageAsset Field Type rendering

Internally the Image Asset Type is render via subrequest (similar to other relation types). Rendering customization is possible by configuring view type asset_image:

ezpublish:
    system:
       default:           
            content_view:
                asset_image:
                    default:
                        template: '::custom_image_asset_template.html.twig'
                        match: []

Generating image variation from the Image Asset

Thanks to the eZ\Bundle\EzPublishCoreBundle\Imagine\ImageAsset\AliasGenerator decorator it is possible to work with \eZ\Publish\SPI\Variation\VariationHandler in same way as with Image Field Type.

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

mikadamczyk · 2018-08-01T07:06:08Z

eZ/Bundle/EzPublishCoreBundle/DependencyInjection/Configuration/Parser/FieldType/ImageAsset.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getFieldTypeIdentifier()


we can add here return type hint

mikadamczyk · 2018-08-01T07:08:19Z

eZ/Bundle/EzPublishCoreBundle/Imagine/ImageAsset/AliasGenerator.php

+        return $this->innerAliasGenerator->getVariation($field, $versionInfo, $variationName, $parameters);
+    }
+
+    public function supportsValue(Value $value): bool


missing docblock

mikadamczyk · 2018-08-01T07:10:40Z

eZ/Bundle/EzPublishCoreBundle/Imagine/ImageAsset/AliasGenerator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getVariation(Field $field, VersionInfo $versionInfo, $variationName, array $parameters = [])


missing return type hint

mikadamczyk · 2018-08-01T07:12:16Z

...ublishCoreBundle/Tests/DependencyInjection/Configuration/Parser/FieldType/ImageAssetTest.php

+        }
+    }
+
+    public function imageAssetSettingsProvider()


missing docblock and return type hint

mikadamczyk · 2018-08-01T07:15:31Z

eZ/Bundle/EzPublishCoreBundle/Tests/Imagine/ImageAsset/AliasGeneratorTest.php

+class AliasGeneratorTest extends TestCase
+{
+    /**
+     * @var \eZ\Bundle\EzPublishCoreBundle\Imagine\ImageAsset\AliasGenerator


Should we use a single line or multiline for /** @var .... */ In this PR both are used

mikadamczyk · 2018-08-01T07:29:03Z

eZ/Publish/Core/FieldType/ImageAsset/Type.php

+     *
+     * @param \eZ\Publish\Core\FieldType\ImageAsset\Value $value
+     *
+     * @return mixed


@return array?

mikadamczyk · 2018-08-01T07:31:22Z

eZ/Publish/Core/FieldType/Tests/ImageAsset/MapperTest.php

+
+    /** @var \eZ\Publish\API\Repository\ContentService|\PHPUnit_Framework_MockObject_MockObject */
+    private $contentService;
+    /** @var \eZ\Publish\API\Repository\LocationService|\PHPUnit_Framework_MockObject_MockObject */


missing empty line

mikadamczyk · 2018-08-01T07:33:14Z

eZ/Publish/Core/FieldType/Tests/ImageAsset/MapperTest.php

+    }
+
+    /**
+     * @expectedException \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException


mikadamczyk · 2018-08-01T07:35:14Z

eZ/Publish/Core/FieldType/Tests/ImageAsset/MapperTest.php

+        $this->assertEquals($expected, $actual);
+    }
+
+    public function dataProviderForIsAsset(): array


Should we add docblock here and below (createMapper(), createPartialMapper(), createContentWithId(), createContentWithContentType()?

mikadamczyk · 2018-08-01T07:37:47Z

eZ/Publish/Core/MVC/Symfony/Templating/Twig/Extension/ImageExtension.php

@@ -76,4 +88,9 @@ public function getImageVariation(Field $field, VersionInfo $versionInfo, $varia
            }
        }
    }
+
+    public function getImageAssetContentFieldIdentifier()


missing docblock and return type hint

alongosz

I'm sorry, but from my perspective it's impossible to review. Please at least separate implementation from tests (I mean by commits). Since we're gonna squash this, PR commits don't need to have an issue number. They need to be in past tense though.

git reset origin/master

is your friend.

adamwojs · 2018-08-02T07:15:10Z

@alongosz I split PR into two separate commits as you requested: implementation and tests

alongosz · 2018-08-02T13:30:19Z

eZ/Publish/Core/FieldType/ImageAsset/Mapper.php

+use eZ\Publish\Core\Base\Exceptions\InvalidArgumentException;
+use eZ\Publish\Core\FieldType\Image\Value as ImageValue;
+
+class Mapper


Please either change this name to be more meaningful or add short PhpDoc stating what this class maps.

adamwojs · 2018-08-03T08:53:47Z

PR ready for the final code review @alongosz @mikadamczyk

mikadamczyk · 2018-08-06T11:06:32Z

eZ/Publish/Core/FieldType/ImageAsset/Type.php

+     *
+     * @return bool
+     */
+    public function isSearchable(): bool


This method is same as in parent class, so it is redundant

This method is same as in parent class, so it is redundant

I'm sorry to disagree in this case. Relying on the fact that the current implementation of FieldType base core class returns false is risky. While we probably wouldn't change it for any minor version (though there's no BC promise for Core), it's going to be difficult to track in case of major version upgrade.

I agree with @mikadamczyk

I'm sorry to disagree in this case. Relying on the fact that the current implementation of FieldType base core class returns false is risky.

Not in this case: \eZ\Publish\Core\FieldType\FieldType is the abstract class . If I cannot rely on methods inherited from it, then I should implement \eZ\Publish\SPI\FieldType\FieldType from scratch.

adamwojs · 2018-08-10T06:24:25Z

PR updated according to @alongosz suggestions.

alongosz

+1, one nitpick, but fixing it won't affect behavior, so can be tested by QA

QA:
please try to see if you're able to use it in multisite environment with changed SiteAccess-aware config of this feature per SA (I suggest just parent_location_id).
I'm asking for it because there are known issues when injecting dynamic parameters from SA config resolver.

alongosz · 2018-08-10T12:42:24Z

eZ/Publish/Core/settings/fieldtype_services.yml

+            $contentService: '@ezpublish.api.service.content'
+            $locationService: '@ezpublish.api.service.location'
+            $contentTypeService: '@ezpublish.api.service.content_type'
+            $mappings: '$fieldtypes.ezimageasset.mappings$'


Since the setting is introduced on Bundle level, here you should rather have some core fallback. It will also allow you to avoid overriding service in test yamls.

barbaragr

Ready to merge @alongosz :)

This comment has been minimized.

Sign in to view

adamwojs force-pushed the EZP-29104-imageasset_clean branch from f781352 to 6b81695 Compare July 25, 2018 08:56

This was referenced Jul 26, 2018

EZP-29104: Impl. ImageAsset field type ezsystems/repository-forms#246

Merged

EZP-29104: Implemented UI for ImageAsset Field Type ezsystems/ezplatform-admin-ui#580

Merged

adamwojs force-pushed the EZP-29104-imageasset_clean branch 2 times, most recently from 6cabfd0 to f131f00 Compare July 27, 2018 09:01

adamwojs requested review from alongosz, kmadejski, lserwatka, mikadamczyk, Nattfarinn, webhdx and ViniTou July 27, 2018 09:02

adamwojs changed the title ~~[WIP] EZP-29104: Impl. ImageAsset field type~~ EZP-29104: Impl. ImageAsset field type Jul 27, 2018

Nattfarinn approved these changes Aug 1, 2018

View reviewed changes

mikadamczyk requested changes Aug 1, 2018

View reviewed changes

alongosz requested changes Aug 1, 2018

View reviewed changes

adamwojs force-pushed the EZP-29104-imageasset_clean branch from a66e067 to 9f77101 Compare August 2, 2018 07:13

This comment has been minimized.

Sign in to view

adamwojs force-pushed the EZP-29104-imageasset_clean branch from 025369d to fa162cb Compare August 2, 2018 08:26

This comment has been minimized.

Sign in to view

alongosz requested changes Aug 2, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

mikadamczyk approved these changes Aug 6, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

alongosz approved these changes Aug 10, 2018

View reviewed changes

barbaragr self-assigned this Aug 22, 2018

adamwojs force-pushed the EZP-29104-imageasset_clean branch from f8d41a3 to 67fa812 Compare September 11, 2018 09:59

barbaragr approved these changes Sep 11, 2018

View reviewed changes

adamwojs added 4 commits September 11, 2018 19:47

EZP-29104: ImageAsset field type (implementation)

c991c44

EZP-29104: ImageAsset field type (tests)

501f646

EZP-29104: Replaced one line docblocks with the three-line equivalents

ef5725c

EZP-29104: Impl. \eZ\Publish\SPI\FieldType\Indexable for ezimageasset FT

525ec16

alongosz force-pushed the EZP-29104-imageasset_clean branch from 67fa812 to 525ec16 Compare September 11, 2018 17:50

alongosz merged commit 4892d15 into ezsystems:master Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29104: Impl. ImageAsset field type #2403

EZP-29104: Impl. ImageAsset field type #2403

adamwojs commented Jul 25, 2018 •

edited

Loading

This comment has been minimized.

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

mikadamczyk Aug 1, 2018

alongosz left a comment

adamwojs commented Aug 2, 2018

This comment has been minimized.

This comment has been minimized.

alongosz Aug 2, 2018

This comment has been minimized.

adamwojs commented Aug 3, 2018

mikadamczyk Aug 6, 2018

alongosz Aug 6, 2018 •

edited

Loading

adamwojs Aug 7, 2018

This comment has been minimized.

This comment has been minimized.

adamwojs commented Aug 10, 2018

alongosz left a comment

alongosz Aug 10, 2018

barbaragr left a comment

EZP-29104: Impl. ImageAsset field type #2403

EZP-29104: Impl. ImageAsset field type #2403

Conversation

adamwojs commented Jul 25, 2018 • edited Loading

ImageAsset Field Type configuration

Customizing ImageAsset Field Type rendering

Generating image variation from the Image Asset

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

adamwojs commented Aug 2, 2018

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

adamwojs commented Aug 3, 2018

Choose a reason for hiding this comment

alongosz Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

adamwojs commented Aug 10, 2018

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaragr left a comment

Choose a reason for hiding this comment

adamwojs commented Jul 25, 2018 •

edited

Loading

alongosz Aug 6, 2018 •

edited

Loading