IBX-6856: Added mime types limitation for ezimage field type #300

ciastektk · 2023-12-01T09:22:26Z

Question	Answer
JIRA issue	IBX-6856
Type	feature
Target Ibexa version	`v4.6`
BC breaks	no

Requires:
ibexa/admin-ui#1021
ibexa/content-forms#55
https://github.com/ibexa/image-editor/pull/80

This PR adds mimeTypes field setting to store which mime types are allowed for Image Field type. If empty array then all image mime types are allowed.

There is also added ibexa.field_type.ezimage.mime_types parameter with predefined mime type list that will be displayed in Image Field type definition.

Checklist:

Provided PR description.
Tested the solution manually.
Provided automated test coverage.
Checked that target branch is set correctly (main for features, the oldest supported for bugs).
Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
Asked for a review (ping @ibexa/engineering).

webhdx · 2023-12-04T10:31:42Z

src/lib/FieldType/Image/Type.php

@@ -36,14 +36,29 @@ class Type extends FieldType implements TranslationContainerInterface
        ],
    ];

+    /** @var array<string, array<array<mixed>|scalar>> */


It would be better to properly define array shape here

Fixed: 2e03387

src/lib/FieldType/Image/Type.php

alongosz · 2023-12-04T12:39:59Z

src/lib/FieldType/Validator.php

@@ -98,7 +99,7 @@ abstract public function validateConstraints($constraints);
     *
     * @return bool
     */
-    abstract public function validate(Value $value);
+    abstract public function validate(Value $value, ?FieldDefinition $fieldDefinition = null);


Tip
This is not an internal class, but it hasn't been documented as an SPI either, so I would just add a note in the upgrade/release notes that this method got an extra parameter (If it were an SPI it would be a breaking change).

alongosz · 2023-12-04T12:43:17Z

src/lib/FieldType/Validator/ImageValidator.php

    {
        // silence `getimagesize` error as extension-wise valid image files might produce it anyway
        // note that file extension checking is done using other validation which should be called before this one
-        if (!@getimagesize($filePath)) {
+        if (!$imageData = @getimagesize($filePath)) {


what kind / types of values causes !$imageData to evaluate to true or false? Can we be always more strict? As a reminder !'0' evaluates to true and yet it is some sort of data ;)

Suggested change

if (!$imageData = @getimagesize($filePath)) {

$imageData = @getimagesize($filePath);

if ($imageData !== false) {

Negation of variable assignment inside of condition can be confusing and less readable expression. It is better to avoid it in a first place.

Fixed: 29a2224#diff-2ff5479b5225a4941f470460612e38060102d4f86b637f2537df1aebc1192840

alongosz · 2023-12-04T12:48:58Z

src/lib/Persistence/Legacy/Content/FieldValue/Converter/ImageConverter.php

            ]
        );
    }

+    /**
+     * @return array<string>


either I'm reading the method wrong, or this is not correct. I wonder why we don't have anything on this from PHPStan. In pure playground environment it reports here an issue.

Fixed: 29a2224#diff-c689fe99fd23e7ed77c884abee4541ecf3d013a6cca3b52e9bd2059ed9fb11a9R213

Nattfarinn · 2023-12-04T13:24:33Z

src/lib/FieldType/Validator/ImageValidator.php

    {
        // silence `getimagesize` error as extension-wise valid image files might produce it anyway
        // note that file extension checking is done using other validation which should be called before this one
-        if (!@getimagesize($filePath)) {
+        if (!$imageData = @getimagesize($filePath)) {


Suggested change

if (!$imageData = @getimagesize($filePath)) {

$imageData = @getimagesize($filePath);

if ($imageData !== false) {

Negation of variable assignment inside of condition can be confusing and less readable expression. It is better to avoid it in a first place.

src/lib/FieldType/Image/Type.php

tests/integration/Core/Repository/FieldType/ImageIntegrationTest.php

Co-authored-by: Maciej Kobus <[email protected]>

sonarqubecloud · 2023-12-08T07:16:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.6% Duplication

ciastektk mentioned this pull request Dec 1, 2023

IBX-6856: Added mime types limitation for ezimage field type #299

Closed

6 tasks

github-actions bot assigned alongosz, kisztof and mikadamczyk Dec 1, 2023

ciastektk marked this pull request as ready for review December 1, 2023 11:37

ciastektk requested a review from a team December 1, 2023 11:44

ciastektk added Feature New feature request Ready for review labels Dec 1, 2023

This was referenced Dec 1, 2023

IBX-6856: Added view variable mime types for ezimage field type ibexa/content-forms#55

Merged

IBX-6856: Added mimeTypes field to ImageFormMapper ibexa/admin-ui#1021

Merged

webhdx approved these changes Dec 4, 2023

View reviewed changes

webhdx requested a review from a team December 4, 2023 10:36

alongosz reviewed Dec 4, 2023

View reviewed changes

Nattfarinn approved these changes Dec 4, 2023

View reviewed changes

konradoboza reviewed Dec 4, 2023

View reviewed changes

src/lib/FieldType/Image/Type.php Show resolved Hide resolved

tests/integration/Core/Repository/FieldType/ImageIntegrationTest.php Outdated Show resolved Hide resolved

ciastektk requested review from alongosz and konradoboza December 5, 2023 07:55

konradoboza approved these changes Dec 5, 2023

View reviewed changes

tests/integration/Core/Repository/FieldType/ImageIntegrationTest.php Outdated Show resolved Hide resolved

tests/integration/Core/Repository/FieldType/ImageIntegrationTest.php Outdated Show resolved Hide resolved

ciastektk force-pushed the ibx-6856-added-mime-types-limitation-for-ezimage-field-type branch from 32ecebe to c44b948 Compare December 5, 2023 10:15

ciastektk added Ready for QA and removed Ready for review labels Dec 6, 2023

Base automatically changed from ibx-6937-changed-max-file-size-value-to-float to main December 6, 2023 08:41

ciastektk and others added 8 commits December 6, 2023 10:00

Added optional argument FieldDefinition to FieldType\Validator::validate

5795694

Added mimeTypes fieldSetting

eeea58c

Added container parameter with default mime type list

02c1073

[Tests] Adjusted tests

848f231

[PHPStan] Regenerated baseline

b7c823b

Update src/lib/FieldType/Image/Type.php

5d0ddb8

Co-authored-by: Maciej Kobus <[email protected]>

Fixed phpdoc for settingSchema

e68ae38

Fixes after review

0ee455b

Fixed return types in ImageIntegrationTest

6cd0d98

ciastektk force-pushed the ibx-6856-added-mime-types-limitation-for-ezimage-field-type branch from c44b948 to 6cd0d98 Compare December 6, 2023 09:01

Changed mimeTypes argument as optional

93eaa4a

mnocon approved these changes Dec 14, 2023

View reviewed changes

webhdx merged commit 3fd56c6 into main Dec 14, 2023
22 checks passed

webhdx deleted the ibx-6856-added-mime-types-limitation-for-ezimage-field-type branch December 14, 2023 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-6856: Added mime types limitation for ezimage field type #300

IBX-6856: Added mime types limitation for ezimage field type #300

ciastektk commented Dec 1, 2023 •

edited

Loading

webhdx Dec 4, 2023

ciastektk Dec 4, 2023

alongosz Dec 4, 2023 •

edited

Loading

alongosz Dec 4, 2023

Nattfarinn Dec 4, 2023

ciastektk Dec 5, 2023

alongosz Dec 4, 2023

ciastektk Dec 5, 2023

Nattfarinn Dec 4, 2023

sonarqubecloud bot commented Dec 8, 2023

	if (!$imageData = @getimagesize($filePath)) {
	$imageData = @getimagesize($filePath);
	if ($imageData !== false) {

IBX-6856: Added mime types limitation for ezimage field type #300

IBX-6856: Added mime types limitation for ezimage field type #300

Conversation

ciastektk commented Dec 1, 2023 • edited Loading

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongosz Dec 4, 2023 • edited Loading

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

sonarqubecloud bot commented Dec 8, 2023

ciastektk commented Dec 1, 2023 •

edited

Loading

alongosz Dec 4, 2023 •

edited

Loading