Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-6937: Changed maxFileSize value to float type for Image Field type #291

Merged
merged 12 commits into from
Dec 6, 2023

Conversation

ciastektk
Copy link
Contributor

@ciastektk ciastektk commented Oct 31, 2023

Question Answer
JIRA issue IBX-6937
Type improvement
Target Ibexa version v4.6
BC breaks no
Requires ibexa/admin-ui#966 & https://github.com/ibexa/installer/pull/131

This PR changes max file size value from integer to float type for Image field type. Cleandata schema is also updated to store float value in dataFloat1 column instead of dataInt1. There is also added additional info to dataText1 column that contains proper size unit set as MB.

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).

@ciastektk ciastektk force-pushed the ibx-6937-changed-max-file-size-value-to-float branch 3 times, most recently from dabd3c5 to e144f58 Compare November 2, 2023 11:21
@ciastektk ciastektk marked this pull request as ready for review November 2, 2023 11:38
@ciastektk ciastektk requested a review from a team November 2, 2023 11:58
@ciastektk ciastektk force-pushed the ibx-6937-changed-max-file-size-value-to-float branch from e144f58 to 83fc044 Compare November 2, 2023 12:41
@adamwojs
Copy link
Member

adamwojs commented Nov 2, 2023

Why is exact reason to change data type from int to type ?

@ciastektk
Copy link
Contributor Author

ciastektk commented Nov 3, 2023

Why is exact reason to change data type from int to type ?

One of our client's requirements is to set max image size to 1.5 MB in DAM .

Copy link
Contributor

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is failing for me when I reinstall the database, the failure is:

In MigrationExecutor.php line 68:

  Executing migration "2023_01_05_13_00_create_customer_content_type.yml" failed. Exception: Unable to add field `image` to Content Type `customer`.


In ContentTypeCreateStepExecutor.php line 168:

  Unable to add field `image` to Content Type `customer`


In ContentTypeService.php line 1303:

  Content Type Field definitions did not validate

This migration contains the following fragment:

            validatorConfiguration:
                FileSizeValidator:
                    maxFileSize: 10
                AlternativeTextValidator:
                    required: false

I dug a bit deeper and the validation error is:

array(1) {
  [0]=>
  object(Ibexa\Core\FieldType\ValidationError)#22454 (4) {
    ["singular":protected]=>
    string(68) "Validator %validator% expects parameter %parameter% to be of %type%."
    ["plural":protected]=>
    NULL
    ["values":protected]=>
    array(3) {
      ["%validator%"]=>
      string(17) "FileSizeValidator"
      ["%parameter%"]=>
      string(11) "maxFileSize"
      ["%type%"]=>
      string(5) "float"
    }
    ["target":protected]=>
    string(32) "[FileSizeValidator][maxFileSize]"
  }
}

Is it possible that we have to change something in migrations as well?

@webhdx webhdx force-pushed the ibx-6937-changed-max-file-size-value-to-float branch from 83fc044 to 22d56db Compare November 22, 2023 07:36
Copy link
Contributor

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All works well on MySQL.

On PostgreSQL it's not possible to specify decimal fractions other than 0.5 - for example entering 0.1 ends up with the following error:

[2023-11-27T19:03:43.588031+00:00] app.ERROR: An error has occurred while handling form submission: Database error {"exception":"[object] (Doctrine\\DBAL\\Exception\\DriverException(code: 0): An exception occurred while executing 'UPDATE ezcontentclass_attribute SET serialized_name_list = ?, serialized_description_list = ?, serialized_data_text = ?, identifier = ?, category = ?, placement = ?, data_type_string = ?, can_translate = ?, is_thumbnail = ?, is_required = ?, is_information_collector = ?, is_searchable = ?, data_float1 = ?, data_float2 = ?, data_float3 = ?, data_float4 = ?, data_int1 = ?, data_int2 = ?, data_int3 = ?, data_int4 = ?, data_text1 = ?, data_text2 = ?, data_text3 = ?, data_text4 = ?, data_text5 = ? WHERE (id = ?) AND (version = ?)' with params [\"a:1:{s:6:\\\"eng-GB\\\";s:14:\\\"New field type\\\";}\", \"a:1:{s:6:\\\"eng-GB\\\";N;}\", \"N;\", \"field_6564b6e80e9dc\", \"content\", 3, \"ezimage\", 1, false, 0, 0, 1, null, null, null, null, 102.4, 0, null, null, \"KB\", null, null, null, null, 243, 1]:\n\nSQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for integer: \"102.4\" at /Users/mareknocon/Desktop/Sites/v4_4/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractPostgreSQLDriver.php:102)\n[previous exception] [object] (Doctrine\\DBAL\\Driver\\PDO\\Exception(code: 22P02): SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for integer: \"102.4\" at /Users/mareknocon/Desktop/Sites/v4_4/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDO/Exception.php:18)\n[previous exception] [object] (PDOException(code: 22P02): SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for integer: \"102.4\" at /Users/mareknocon/Desktop/Sites/v4_4/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:117)"} []
 invalid input syntax for integer: \"102.4\"

(102.4 = 0.1 * 1024)

It works with 0.5 because the result is an integer

@webhdx webhdx requested a review from a team December 1, 2023 08:09
src/lib/FieldType/Image/Type.php Outdated Show resolved Hide resolved
data/mysql/cleandata.sql Show resolved Hide resolved
@alongosz alongosz requested a review from a team December 1, 2023 17:12
Copy link

sonarqubecloud bot commented Dec 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ciastektk ciastektk requested a review from mnocon December 4, 2023 11:25
Copy link
Contributor

@mnocon mnocon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now!

@alongosz alongosz merged commit 1044c97 into main Dec 6, 2023
22 checks passed
@alongosz alongosz deleted the ibx-6937-changed-max-file-size-value-to-float branch December 6, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants