add base64 upload #495

mandricmihai · 2018-08-06T18:35:01Z

Settings to use it

 'pathProcessor' => Base64Processor::class,
 'base64_extension' => '.jpg',
 'transformer' => Base64Transformer::class,
 'uploadValidator' => Base64UploadValidator::class,

@josegonzalez suggestions on how I could get rid of is_string() and isset()
On base64 we just get a long string of data with no filename, no type, no size.
Type and size column will also not be set.

josegonzalez · 2018-08-06T18:44:16Z

Needs tests before I'll review, sorry.

josegonzalez · 2018-08-06T18:44:42Z

src/File/Path/Base64Processor.php

@@ -0,0 +1,39 @@
+<?php
+/**


These docblocks shouldn't be here.

mandricmihai · 2018-08-07T19:51:33Z

@josegonzalez I added unit tests

mandricmihai · 2018-08-08T10:10:19Z

Looks like some code style fails. I will fix it later today.

mandricmihai · 2018-08-08T18:11:11Z

@josegonzalez could you review, please?

josegonzalez · 2018-08-08T21:30:42Z

src/Model/Behavior/UploadBehavior.php

+                $temp = [];
+                $temp['name'] = $filename;
+                $temp['data'] = $data;
+                $data = $temp;


What is the purpose of this?

When upload base64 $data is just a string f.ex
Y2FrZXBocA==
It's not an array and doesn't have an name.
Could you think about a better solution to this problem?
Here is the same thing https://github.com/FriendsOfCake/cakephp-upload/pull/495/files#diff-84c66ea1e7013949dabde1157612da57R122
$data['size'] and $data['type'] are not set when upload base64, when doing regular upload they are already provided by php.
Maybe implement another 2 interface?
The if's are there just to detect that were trying to upload base64

Also because of the https://github.com/FriendsOfCake/cakephp-upload/pull/495/files#diff-84c66ea1e7013949dabde1157612da57R122
When uploading base64 we can't use the size and type features to have them set in the database

Can you show how this would be used in practice? It's not clear whats going on here.

You mean with the 2 extra interfaces?

$data = $entity->get($field); $path = $this->getPathProcessor($entity, $data, $field, $settings); $basepath = $path->basepath(); $filename = $path->filename(); $dataFormatter = $this->getDataFormatter($field, $filename); $data = $dataFormatter->formatData(); $files = $this->constructFiles($entity, $data, $field, $settings, $basepath); $writer = $this->getWriter($entity, $data, $field, $settings); $success = $writer->write($files); if ((new Collection($success))->contains(false)) { return false; } $entity->set($field, $filename); $metaDataProcessor = $this->getGetMetaDataProcessor($files); $entity->set(Hash::get($settings, 'fields.dir', 'dir'), $basepath); $entity->set(Hash::get($settings, 'fields.size', 'size'), $metaDataProcessor->getSize()); $entity->set(Hash::get($settings, 'fields.type', 'type'), $metaDataProcessor->getType());

Something like this.Maybe with some better names 😄

Maybe you could come with something better?I tried to think of a design pattern to apply, but I could only think of template or strategy but is pretty much like what we already have with the interfaces in place

I meant in terms of an end user. How are you planning on using this/why should this go into the core?

I'm using https://foliotek.github.io/Croppie/ so that the user can select his profile picture and the result is in base64.

josegonzalez · 2018-08-08T21:31:16Z

src/Model/Behavior/UploadBehavior.php

@@ -87,7 +87,8 @@ public function beforeSave(Event $event, Entity $entity, ArrayObject $options)
                continue;
            }

-            if (Hash::get((array)$entity->get($field), 'error') !== UPLOAD_ERR_OK) {
+            $uploadValidator = $this->getUploadValidator($entity, $settings, $field);
+            if ($uploadValidator->hasUploadFailed()) {


davidyell · 2018-08-10T11:11:38Z

I'm not quite sure why this functionality needs to be added to the core? Who will want to use this?

To me this is an edge-case and could be implemented with custom classes in userland code base.

mandricmihai · 2018-08-10T14:05:24Z

I think you are right.In this case, we can close the pull.

josegonzalez · 2018-08-10T21:16:47Z

Could be useful. Can we get documentation for this?

mandricmihai · 2018-08-11T05:52:28Z

@josegonzalez before getting the doc for this, we should decide what to do with the type and the size, right now they are not supported and if we are ok with https://github.com/FriendsOfCake/cakephp-upload/pull/495/files#diff-84c66ea1e7013949dabde1157612da57R103

jorisvaesen · 2018-08-11T09:01:35Z

After some googling

Filesize:

strlen(base64_decode($data));

Filetype:

$f = finfo_open();
$mime_type = finfo_buffer($f, $data, FILEINFO_MIME_TYPE);

josegonzalez · 2018-08-12T01:05:57Z

Those seem reasonable to me @jorisvaesen.

mandricmihai · 2018-08-12T08:32:13Z

The problem is not how to determine the file size and type, is where do we put that code that does this?

josegonzalez · 2018-08-13T00:18:56Z

We could have a FileProcessor or something that represents how to get that data for each type of file upload?

mandricmihai · 2018-08-13T05:51:19Z

That will solve one problem.
How about this if https://github.com/FriendsOfCake/cakephp-upload/pull/495/files#diff-84c66ea1e7013949dabde1157612da57R103
Are you ok with having this if or is better to make another interface?

josegonzalez · 2018-08-13T14:39:46Z

@mma can you add actual usage docs for this so I can see how we plan on presenting it's use for users? That'll give me a better idea of what the internals should look like.

jorisvaesen · 2018-08-13T16:31:22Z

Adding a FileProcessor interface and a FileProcessor option to the config seems to me the right thing to do. Just make sure that this config option allows a callback function so an user can return the right processor based on the upload (base64, php file upload, ...)

mandricmihai · 2018-08-13T17:59:13Z

@josegonzalez
Sure, I will add the docs this week.

ADmad · 2018-08-15T16:07:28Z

tests/TestCase/File/Transformer/Base64TransformerTest.php

+    {
+        $this->entity = $this->getMockBuilder('Cake\ORM\Entity')->getMock();
+        $this->table = $this->getMockBuilder('Cake\ORM\Table')->getMock();
+        $this->data = ['data' => 'Y2FrZXBocA==', 'name' => '5a2e69ff-c2c0-44c1-94a7-d791202f0067.txt'];


We should keep in mind that using a custom array format like this makes validation tricker. Most validation methods for uploaded files won't be usable. I haven't reviewed the full PR but you need to ensure proper validation is done for this array format and someone can't sneak in whatever they want using this feature.

ADmad · 2018-08-15T16:15:26Z

src/UploadValidator/Base64UploadValidator.php

+     */
+    public function hasUploadFailed()
+    {
+        return !base64_decode($this->entity->get($this->field), true);


How can one do other validations like for e.g checking for particular mimetype / file type?

Ah I now see the comments regarding adding methods/interfaces for checking size and mime type.

I don't think is possible to do check on mimetype / file type without having the file already written on the disk

Wouldn't it be possible having validation methods using the example code provided in this comment #495 (comment) ?

I just tried and it works. I wasn't aware of such method to determine the mimetype.

ADmad · 2018-08-15T16:25:15Z

The problem is not how to determine the file size and type, is where do we put that code that does this?

It should be in validation methods shouldn't it?

mandricmihai · 2018-08-15T16:32:40Z

I don't think it should be in validation.We want to use the methods to get the actual size and file type https://github.com/FriendsOfCake/cakephp-upload/pull/495/files#diff-84c66ea1e7013949dabde1157612da57R123
so that we can set on the entity.

ADmad · 2018-08-15T16:41:29Z

Just FYI, how I provide cropping feature is use js code to just generate the cropping dimensions and submit it in a separate field without touching the actual upload file. Then use the cropping data to crop on server side.

Only drawback of this way is the full image is uploaded but in return it saves all the extra code for handing base64 encoded data and more importantly upload validation.

ADmad · 2018-08-15T16:44:06Z

I don't think it should be in validation.

If not in validation then how can I prevent someone from submitting a non image base64 encoded data?

mandricmihai · 2018-08-15T16:45:12Z

We can provided a method in ImageValidationTrait.php that uses https://secure.php.net/manual/en/function.imagecreatefromstring.php

ADmad · 2018-08-15T16:52:10Z

Unless cake's validator can be used to validate the mime type of base64 encoded data before any other processing begins, I am a big "NO" on adding this feature to the plugin.

mandricmihai · 2018-08-15T17:12:31Z

When you say cake's validators you talk about the validators already provided by cake or the custom validators?

ADmad · 2018-08-15T17:19:01Z

Custom validation methods which will be used by Cake's Validator 🙂

mandricmihai · 2018-08-22T18:39:32Z

@ADmad could you take a look over the validator when you have some time?

boyteenphonui · 2018-08-30T15:03:09Z

It is very useful, I also really need it. I am very supportive of this.

AlexAlexandru added 3 commits August 6, 2018 21:27

add base64 upload

c910542

newline

bf8499d

newline

6c83c5c

josegonzalez reviewed Aug 6, 2018

View reviewed changes

src/File/Path/Base64Processor.php Outdated

@@ -0,0 +1,39 @@

<?php

/**

Copy link

Member

josegonzalez Aug 6, 2018

Choose a reason for hiding this comment

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

These docblocks shouldn't be here.

AlexAlexandru added 2 commits August 7, 2018 22:31

added unit tests

a0573c0

fix type from rename

af00b22

fixes code sniffer errors

7777cb5

josegonzalez reviewed Aug 8, 2018

View reviewed changes

AlexAlexandru added 4 commits August 9, 2018 20:03

UploadBehaviour for base64 is missing, I should add it

0919441

I was talking about the UploadBehaviorTest for base64 that is missing

4fafa7a

code style

8a6e439

FIX STYL

889215f

ADmad reviewed Aug 15, 2018

View reviewed changes

AlexAlexandru added 2 commits August 22, 2018 21:01

added base64 validation

5468717

code style

8ceb373

add base64 upload #495

Are you sure you want to change the base?

add base64 upload #495

Conversation

mandricmihai commented Aug 6, 2018 • edited Loading

josegonzalez commented Aug 6, 2018

Choose a reason for hiding this comment

mandricmihai commented Aug 7, 2018

mandricmihai commented Aug 8, 2018

mandricmihai commented Aug 8, 2018

Choose a reason for hiding this comment

mandricmihai Aug 9, 2018 • 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

mandricmihai Aug 9, 2018 • 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

davidyell commented Aug 10, 2018 • edited Loading

mandricmihai commented Aug 10, 2018 • edited Loading

josegonzalez commented Aug 10, 2018

mandricmihai commented Aug 11, 2018 • edited Loading

jorisvaesen commented Aug 11, 2018

josegonzalez commented Aug 12, 2018

mandricmihai commented Aug 12, 2018

josegonzalez commented Aug 13, 2018

mandricmihai commented Aug 13, 2018 • edited Loading

josegonzalez commented Aug 13, 2018

jorisvaesen commented Aug 13, 2018 • edited Loading

mandricmihai commented Aug 13, 2018

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

ADmad Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADmad commented Aug 15, 2018 • edited Loading

mandricmihai commented Aug 15, 2018 • edited Loading

ADmad commented Aug 15, 2018 • edited Loading

ADmad commented Aug 15, 2018 • edited Loading

mandricmihai commented Aug 15, 2018

ADmad commented Aug 15, 2018

mandricmihai commented Aug 15, 2018

ADmad commented Aug 15, 2018

mandricmihai commented Aug 22, 2018

boyteenphonui commented Aug 30, 2018

mandricmihai commented Aug 6, 2018 •

edited

Loading

mandricmihai Aug 9, 2018 •

edited

Loading

mandricmihai Aug 9, 2018 •

edited

Loading

davidyell commented Aug 10, 2018 •

edited

Loading

mandricmihai commented Aug 10, 2018 •

edited

Loading

mandricmihai commented Aug 11, 2018 •

edited

Loading

mandricmihai commented Aug 13, 2018 •

edited

Loading

jorisvaesen commented Aug 13, 2018 •

edited

Loading

ADmad Aug 15, 2018 •

edited

Loading

ADmad commented Aug 15, 2018 •

edited

Loading

mandricmihai commented Aug 15, 2018 •

edited

Loading

ADmad commented Aug 15, 2018 •

edited

Loading

ADmad commented Aug 15, 2018 •

edited

Loading