drupal-graphql / graphql

GraphQL integration for Drupal 9/10
286 stars 202 forks source link

feat(utility/fileupload): file_validate_image_resolution #1342

Closed aurelianzaha closed 1 year ago

aurelianzaha commented 1 year ago

make it possible to validate the image resolution

file size and file extension validation is already in place

klausi commented 1 year ago

Hm, but will this change be enough? getUploadValidators() is called only once, then the $validator array is passed to prepareFilename(), but it is never looped over?

Can you add a test case to UploadFileServiceTest?

We copied the code from Drupal core FileItem, how does an image file item handle this? I guess it is done in ImageWidget, that's why it is missing.

klausi commented 1 year ago

Another problem is that you cannot use file_validate_image_resolution() fully because it invokes the drupal message system when changing the scale of an image. That is not ideal, as it starts a user session that you don't need.

So probably you will need a copy of file_validate_image_resolution() as method in our file upload service.

aurelianzaha commented 1 year ago

Hm, but will this change be enough? getUploadValidators() is called only once, then the $validator array is passed to prepareFilename(), but it is never looped over?

Can you add a test case to UploadFileServiceTest?

We copied the code from Drupal core FileItem, how does an image file item handle this? I guess it is done in ImageWidget, that's why it is missing.

@klausi yes, that is enough, the $validators array is used here https://github.com/drupal-graphql/graphql/blob/8.x-4.x/src/GraphQL/Utility/FileUpload.php#L261 this is where the loop happens

aurelianzaha commented 1 year ago

@klausi I made a copy of file_validate_image_resolution do we still need the tests ?