EZP-29104: Impl. ImageAsset field type #246
Conversation
adamwojs
force-pushed
the
EZP-29104-imageasset_clean
branch
from
4cb73fc to
c41b7e6
Compare
adamwojs
changed the title
adamwojs
requested review from
lserwatka,
mikadamczyk,
Nattfarinn,
alongosz,
webhdx,
kmadejski and
ViniTou
| public function transform($value) | ||
| { | ||
| if (!$value instanceof Value) { | ||
| return null; |
There was a problem hiding this comment.
To make it more future proof I'd rather return null for $value = null but for !$value instanceof Value it should throw \Symfony\Component\Form\Exception\TransformationFailedException.
| $this->fieldTypeService = $fieldTypeService; | ||
| } | ||
|
|
||
| public function mapFieldDefinitionForm(FormInterface $fieldDefinitionForm, FieldDefinitionData $data) |
There was a problem hiding this comment.
I don't think you need to implement FieldDefinitionFormMapperInterface and define empty method. It should work without these.
| class ImageAssetFieldType extends AbstractType | ||
| { | ||
| /** @var \eZ\Publish\API\Repository\ContentService */ | ||
| private $contentService; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| { | ||
| $builder | ||
| ->add( | ||
| 'destinationContentId', |
There was a problem hiding this comment.
Nitpick: no need to make it multiline
|
|
||
| class ImageAssetValueTransformer extends AbstractBinaryBaseTransformer implements DataTransformerInterface | ||
| { | ||
| public function transform($value) |
There was a problem hiding this comment.
what about docblock and return type hint here and reverseTransform()?
| /** @var \eZ\Publish\API\Repository\FieldTypeService */ | ||
| private $fieldTypeService; | ||
|
|
||
| public function __construct(FieldTypeService $fieldTypeService) |
|
PR updated according to @webhdx @mikadamczyk suggestions. |
|
PR ready for the final code review @mikadamczyk @webhdx |
| <?php | ||
|
|
||
| /** | ||
| * This file is part of the eZ Publish Kernel package. |
There was a problem hiding this comment.
This is not a part of the Kernel package. We can add declare(strict_types=1);
|
|
||
| class SelectionOption extends ValueObject | ||
| { | ||
| public $isDefault; |
There was a problem hiding this comment.
Missing dockblock and an empty line between properties
|
|
||
| EzSystems\RepositoryForms\FieldType\Mapper\ImageAssetFormMapper: | ||
| tags: | ||
| - { name: ez.fieldFormMapper.definition, fieldType: ezimageasset } |
There was a problem hiding this comment.
ez.fieldFormMapper.definition is not needed as the class is not implementing FieldDefinitionFormMapperInterface
Co-authored-by: Dawid Parafiński <[email protected]>
adamwojs
force-pushed
the
EZP-29104-imageasset_clean
branch
from
8457386 to
a503d1e
Compare
Description
Impl. of
\EzSystems\RepositoryForms\FieldType\FieldDefinitionFormMapperInterfaceand\EzSystems\RepositoryForms\FieldType\FieldValueFormMapperInterfacefor Image Asset Field Type.