Problem/Motivation

File entities and file/image fields do not currently work with the new REST GET/POST/PATCH/PUT operations. There is currently no way to create files by providing the actual data.

Proposed resolution

The plan is to provide a separate endpoint that is only for the creation of file entities. This endpoint will only accept binary data (application/octet-stream), not any file entity representation, although a file entity will be created from this data. Large amounts of data can then be streamed in a request instead. These file entities will have a status of false, i.e. they are not in a permanent state. The file uploads should be validated within the context of a file field, as we are currently restricted to this approach in Drupal - as files are not really stand alone first-class entities. So all appropriate file entity validations configured for the file field context being used (like size, extension) will be running.

A separate request can then be made to reference the created file entity from a parent entity. When the entity is updated or created that will in turn make the file permanent.

Read about how to POST aka create entities https://www.drupal.org/node/2098511

More documentation about REST: https://www.drupal.org/documentation/modules/rest

Remaining tasks

- Decide on which approach should be used to determine the name of the file. header or query string parameter I think are the options here. We need to decide on naming etc.. for this though
- Fix access checking
- Fix test failures (access checking related too)
- Decide if we should remove the current, broken and probably insecure behaviour (I think we have to really, at least for denormalization)
- Reviews

User interface changes

N/A

API changes

Addition of a file/upload/{entity_type}/{bundle}/{field_name} endpoint that binary file data can be POSTED/streamed to.

Original report by @moshe weitzman

Files: 
CommentFileSizeAuthor
#395 interdiff-1927648-395.txt11.79 KBdamiankloip
#395 1927648-395.patch45.26 KBdamiankloip
#394 interdiff-1927648-394.txt4.02 KBdamiankloip
#394 1927648-394.patch38.45 KBdamiankloip
#392 1927648-392.patch35.16 KBWim Leers
#379 1927648-379.patch29.7 KBpnagornyak
#366 interdiff-1927648-366.txt564 bytesdamiankloip
#366 1927648-366.patch33.88 KBdamiankloip
#364 interdiff-1927648-364.txt3.7 KBdamiankloip
#364 1927648-364.patch33.85 KBdamiankloip
#359 interdiff-1927648-359.txt6.66 KBdamiankloip
#359 1927648-359.patch31.65 KBdamiankloip
#356 interdiff-1927648-356.txt2.69 KBdamiankloip
#356 1927648-356.patch26.85 KBdamiankloip
#355 interdiff-1927648-354.txt12.39 KBdamiankloip
#355 1927648-354.patch26.57 KBdamiankloip
#350 interdiff-1927648-350.txt9.02 KBdamiankloip
#350 1927648-350.patch22.15 KBdamiankloip
#344 interdiff-1927648-343.txt6.81 KBdamiankloip
#344 1927648-343.patch23.02 KBdamiankloip
#331 interdiff-1927648-331.txt1.28 KBdamiankloip
#331 1927648-331.patch19.54 KBdamiankloip
#330 interdiff-1927648-329.txt14.11 KBdamiankloip
#330 1927648-329.patch19.46 KBdamiankloip
#322 interdiff-1927648-322.txt5.72 KBibustos
#322 1927648-322.patch17.72 KBibustos
#310 interdiff-1927648-309.txt1.88 KBibustos
#309 1927648-309.patch15.81 KBibustos

Comments

linclark’s picture

We had discussed creating this as a temp file stream and then passing that off to the field. When you create a temp file, is there any way it is accessible via HTTP? If so, I believe it could open up a vulnerability where a malicious script could be base64 encoded and then run. I don't know enough about the file system to be sure.

moshe weitzman’s picture

It would be poor practice but I do think some sites put their temp dir within the docroot. So, your worry is well founded. I debugged our current file upload validation and the key function we have to mimic is file_save_upload. We have to mimic it because it is hard coded to work with uploads (i.e. the $_FILES superglobal).

The key bits from that function are

$file = entity_create($values);
$validators = file_field_widget_upload_validators($field, $instance);
$errors = file_validate($file, $validators);

Unfortunately, this function seems to do a bit of validation of its own (file_validate_extensions, file_munge_filename(), file_validate_name_length) that are not contained within file_validate() call. As a first pass, I would just ignore this extra validation and rely on the file_validate() call.

Also, you will want a stream wrapper which is inline data, so you don't have to save the file before calling file_validate. See the data:// wrapper at http://php.net/manual/en/wrappers.data.php

moshe weitzman’s picture

Issue tags: +WSCCI

Add tag

linclark’s picture

We discussed this on the REST call today.

I'm pretty sure that image fields are basically a special kind of entity reference, which reference a file entity. Thus, the URI should be a property of the file entity, which means that we can handle the relationship between the file entity and the referencing entity could be handled in the same way.

linclark’s picture

We discussed this with quicksketch on one of the calls. He agreed that the API interaction should be to create the file entity and then use the entity id from that to make another call to create the node that points to it.

To do this, we need Entity API support for files, #1818568: Convert files to the new Entity Field API.

linclark’s picture

Because supporting this in serialization is a separate issue, independent of REST, I created #1988426: Support deserialization of file entities in HAL.

For now, we should be able to put in a stub Normalizer that handles the current File class (which is not NG). This approach can be re-evaluated once File in NG.

moshe weitzman’s picture

Issue summary: View changes

Still a gaping hole in our REST support. Come on Internets ...

rcaracaus’s picture

I managed to get this working on GET, sort of.. but it was a hack. What are the next steps to properly get this into core now that the blockers above are resolved?

rteijeiro’s picture

Hi @rcaracaus,

How did you manage to get this working? I'm getting this output:

field_image: [
  {
    target_id: "1",
    display: null,
    description: null,
    alt: "",
    title: "",
    width: "1134",
    height: "1134"
  }
]

I'm not sure if I have to make a new request for image entity or something else in order to get the image url, for example.

webchick’s picture

Priority: Normal » Major

Seems like this should be elevated, given the impact.

rteijeiro’s picture

I'm pretty interested in make this work. If someone can point me how to start working on this (related issues or something else) I will be pleased to contribute :)

clemens.tolboom’s picture

@rteijeiro we do have our admin/help/rest which goes to https://www.drupal.org/documentation/modules/rest and in particular https://www.drupal.org/node/2098511 (POST content)

As pointed out in #0 and #2 checkout devel_generate in particular DevelGenerateFieldImage.

I use UIs on Chrome: Rest console and DHC - REST HTTP API Client but prefer command-line scripts

[Stock response from Dreditor templates and macros.]

Please update the issue summary by applying the template from http://drupal.org/node/1155816.

rteijeiro’s picture

Issue summary: View changes
rteijeiro’s picture

Issue summary: View changes
webchick’s picture

Went to look into this tonight, but discovered that atm Devel Generate's image generation is broken: #2294283: Generate images is broken So can't really use that as a model.

juampynr’s picture

Status: Active » Needs review
FileSize
1.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,412 pass(es), 1 fail(s), and 0 exception(s). View

Here is a first stab at this.

Given the following:

  • Page content type has a field_image in it.
  • Rest and Hal modules enabled.
  • POST method for nodes supported by cookie authentication in REST settings.
  • Anonymous users with permission to access POST for content nodes, create Page content types and Create URL aliases

And the following Guzzle 3 request:

<?php

/**
 * @file
 *
 * Performs a request to create a page node 
 */

require 'vendor/autoload.php';

use Guzzle\Http\Client;
use Guzzle\Plugin\Cookie\CookiePlugin;
use Guzzle\Plugin\Cookie\CookieJar\ArrayCookieJar;

$cookiePlugin = new CookiePlugin(new ArrayCookieJar());

$client = new Client('http://d8.local');
$client->addSubscriber($cookiePlugin);

// Encode the image.
$image = base64_encode(file_get_contents('/home/juampy/Pictures/sample.jpg'));

// Create the array of data to post.
$node = array(
  '_links' => array(
    'type' => array(
      'href' => 'http://d8.local/rest/type/node/page'
    )
  ),
  'title' => array(0 => array(
    'value' => 'New node title ' . rand(1, 500),
  )),
  'field_image' => array(0 => array(
    'value' => $image,
    'filename' => 'sample.jpg',
  )),
);
$data = json_encode($node);

// Perform the actual request.
$request = $client->post('entity/node', array(
    'Content-type' => 'application/hal+json',
), $data);
$response = $request->send();

// Print out results.
if ($response->getStatusCode() == 201) {
  print 'Node creation successful!';
}
else {
  print_r($response);
}

With the attached patch image fields get populated. Any feedback is welcome. I will start adding testing coverage.

clemens.tolboom’s picture

I just created a patch for GET in #2310307: File needs CRUD permissions to make REST work on entity/file/{id} so revisit this issue.

The title for this issue is weird as it suggest all 'REST operations'. This seems only the be about POST a parent content type _containing_ an image. Please update the summary :)

Reading the patch and issue comments what's the plan for ie a PDF file aka non-images?

The patch misses handling for image.alt and image.title

In #2 it seems we are coding around the validators. Is that still the biggest problem?

A side note: in #1925618: Ensure Drupal's web services are self-documenting: Swagger support OR rest_api_doc to Drupal core as an experimental module? I try to build the API docs by digging into the exposed entity/bundle/field features. How can I get to (lookup) this new normalizer in context of json and hal+json?

Berdir’s picture

Yes, should work for all files. image/file references are both entity references, it would make a lot more sense to treat this like any other reference I think, so we would add this base64 magic on the file resource, so you'd upload your file first, and then reference it using the UUID in the image/file field? You could also have entity_reference fields pointing to files, for example.

juampynr’s picture

Thanks for the feedback! I will give it another go and check out what's going on on the issues that @clemens.tolboom mentioned.

clemens.tolboom’s picture

  1. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemImageNormalizer.php
    @@ -0,0 +1,33 @@
    +      $filename = rand(1, 500) . $data['filename'];
    

    Why rand() call?

  2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemImageNormalizer.php
    @@ -0,0 +1,33 @@
    +      $filename = rand(1, 500) . $data['filename'];
    +      $image = file_save_data($image, 'public://' . $filename);
    

    What about filename transliteration?

  3. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemImageNormalizer.php
    @@ -0,0 +1,33 @@
    +      $image = file_save_data($image, 'public://' . $filename);
    

    This should not always be public://

juampynr’s picture

Status: Needs work » Needs review

Addressed @clemens.tolboom's suggestions:

* Removed rand() statement.
* Filename is now transliterated.
* Now reading field insance's settings in order to determine where to save the file.

juampynr’s picture

FileSize
1.78 KB
2.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,263 pass(es), 1 fail(s), and 0 exception(s). View

Oopsie. Here are the files.

juampynr’s picture

Next, I will address suggestions in #18 and #19.

juampynr’s picture

Title: Imagefields do not support REST operations » File fields do not support REST operations
Issue summary: View changes

Upgrading scope so it covers file fields and not just image fields.

clemens.tolboom’s picture

Title: File fields do not support REST operations » base64 File field content to support REST POST and PATCH operations
Issue summary: View changes
Arla’s picture

Based on the latest patch, I have just implemented something like this for the File entity module. I will make it a patch for core shortly and post it here.

juampynr’s picture

Status: Needs work » Needs review
FileSize
2.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,805 pass(es), 1 fail(s), and 0 exception(s). View
4.27 KB

Here it is :D

Next step, I will create another one that inherit from this normalizer for Images.

juampynr’s picture

Here is the sample script that I have been using. The readline() statement is a sneaky trick to be able to trigger XDebug for the Drupal request and not this script:

<?php

/**
 * @file
 *
 * Simple request to create a node with a file field in it.
 */

require 'vendor/autoload.php';

use Guzzle\Http\Client;
use Guzzle\Plugin\Cookie\CookiePlugin;
use Guzzle\Plugin\Cookie\CookieJar\ArrayCookieJar;

readline();
$cookiePlugin = new CookiePlugin(new ArrayCookieJar());

$client = new Client('http://d8.local');
$client->addSubscriber($cookiePlugin);
$image = base64_encode(file_get_contents('/home/juampy/test.txt'));
$node = array(
  '_links' => array(
    'type' => array(
      'href' => 'http://d8.local/rest/type/node/page'
    )
  ),
  'title' => array(0 => array(
    'value' => 'New node title ' . rand(1, 500),
  )),
  'field_file' => array(0 => array(
    'value' => $image,
    'filename' => 'test.txt',
  )),
);
$data = json_encode($node);

$request = $client->post('entity/node', array(
    'Content-type' => 'application/hal+json',
  ), $data);
$request->getQuery()->set('XDEBUG_SESSION_START', '1');

echo "{$request->getQuery()}\n";

$response = $request->send();
if ($response->getStatusCode() == 201) {
  print 'Node creation successful!';
}
else {
  print_r($response);
}
joshk’s picture

Is there a separate issue for handling the GET side of things? I'm working on a D8 + Angular demo and getting rest responses back (read only even) with image references would make it approximately 1100% hotter.

Berdir’s picture

The patch that @Arla is working on makes file/image behave like other entity references (with a UUID reference) and moves the support for passing along the file content as base64 to GET/POST of files, so that should include GET yes.

clemens.tolboom’s picture

@berdir where is that issue patch done by @arla as that sound like a way better solution then this :)

Arla’s picture

Status: Needs work » Needs review
FileSize
15.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,282 pass(es), 3 fail(s), and 5 exception(s). View

So here's the patch.

I'm not sure how image fields are affected by these changes.

I got lots of help from @Berdir.

clemens.tolboom’s picture

@Arla please add some hints on how to test this manually. I assume you have some code lying around :)

Arla’s picture

I have just been running the hal kernel tests EntityTest, FileNormalizeTest and FileFieldNormalizeTest. I suppose REST requests will work as expected assuming the normalization is correct.

Berdir’s picture

It works with the same principle as the other patch, but you submit the base64 data first by creating a file entity, then you get a UUID (or you submit it already) and then create a node where you reference the file through that UUID. Just like you would create a node with author, or a node with terms.

clemens.tolboom’s picture

@Arla @Berdir I expected a non base64 solution but misread. So File entity supports temporary files now seemed a problem mentioned in #2 and #18.

#35 is a big improvement :-)

Arla’s picture

Status: Needs work » Needs review
FileSize
19.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,118 pass(es), 4 fail(s), and 2 exception(s). View
8.44 KB

Actually, it might make more sense to pull the responsibility of normalizing the field properties (description, alt, etc.) up to EntityReferenceItemNormalizer, like in this patch.

Arla’s picture

Status: Needs work » Needs review
FileSize
21.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,146 pass(es), 3 fail(s), and 0 exception(s). View
2.49 KB

By relying on parent::denormalize() in FileEntityNormalizer, we practically bypass the filename and filemime assignment in File::preCreate() (because ContentEntityNormalizer passes an empty $values to create()). Thus I removed a bit from FileDenormalizeTest which was testing just this.

Another option could be be to do ContentEntityNormalizer::denormalize() in a different way, but I haven't looked into that... yet.

Edit: I realized that the point of handling 'patch' differently in denormalize() is precisely to avoid setting default values. So it should make sense to remove that particular test case as I did.

juampynr’s picture

@arla, how can we test this? I tried the following without success:

1. Gave access to anonymous users to file and node entities.
2. Added a file field to the page content type.
3. Created a node with a file.
4. Opened http://d8.local/node/2 >> I can see the file as an embedded entity.
5. Tried to access the file without luck. I guess this is related with #2310307: File needs CRUD permissions to make REST work on entity/file/{id} .
6. What should be the structure to be sent in order to create a file? I can't figure it out by reading the above patch.

Berdir’s picture

Yeah, not being able to access the file is an access problem, we've been testing this in combination with https://github.com/md-systems/file_entity, which adds an improved access controller.

The structure to create a file is the same as you get with proper access, a normal content entity serialized to hal+json and additionally 'value' with the base64 encoded file content.

clemens.tolboom’s picture

Arla’s picture

Without using file_entity module, you can test it like this:

  1. Add a file field to the page content type
  2. Create a page with a file
  3. Enable module hal
  4. drush cedit rest.settings
    1. substitute basic_auth with cookie
    2. duplicate the entity:node section but change to entity:file
    3. drush cr afterwards
  5. Edit FileAccessControlHandler to make sure checkAccess() returns TRUE (as said, this is treated in #2310307)
  6. Open http://d8/entity/file/1, you should see the file entity in hal+json

To correct #46, the base64 encoded file content goes in 'data', not 'value', per the patch. The name of this JSON property is subject to review, though.

Arla’s picture

Status: Needs work » Needs review
FileSize
23.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,240 pass(es). View
5.11 KB

Trying to address the failing tests. In essence:

  1. In ContentEntityNormalizer::denormalize(), there seems to be no need to unset and re-set the bundle key field.
  2. In EntityReferenceItemNormalizer::constructValue(), I changed to array_key_exists() because otherwise fields with value NULL are skipped. NULL is to be regarded as not set, so maybe the isset() was correct. But that breaks the expectation that an entity is equal before and after serialization+deserialization, as indicated by the failing FileFieldNormalizeTest.
  3. EntityTest::testComment() seemed to expect that revision_id of the target entity be discarded during serialization (from #2233157: Make the comment entity_id be a reference field). Now that EntityReferenceItemNormalizer includes all field properties, it is kept, and as far as I know that is how it should be.
clemens.tolboom’s picture

In #18 one of my questions was about #2

In #2 it seems we are coding around the validators. Is that still the biggest problem?

A rephrase:

We currently want to upload files by creating a entity with file/image fields containing BASE64 encoded field values.

Why do we do it this way?

Why not similar to 'normal' Drupal UI or like the workflow on https://developers.facebook.com/docs/php/howto/uploadphoto/4.0.0 is to post a file and use the resulting ID for further processing. They use http://php.net/manual/en/class.curlfile.php

What happens if I want to create a node containing lots of images when the REST POST size it to big to handle?

Berdir’s picture

I don't understand the question.

We *are* doing separate requests for files, so if you want to upload 10 files, you do 10 POST requests for the files first, then pass all the UUID's when you POST the node.

The only difference is that we pass it along as base64 encoded instead of a multipart POST, but I have no idea if our rest/serializer API supports that.

clemens.tolboom’s picture

@Berdir then please update the summary and title according.

I only scanned the patch and comments so misconcluded.

The steps in @Arla in #48 suggest it's fieldness too.

Arla’s picture

Title: base64 File field content to support REST POST and PATCH operations » Serialize file content (base64) to support REST GET/POST/PATCH on file entity
Category: Bug report » Feature request
Issue summary: View changes

Yes, since #33 we have been moving the serializing of file contents from the file field to the referenced file entity. Editing summary accordingly.

clemens.tolboom’s picture

Issue summary: View changes
clemens.tolboom’s picture

Status: Needs review » Needs work
$ patch -p1 < base64_file_field-1927648-49.patch
patching file core/modules/hal/hal.services.yml
patching file core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
Hunk #1 FAILED at 124.
Hunk #2 FAILED at 140.
2 out of 2 hunks FAILED -- saving rejects to file core/modules/hal/src/Normalizer/ContentEntityNormalizer.php.rej
patching file core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
patching file core/modules/hal/src/Normalizer/FileEntityNormalizer.php
patching file core/modules/hal/src/Tests/DenormalizeTest.php
patching file core/modules/hal/src/Tests/EntityTest.php
Hunk #4 succeeded at 182 (offset 8 lines).
patching file core/modules/hal/src/Tests/FileDenormalizeTest.php
patching file core/modules/hal/src/Tests/FileFieldNormalizeTest.php
patching file core/modules/hal/src/Tests/FileNormalizeTest.php
patching file core/modules/hal/src/Tests/NormalizerTestBase.php
Hunk #1 FAILED at 14.
Hunk #2 succeeded at 117 (offset -7 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/hal/src/Tests/NormalizerTestBase.php.rej
patching file core/modules/rest/src/LinkManager/RelationLinkManager.php
Arla’s picture

Status: Needs work » Needs review
FileSize
23.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch base64_file_field-1927648-56.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll.

larowlan’s picture

Assigned: Unassigned » larowlan

rerolling

Dave Reid’s picture

How would this work with stream wrappers that don't need to be base-64 encoded, like referring to files on Amazon S3? Is there a way to bypass this?

larowlan’s picture

Yeah I think we need to check for the presence of 'data' and fallback to default, which the current patch doesn't yet do

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -64,12 +38,25 @@ public function normalize($entity, $format = NULL, array $context = array()) {
+    // Avoid 'data' being treated as a field.
+    $file_data = $data['data'][0]['value'];
+    unset($data['data']);
+
+    $entity = parent::denormalize($data, $class, $format, $context);
+
+    // Decode and save to file if it's a new file.
+    if (!isset($context['request_method']) || $context['request_method'] != 'patch') {
+      $file_contents = base64_decode($file_data);
+      $dirname = drupal_dirname($entity->getFileUri());
+      file_prepare_directory($dirname, FILE_CREATE_DIRECTORY);
+      if ($uri = file_unmanaged_save_data($file_contents, $entity->getFileUri())) {
+        $entity->setFileUri($uri);
+      }
+      else {
+        throw new RuntimeException(String::format('Failed to write @filename.', array('@filename' => $entity->getFilename())));
+      }
+    }
+    return $entity;

this bit

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
22.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,531 pass(es), 2 fail(s), and 0 exception(s). View

re-roll, EntityTest fails - revision id is being added to entity_id field for comments
doesn't address #60, #61

larowlan’s picture

Assigned: larowlan » Unassigned
larowlan’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
22.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch file-base64-1927648.66.patch. Unable to apply patch. See the log in the details link for more information. View
cloudbull’s picture

May i ask for a sample of json file that use to post for file upload ? I cannot figure out whether it is a client / server side problem.

Thanks
Keith

cloudbull’s picture

Status: Needs review » Needs work

This is the json file i use to post file, tried to put both embedded and field_image no luck

I used angularjs project of #32, added a angularjs images to base64 library.

  $scope.ndata =
        {
         "_links" : {
                "type":
                {
                	"href": mySite + "rest/type/node/article"
                },
	    },
	"_embedded": {
	     file_entity_uri : [
	      {
		"_links": {
		  "type": {
		    "href": mySite + "rest/type/file/file"
		  }
		},
		"lang": "en",
		"values": {
		   "filename": $scope.nodeimage.filename,
		   "file": $scope.nodeimage.base64,
		}
	      }
	    ]
	},
        "langcode" : [
            {
            "value": "en"
            }
        ],
        "field_image" : [
            {
            "value": $scope.nodeimage.base64,
	    "filename": $scope.nodeimage.filename,
            }
        ],
        "title" : {"value": $scope.nodetitle },
        "body" :
          [
            {
            "value": $scope.nodebody,
            "format": "basic_html",
            "summary": "",
            "lang": "en"
            }
          ],
        };

Any help is appreciated.

larowlan’s picture

Status: Needs work » Needs review

Cloudbull please open a new support issue for your particular problem - this issue is about changing the way files are serialized - not providing support for the current format.

cloudbull’s picture

larowlan,

I mean i applied the patch, but things doesn't work. The node is created but file is not uploaded. So, i put my testing json file here for reference.

Keith

larowlan’s picture

@cloudbull - oh sorry - misunderstood

cloudbull’s picture

Status: Needs review » Needs work

So i think the status will be need work ?

Berdir’s picture

Status: Needs work » Needs review

No.

The structure for POST'ing is always identical to the structure you get back. So manually create a file on the server, then do a GET on file/ID and change it as you want.

Your example structure is impossible to read without being formatted, but one thing that is wrong for example is that the base64 encoded file contents are in 'data', not 'file'.

cloudbull’s picture

Berdir,

thanks your help.

This is the sample i get from D8 beta2, from the embeded image part. But, I think it need to change in order to fit the base64 content, right ?


    "http://localhost/src/drucloud-distro/rest/relation/node/article/field_image": [
      {
        "_links": {
          "self": {
            "href": "http://localhost/src/drucloud-distro/sites/default/files/field/image/50cuteanimpic6.jpg"
          },
          "type": {
            "href": "http://localhost/src/drucloud-distro/rest/type/file/file"
          }
        },
        "uuid": [
          {
            "value": "404ee0e6-6d9c-4754-9200-c50beb9bdc29"
          }
        ],
        "uri": [
          {
            "value": "http://localhost/src/drucloud-distro/sites/default/files/field/image/50cuteanimpic6.jpg"
          }
        ],
        "lang": "en"
      }
    ]
  },


if #16 / #30 's PHP client is correct, the json should be something like

   $scope.ndata[file_entity_uri] = {
            "value": $scope.nodeimage.base64,
	    "filename": $scope.nodeimage.filename,
   };

But i cannot see any json return from the server is similar to this format. Any idea will be appreciated.

Thanks
Keith

Berdir’s picture

That's because they messed up the href to the file, it should be something like file/1, aka a resource that you can fetch. That would contain the data you are looking for :(

Have a look at https://github.com/md-systems/file_entity, that should change that and give you a resource you can work with. Note that the module is in early development phase and might/will break.

cloudbull’s picture

Thanks Berdir, So u mean i need to install this module as a contrib module in order to make post file works ?

Berdir’s picture

For now, yes. This issue is one step towards making it work in core, but I'm no sure how far we can or should go here. The referenced permission issue is another step (that should also not be a problem if you use file_entity), the URI/href stuff is yet another problem.

Files have always been a second-class thing in Core, and that's changing only slowly.

The current patch is all about serialization of the file, so it only solves the first part of the issue title. Maybe it should be moved to a separate issue, so we can get it in as a first step without being blocked on having an complete working rest integration.

cloudbull’s picture

OK, thanks Berdir, I will install this module and try GET file/1 first. As for now, without this module, I even cannot do file/1 but ok with node/1 or user/1 in D8 Beta2

skyredwang’s picture

@Berdir, make sense. I don't know how much work has been done in your D8 file_entity on github. But, since D8 support both file and rest, therefore, we need to patch either core or upload your branch to file_entity. I would guess rest integration for file entity needs to go in core.

cloudbull’s picture

Berdir,

I tried to enable the file_entity module and got error below

[Tue Oct 28 20:55:14.114583 2014] [:error] [pid 4811] [client 127.0.0.1:49954] PHP Fatal error: Call to a member function getConfigDependencyName() on a non-object in /var/www/html/src/drucloud-distro/core/lib/Drupal/Core/Entity/EntityDisplayBase.php on line 165, referer: http://localhost/src/drucloud-distro/admin/modules

Is that any update require to work with D8 Beta2 ?

And then calling file/1 will have error below

[Tue Oct 28 20:58:42.583792 2014] [:error] [pid 1531] [client 127.0.0.1:49974] Uncaught PHP Exception Drupal\\Core\\Database\\DatabaseExceptionWrapper: "SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.type' in 'field list': SELECT base.fid AS fid, base.type AS type, base.uuid AS uuid, base.langcode AS langcode, base.uid AS uid, base.filename AS filename, base.uri AS uri, base.filemime AS filemime, base.filesize AS filesize, base.status AS status, base.created AS created, base.changed AS changed\nFROM \n{file_managed} base\nWHERE (base.fid IN (:db_condition_placeholder_0)) ; Array\n(\n [:db_condition_placeholder_0] => 1\n)\n" at /var/www/html/src/drucloud-distro/core/lib/Drupal/Core/Database/Connection.php line 569

Thanks
Keith

Sam152’s picture

+1 RTBC. Installed and exporting/importing entities with file/image fields works as expected. This fixed another issue where some were missing on the field causing SQL errors when importing files.

Sam152’s picture

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -25,38 +23,14 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
+    if (!isset($context['included_fields']) || in_array('data', $context['included_fields'])) {
+      // Save base64-encoded file contents to the "data" property.
+      $data['data'][]['value'] = base64_encode(file_get_contents($entity->getFileUri()));
+    }

Hmm, after digging into this a little more, is "included_fields" being overloaded here? Should the base64 be toggled with a different key, say "include_data"? I think there are some places which expect everything in that array to be a valid field name, which data is not (I believe?).

InvalidArgumentException: Field data is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 349 of core/lib/Drupal/Core/Entity/ContentEntityBase.php).

Sam152’s picture

Status: Needs work » Needs review
FileSize
22.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,535 pass(es), 4 fail(s), and 0 exception(s). View

Reroll.

Sam152’s picture

Continuing on from #84, if users can skip the base64 encoding, should the default behaviour be to still download the files over HTTP? Would seem useful for larger files.

Dave Reid’s picture

I also have a suspicion that base 64 encoding is not going to be useful for the majority of sites when using GET operations.

Sam152’s picture

So if we were to consolidate the requirements for getting this over the line, they might be:

* Optional base64 encoded assets (based on some context, not sure what specifically just yet).
* Default behavior is to base64 encode?
* Fall back downloads the assets over HTTP.

Would be good to get a decisive direction before doing more development.

queenvictoria’s picture

FileSize
22.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch base64_file_field-1927648-90.patch. Unable to apply patch. See the log in the details link for more information. View

I'm not sure if I'm doing the right thing here so please have patience. In 88 and 89 it seems to indicate that including base64 in the HAL response is probably not the right way to do things (possibly just the URI maybe?). However I am trying to get support for base64 in PUT and the method I'm following is to use the format provided by GET.

Firstly I've applied the patch in 85 to the current 8.0.x branch. There is a chuck that doesn't apply which doesn't seem to be related anyway so I've ignored it.

Secondly in order to return the data field I've tried to add 'data' to the included fields. This fails as 'data' is not in the field definition for 'files'.

Finally I've unset the included_fields completely for files as that has the same effect (ie: includes ALL fields). This works well for returning the encoded file in the HAL response.

Again: I don't think this is the right way to proceed. Please use with caution.

larowlan’s picture

Status: Needs work » Needs review

go bot go

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Feature request => 8.1.x.

marthinal’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
39.07 KB

Hi guys!

I need to upload/create images from an angular app.

If I apply #85 (Reroll) I can upload images but we have a couple of problems :-)

1) The first one is "permissions". I'm checking #2310307: File needs CRUD permissions to make REST work on entity/file/{id} and to be honest I'm not sure if file_entity will be the solution or we could fix it in core. I can create images commenting the access() method on post. Enabling file_entity with the same hal+json i have this error:

error: "Unprocessable Entity: validation failed. type.0: The referenced entity (<em class="placeholder">file_type</em>: <em class="placeholder">file</em>) does not exist. "

I need to verify what's going on..

Should we have these permissions into the core?? At least Add/upload and view... Not sure if this is the way and how complicated it is...

2) Once the file is created we should receive the id needed when creating a node for example.

clemens.tolboom’s picture

@marthinal best thing to do is upload your reroll of #85 so we know what you're talking about :-)

BTW what is wrong with #90? Should we hide it?

marthinal’s picture

@queenvictoria about #90, I found the problems described in #94 and had no time to check your changes.

marthinal’s picture

Issue tags: +DrupalCampSpain2015
FileSize
22.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1927648-97.patch. Unable to apply patch. See the log in the details link for more information. View
81.14 KB

About #90, Why not avoid to add "data" property on FileEntityNormalizer::normalize() ?

Rerolled #85.

Need help with #94

@queenvictoria Not sure about your changes, if we want to avoid the "data" property for GET method(because the uri is probably enough) then remove this line :

      // Save base64-encoded file contents to the "data" property.
      $data['data'][]['value'] = base64_encode(file_get_contents($entity->getFileUri()));

AFAIK we don't need this property, maybe I'm missing something...

I've detected that uid is not added when creating a new file.

Image attached(file_managed table).

The second one with uid was added from UI.

marthinal’s picture

FileSize
22.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,980 pass(es), 3 fail(s), and 50 exception(s). View

Reroll again. I'm commenting the access check at EntityResource::post() and at FileEntityNormalizer::normalize() this line

$data['data'][]['value'] = base64_encode(file_get_contents($entity->getFileUri()));

in this case to avoid the data property on get requests.

With the current permissions it is not possible to avoid the access method without commenting and not possible to continue working-testing the file creation.

Anyways I'm adding the patch and then you can try to upload files and verify that it works. As commented in #97 missing uid when creating a new file entity.

marthinal’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Needs review

I had some problems with the version working on another patch and maybe this is the problem...

marthinal’s picture

That was the problem. Cannot apply the patch for 8.1.x

marthinal’s picture

Status: Needs work » Needs review
FileSize
23.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,028 pass(es), 3 fail(s), and 50 exception(s). View
1.17 KB

Found the problem. Interdiff attached.

Does it make sense to have this static method only for Node?

 ->setDefaultValueCallback('Drupal\node\Entity\Node::getCurrentUserId')

If I need the same method for File... not sure about the best way to obtain the uid then. So, maybe a getter for the uid from the Drupal class? Maybe this method is useful in other cases too...

So at this point we can create files and get the file with the uri where the file is located.

marthinal’s picture

Status: Needs work » Needs review
FileSize
25.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,179 pass(es), 3 fail(s), and 56 exception(s). View
5.8 KB

As suggested by @berdir (IRC), we can simply allow to upload files. So, if you have access to the file entity resource then you can upload a file.

Code

--- a/core/modules/file/src/FileAccessControlHandler.php
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -65,4 +65,11 @@ protected function getFileReferences(FileInterface $file) {
     return file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_REVISION, NULL);
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
+    return AccessResult::allowed();
+  }
+
 }

But, what about validators? Actually we can upload files without controlling the extension or size for example...
From the UI when creating a node, the first step is to upload the image(in the current context :) ). From rest you need to upload the image before too. But we don't know the content type for example or you don't need this to create an image. So you can create an image and attach it to the content type you want. There's no control.

I think maybe we can do something like this:

1) Create the file from rest. When creating the file, by default it will be temporary. So we need to attach it to a node for example to mark as permanent. If you want to validate as the UI does, then add validators to the json. See FileEntityNormalizer::denormalize().

2) You can create a file for a content type and attach it to another one (if you know the uuid), with a extension not allowed for example. So let's add validators, detecting if the field type == 'file' . I was testing manually and if you upload a file from rest, then attach this uuid to the node and try to create the node, the validation works as expected. To be honest for the moment only tested for the extension validator.

We are validating 2 times. The first one is optional and the second when creating a node.

I think we need flood control here too... otherwise you can upload unlimited number of files. Temporary files are deleted by a cron task.

Not sure if I'm missing something but maybe this is a good first step.

marthinal’s picture

If you want to test validators when creating the file then use something like this:


"_links":{"type":{"href":"http://d8/rest/type/file/file"}},
 "filename":[{"value":"mytest.jpg"}],
 "filemime":[{"value":"image/jpg"}],
"validators":[{"file_validate_extensions":[{"value": "jpg"},{"value": "txt"}]}],

 "data":the file here

marthinal’s picture

Probably the first validation could be done from your app...

marthinal’s picture

Status: Needs work » Needs review

About PATCH. You can add an empty entity reference, the file will be removed from the node and then the status will be set to 0. The file will be removed from cron.


{"_links":{"type":{"href":"http://d8/rest/type/node/page"}},"title":[{"value":"Update title"}], "body":[{"value":"body!!!!"}]
,"_embedded":
{
"http://d8/rest/relation/node/page/field_test_image":[{"uuid":[{"value":""}]}]
}
}

Cannot DELETE the file from rest and not sure why.

marthinal’s picture

FileSize
24.92 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1927648-113.patch. Unable to apply patch. See the log in the details link for more information. View
4.1 KB

Verify that File module is installed and check if the current entity has files/images, then validate the file.

marthinal’s picture

Status: Needs work » Needs review
andypost’s picture

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -25,37 +24,14 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
+    if (!isset($context['included_fields']) || in_array('data', $context['included_fields'])) {
+      // Save base64-encoded file contents to the "data" property.
+      //$data['data'][]['value'] = base64_encode(file_get_contents($entity->getFileUri()));

commented out?

  1. +++ b/core/lib/Drupal.php
    @@ -247,6 +247,15 @@ public static function currentUser() {
    +   * Gets the uid from the current active user.
    +   *
    +   * @return \Drupal\Core\Session\AccountProxyInterface
    +   */
    +  public static function getCurrentUserId() {
    +    return static::getContainer()->get('current_user')->id();
    

    please remove, no reason anymore

  2. +++ b/core/modules/file/src/Entity/File.php
    @@ -242,6 +242,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setDefaultValueCallback('Drupal::getCurrentUserId')
    

    just add new method like Node does

Dave Reid’s picture

Issue tags: +D8Media
gaurav.goyal’s picture

FileSize
24.77 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,780 pass(es), 3 fail(s), and 56 exception(s). View

Attached is the rerolled patch, Need to fix #116

marthinal’s picture

Assigned: Unassigned » marthinal
marthinal’s picture

Status: Needs work » Needs review
FileSize
25.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,503 pass(es), 3 fail(s), and 56 exception(s). View
2.98 KB

Fixed Error 500. Verify if canonical path exists.

marthinal’s picture

Status: Needs work » Needs review
Issue tags: +Barcelona2015
FileSize
26.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,755 pass(es). View
7.92 KB

As discussed with @klausi we avoid to add the data when normalizing because we can access to the file using the uri.

vivekvpandya’s picture

Thanks Jose,

The latest patch worked on 8.0.x branch. I have uploaded image and text file, it works fine for both
But here is a small problem file is uploaded but when deserilasing it , file is not getting a proper name which is passed along the request.
for following JSON :

{"_links":
{"type":{"href":"http://localhost/dr8b14/rest/type/file/file"}},
"filename":[{"value":"Batman.jpg"}],
"filemime":[{"value":"image/jpg"}],
"data":[{"value":"some base 64 encoded image "}]
}

The uploaded file is not getting proper name i.e in this case it should be Batman.jpg but it gets file4Inkr8 . It seems like file + 6 random chars .
I think that patch will also work for other types of files.

One more point I would like to make here is this method is fine but base64 encoding for file is 33% larger than original file so for mobile apps this matters and also for videos and audios. So Is the Drupal community planning some other option to upload file via REST without base64 encoding ?

vivekvpandya’s picture

@marthinal , one more question here , How can we specify particular directory name into which particular file should go ?

marthinal’s picture

FileSize
30.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,783 pass(es), 5 fail(s), and 0 exception(s). View
6.94 KB

As discussed with @berdir, the user can create a file and set it as persistent(status=1). This patch fix this problem. By default the user can upload a file and if this file is not referenced by an entity(node,user...) then should be removed by a cron task.

Now we are validating using a constraint. So, we only need use this constraint to validate the field(image,file,video...).

@vivekvpandya

1) Let me check why we are adding a random name.

2) Not sure about large files at this moment. I need to comment with @klausi or @berdir.

3) You need to add "uri" to your request

"uri":[{"value": "public://testfolder"}],

vivekvpandya’s picture

@marthinal,
I tried to upload a image file without mime type info and it worked successfully, So what is the use of specifying mime-type ? Is it there for Drupal's file-type restriction ?
Please add all available fields that can be sent via request JSON in documentation. Also let me know if you require any help for that.

marthinal’s picture

Status: Needs work » Needs review
FileSize
29.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,776 pass(es). View
2.44 KB

Deny only edit operations for status field.

vivekvpandya’s picture

After successful creation of file resource on site, with 201 it should also report URI for the file so that it can be used in further requests. I hope https://www.drupal.org/node/2546216 will cover this.

catch’s picture

Category: Feature request » Task
vivekvpandya’s picture

Re-rolling the patch

marthinal’s picture

Status: Needs work » Needs review

@vivekvpandya #137 applies and works as expected.

vivekvpandya’s picture

@marthinal it works on RC1 for me but not on 8.0.x branch , and on RC1 it returns 200 instead of 201.

marthinal’s picture

Related issue.

@vivekvpandya This is weird... Attached image using #137 patch on 8.0.x branch.

kylebrowning’s picture

Re-roll

YesCT’s picture

Assigned: marthinal » Unassigned

it's been a while, so unassigning.

drnikki’s picture

This works for me on RC2.

kylebrowning’s picture

Status: Needs review » Reviewed & tested by the community

SO RTBC then?

drnikki’s picture

It applied cleanly to rc2 but not to 8.0.x. Here's a reroll of kyle's reroll.

neclimdul’s picture

unselecting all the old patches because RTBC acts weird...

We're using this and it seems to work but I don't understand how these changes are making it work so I'm not personally comfortable re-RTBCing this.

dawehner’s picture

Here are a couple of nitpicks.

PS: I cannot see any tests for the FileValidation constraint

  1. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -58,12 +59,19 @@ public function normalize($field_item, $format = NULL, array $context = array())
    +    // Discard the target_id property.
    +    $field_name = $field_item->getParent()->getName();
    +    unset($properties[$field_name][0]['target_id']);
    

    Seems kinda out of scope of this issue to be honest. For me I could totally see value in exposing the target_id as well. Don't see any particular problem with that. In case there are some, maybe we should document this here.

  2. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -100,12 +106,19 @@ public function normalize($field_item, $format = NULL, array $context = array())
         if (isset($id)) {
    -      return array('target_id' => $id);
    +      $constructed = array('target_id' => $id);
    +      foreach ($field_item->getProperties() as $property => $value) {
    +        if ($property != 'target_id' && array_key_exists($property, $data)) {
    +          $constructed[$property] = $data[$property];
    +        }
    +      }
    +      return $constructed;
    

    Maybe the unsetting is related with that piece of code? Can we document why we need this here, why can't we just copy all of the properties?

  3. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -124,4 +137,21 @@ public function getUuid($data) {
    +   * @param FieldItemInterface $field_item
    

    Nitpick: Needs FQCN

  4. +++ b/core/modules/hal/src/Tests/EntityTest.php
    @@ -222,4 +223,41 @@ public function testComment() {
    +    $user = entity_create('user', array('name' => $this->randomMachineName()));
    ...
    +    $file = entity_create('file', array(
    

    Nitpick: Isn't entity_create() kinda deprecated already?

marthinal’s picture

neclimdul’s picture

which was committed. reroll without the old constraint patch.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -68,4 +70,22 @@ protected function getFileReferences(FileInterface $file) {
    +    // No user can add the status of a file to avoid to save as persistent.
    +    if ($field_definition->getName() == 'status' && $operation == 'edit') {
    

    you mean "edit" instead of "add"? Did you mean "No user can edit the status of a file. Prevents saving a new file as persistent before even validating it."

  2. +++ b/core/modules/file/src/Plugin/Validation/Constraint/FileValidationConstraint.php
    @@ -17,6 +17,4 @@
    -class FileValidationConstraint extends Constraint {
    -
    -}
    +class FileValidationConstraint extends Constraint {}
    

    unrelated change not needed for this patch.

  3. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -64,12 +34,26 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +    // Avoid 'data' being treated as a field.
    +    $file_data = $data['data'][0]['value'];
    +    unset($data['data']);
    

    why do we avoid treating data as a field? Suggested comment: "File content can be passed base64 encoded in a special "data" property. That property is not a field, so we remove it before denormalizing the rest of the file entity."

  4. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -64,12 +34,26 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +      $dirname = \Drupal::service('file_system')->dirname($entity->getFileUri());
    

    the file_system service should be injected instead of using \Drupal.

  5. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -64,12 +34,26 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +      file_prepare_directory($dirname, FILE_CREATE_DIRECTORY);
    +      if ($uri = file_unmanaged_save_data($file_contents, $entity->getFileUri())) {
    +        $entity->setFileUri($uri);
    

    We save the file contents to the file system, but we don't keep track of it. Ideally we would later delete unused files after 6 hours, same as we do with managed files. I don't see how we could track that except for saving a managed file, which a denormalizer should not do. So I don't see a solution, just worrying a bit.

  6. +++ b/core/modules/hal/src/Tests/EntityTest.php
    @@ -222,4 +225,43 @@ public function testComment() {
    +      'name' => $this->randomMachineName(),
    

    never use random data in tests, see also #2571183: Deprecate random() usage in tests to avoid random test failures.

  7. +++ b/core/modules/hal/src/Tests/EntityTest.php
    @@ -222,4 +225,43 @@ public function testComment() {
    +    $file_uri = 'public://' . $this->randomMachineName();
    

    same here, please no random test data.

  8. +++ b/core/modules/hal/src/Tests/EntityTest.php
    @@ -222,4 +225,43 @@ public function testComment() {
    +    file_put_contents($file_uri, 'hello world');
    +
    +    $data = file_get_contents($file_uri);
    +    $data = base64_encode($data);
    +    file_put_contents($file_uri, 'hello world');
    

    two times file_put_contents()? Why? Why do we need file_get_contents(), the data is just "hello world"?

  9. +++ b/core/modules/hal/src/Tests/EntityTest.php
    @@ -222,4 +225,43 @@ public function testComment() {
    +    // Use PATCH to avoid trying to create new file on denormalize.
    +    $denormalized_file = $this->serializer->denormalize($normalized, File::class, $this->format, ['request_method' => 'patch']);
    

    I don't understand that comment. The serializer is supposed to create a new file object? why do we need "patch" as context here?

  10. +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
    @@ -39,14 +39,18 @@ public function testFileDenormalize() {
    +    $denormalized = $serializer->denormalize($normalized_data, 'Drupal\file\Entity\File', 'hal_json', array('request_method' => 'patch'));
    

    let's use File::class here

  11. +++ b/core/modules/hal/src/Tests/FileFieldNormalizeTest.php
    @@ -0,0 +1,117 @@
    +    $file_name = $this->randomMachineName() . '.txt';
    +    file_put_contents("public://$file_name", $this->randomString());
    

    please, no random data in tests.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
26.19 KB
8.64 KB

Fixed all points except 9. Not sure what the logic is here, so I am probably not the right person to answer the question or change the patch.

eiriksm’s picture

Oops, sorry. Undefined index. A bit quick there.

Also updated a test class comment that was not accurate anymore.

lightguardjp’s picture

Does anyone have the hal format or required fields for creating a file resource via REST? I've found this part to be severely lacking in the docs department. Maybe it's because I'm new to drupal, but it's been very frustrating try to guess the magic incantation to make things work via REST calls.

vivekvpandya’s picture

@lightguardjp
Here is an example hal+json

{"_links":
{"type":{"href":http://your drupal 8 web site/rest/type/file/file"}},
"filename":[{"value":"filename"}],
"filemime":[{"value":"MIME type"}],
"uri":[{"value": "private://testfolder"}],
"data":[{"value":" base 64 encoded content of your file"
}]
}

Here you can skip uri filed to put the file in public directory.
Here one issue know is that file gets uploaded with some random name. But perhaps @eiriksm 's patch may have solved it.

lightguardjp’s picture

@vivekvpandya Still having issues, maybe we can talk via email (I sent one earlier). During deserialization it blows up as curl doesn't have the private, public, tempory, etc schemes enabled. If you leave out uri then it blows because it uses it for deserializing.

gcardinal’s picture

FileSize
60.55 KB

Tested serialize_file_content-1927648-164.patch on 8.0.0 with following request:

{"_links":
{
      "type":{"href":"http://drupal.url/rest/type/file/file"}
},
	"filename":[{"value":"input.jpg"}],
	"filemime":[{"value":"image/jpeg"}],
	"data":[{"value":"base64-image-data"}]
}

Image was converted using base64 command line line tool
Using basic authorization
Content-Type: application/hal+json
POST send to http://drupal.url/entity/file/

File has been successfully uploaded to public folder.

fjen’s picture

I'm getting:
Recoverable fatal error: Argument 1 passed to Drupal\hal\Normalizer\FileEntityNormalizer::__construct() must implement interface Drupal\rest\LinkManager\LinkManagerInterface, instance of Drupal\Core\Entity\EntityManager given, called in /var/www/html/core/lib/Drupal/Component/DependencyInjection/Container.php on line 277 and defined in Drupal\hal\Normalizer\FileEntityNormalizer->__construct() (line 39 of /var/www/html/core/modules/hal/src/Normalizer/FileEntityNormalizer.php).

for testing against patched D 8.0.0 with serialize_file_content-1927648-164.patch. Here is my request:

POST /entity/file/ HTTP/1.1
Host: 192.168.99.100:8787
X-CSRF-Token: IawzS8poVGS9Y-zrlmiRR9PF2ANrAN3AoVE_7lHVaS4
Content-Type: application/hal+json
Cache-Control: no-cache
Postman-Token: 36482acb-f429-2b63-f9b6-1c23edbddcf3

{"_links":{"type":{"href":"http://192.168.99.100:8787/rest/type/file/file"}},
"filename":[{"value":"input.png"}],
"filemime":[{"value":"image/png"}],
"data":[{"value":"data:image/png;base64,iVBORw0KGgoAAAANSUh…"}]
}
gcardinal’s picture

For this to work, you will also need to activate / install "Responsive Image" module under Core

gcardinal’s picture

Patch #164 used only temp filenames, changed denormalize() function to pass real filename to file_unmanaged_save_data() function, on line #280
Now files are created with provided filename in request.

Wim Leers’s picture

Issue tags: +Needs tests

I'm missing integration tests that effectively prove that GETting/POSTing/PATCHing a base64-encoded file works, like the title indicates.

Berdir’s picture

It doesn't work because that doesn't just depend on this issue. This is just the serialization part. There has been a lot of back and forth but I still think that we should split this up.

Actually using it for REST requests also requires working file permissions. See #2310307: File needs CRUD permissions to make REST work on entity/file/{id} . Note that file_entity adds permissions and it works with that (it also contains an override for this, so it works anyway with file_entity, also without this patch).

gcardinal’s picture

Category: Task » Bug report
Priority: Major » Critical

Setting this one to 'critical' - this one needs urgent attention. Working REST is critical part of a modern CMS and one can't go to 8.1.x and 8.2.x without giving this issue time it needs.

larowlan’s picture

Priority: Critical » Major

Please read the priority values post (https://www.drupal.org/node/45111)

gcardinal’s picture

Priority: Major » Critical

From priority values post (https://www.drupal.org/node/45111)
- Significant regressions in user experience, developer experience, or documentation (at the core maintainers' discretion)

Not being able to get, post or edit image using REST is "Significant regressions in developer experience".

Without this basic functionality it is impossible to integrate Drupal 8 or replace existing Drupal 7 installations.

This should be either adresed or support for REST removed from Drupal 8 - this way so RESTful can be turned back to what it was for Drupal 7. Ignoring this issue for months is not a solution.

klausi’s picture

Priority: Critical » Major
Issue tags: +API-First Initiative

Since the REST module is new in Drupal 8 core this cannot be a regression, because it did not exist in Drupal 7 core.

gcardinal’s picture

In practical sense this is a regression. Because REST is in core now, work on contribution module makes no sense. Based on how popular restful for D7 was, it is not unlikely that many D8 installation also depend on REST for integration.

And as long as this in Major it can stay ignored for month. This is just part of unfinished legacy of Drupal 8 release. Work was simply halted and never completed for Drupal 8 release. Then it got this major priority and unfinished bugged code gets dragget from release to release without people starting on Drupal 8 site even being aware of that basic facts that REST in Drupal 8 is not working for anything else then text.

tstoeckler’s picture

Re @gcardinal: While I agree this is issue is important (not making any suggestion on the issue status, though) it's not correct that it's impossible to work with Rest and Files. You just need the contributed File Entity module at the moment.

gcardinal’s picture

@tstoeckler This patch combined with project/file_entity which is in beta and nothing happened over there since January is not exactly a cocktail anyone would want to build any integration up on.

As for Drupal as a project this is just shameful "thing" that just gets swiped under the carpet. After all, this is not a bug - this is supposed to be part of Drupal 8 that never got finished. Its not like its there and dont work - its simply not there.

marthinal’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
562 bytes
26.84 KB

I think it should be green now...

marthinal’s picture

Status: Needs work » Needs review
FileSize
27.54 KB

Ok. Should apply for 8.2.x

timmillwood’s picture

Here's how we do this in the Replication module http://cgit.drupalcode.org/replication/tree/src/Normalizer/FileItemNorma...
(used by RELAXed web services and Workspace)

marthinal’s picture

Status: Needs work » Needs review
FileSize
672 bytes
27.54 KB

@timmillwood Interesting, thanks for the info.

marthinal’s picture

Status: Needs work » Needs review
FileSize
8.78 KB
27.5 KB

1. EntityNormalizeTest
when denormalizing $data, it contains the fid. So removing this assertion.

2. Moving FileFieldNormalizeTest to the Kernel tests.

marthinal’s picture

Adding integration test for POST method

marthinal’s picture

Integration test for GET method.

marthinal’s picture

This change makes possible enable multiple services per test.

marthinal’s picture

Ok. The next time I'll try to run only a couple of times the testbot :)

Adding PATCH + DELETE.

I think we can improve the tests. For example we should verify that only the owner can remove a file.

marthinal’s picture

1. Adding integration test to verify that only the owner/admin can remove a file. I'm using the patch #2310307: File needs CRUD permissions to make REST work on entity/file/{id}

2. DELETE: I've detected that the file entity is deleted from DB but seems like the file is renamed. So for example foo.txt will be removed from DB but a new foo_0.txt file appears in sites/default/files/... To be honest I have no idea for the moment about how to fix it. Running cron the file still appears.

3. PATCH: You can change the uri and the filename will be the same. So... if I change the uri and then remove the file entity, the file still appears in sites/default/files and ... AFAIK we need to know the file status(from DB) to remove the file using cron. Probably we cannot remove this file. But I'm not sure if I'm missing...

neclimdul’s picture

what about data in the form of data:image/jpeg;base64,? I'm not sure if that's a consistent browser thing or a quirk of the application I'm interacting with but the encoded data is prefixed with that metadata which with the current path is encoded into the contents corrupting the file.

tedbow’s picture

Because of the weird behavior @marthinal described in #198 I thought it would be good to add a check to make sure the file is actually deleted from the database and file system.

This patches adds those checks. It also checks file is actually in the file system before delete.

anty56’s picture

I'm using JSON format, but when I post file this error is return :

Field data is unknown

My JSON :

{
    "filename":[{"value":"input.jpg"}],
    "filemime":[{"value":"image/jpeg"}],
    "uri": [{"value": "http://drupalvm.dev/toto.jpg"}],
    "data": [{"value": "base 64 encode image value"}]
}
Wim Leers’s picture

Status: Needs review » Needs work

This is clearly getting there, but we need some polish/clean-up to have this be in a maintainable and understandable state. And we definitely need extra test coverage: it's not clear to me at the moment whether you can POST/PATCH multiple files at once, for example (for entities that have multiple file fields).

  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -272,4 +273,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +  public static function getCurrentUserId() {
    +    return array(\Drupal::currentUser()->id());
    +  }
    

    Ughhhhh this is awful. But there is a precedent: \Drupal\node\Entity\Node::getCurrentUserId().

    So, fine for now.

  2. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -43,6 +45,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    +      // Only admin users and the file owner can delete the file entity.
    

    This is talking about deleting, but this block of code is executed for both updating and deleting…

  3. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -43,6 +45,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    +      if ($account->hasPermission('administer nodes') || $account->id() == $file_uid[0]['target_id']) {
    

    $file_uid->getTargetIdentifier()

  4. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -43,6 +45,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    +        return AccessResult::allowed();
    ...
    +      return AccessResult::forbidden();
    

    Missing cacheability metadata Per that if-test, these both vary by:

    • the permissions cache context
    • the file entity, since you're using a property on the file entity, so do addCacheableDependency($entity)
  5. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -63,4 +75,23 @@ protected function getFileReferences(FileInterface $file) {
    +  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    +    return AccessResult::allowed();
    +  }
    +
    

    This means anybody can create file entities. Which is exactly what \Drupal\Core\Entity\EntityAccessControlHandler::checkCreateAccess() does for us.

    So you can remove this.

  6. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -63,4 +75,23 @@ protected function getFileReferences(FileInterface $file) {
    +    if ($field_definition->getName() == 'status' && $operation == 'edit') {
    

    Let's use strict equality.

  7. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -63,4 +75,23 @@ protected function getFileReferences(FileInterface $file) {
    +    return AccessResult::allowed();
    

    Let's instead call the parent method. Which means this then is a pure decorator of the base class for this method.

  8. +++ b/core/modules/hal/hal.services.yml
    @@ -16,7 +16,7 @@ services:
    -    arguments: ['@entity.manager', '@http_client', '@rest.link_manager', '@module_handler']
    +    arguments: ['@rest.link_manager', '@entity.manager', '@module_handler', '@file_system']
    

    Why change the position of every injected service that we keep? We remove one service, and add another, but every single one is in a different position. That's not necessary.

  9. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
    @@ -142,10 +143,9 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
    -      // Unset the bundle key from data, if it's there.
    -      unset($data[$bundle_key]);
    

    Why this change?

  10. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -78,15 +86,13 @@ public function normalize($field_item, $format = NULL, array $context = array())
    -        $field_uri => array($embedded),
    +        $field_uri => array($embedded + $properties[$field_name][0]),
    

    This looks… very weird. Could use a comment.

  11. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -95,12 +101,19 @@ public function normalize($field_item, $format = NULL, array $context = array())
    +    /** @var FieldItemInterface $field_item */
    
    @@ -119,4 +132,21 @@ public function getUuid($data) {
    +   * @param FieldItemInterface $field_item
    

    Needs FQCN.

  12. +++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
    @@ -95,12 +101,19 @@ public function normalize($field_item, $format = NULL, array $context = array())
    +        if ($property != 'target_id' && array_key_exists($property, $data)) {
    

    Nit: Strict equality.

    Why not isset()? Can the value of this key be NULL?

  13. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -20,28 +21,19 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    -   * Constructs a FileEntityNormalizer object.
    +   * Constructs an FileEntityNormalizer object.
    

    Wrong change.

  14. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -20,28 +21,19 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    -   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
    -   *   The entity manager.
    -   * @param \GuzzleHttp\ClientInterface $http_client
    -   *   The HTTP Client.
    -   * @param \Drupal\rest\LinkManager\LinkManagerInterface $link_manager
    -   *   The hypermedia link manager.
    -   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
    -   *   The module handler.
    +   * @param \Drupal\Core\File\FileSystemInterface $file_system
    +   *   The file system.
        */
    -  public function __construct(EntityManagerInterface $entity_manager, ClientInterface $http_client, LinkManagerInterface $link_manager, ModuleHandlerInterface $module_handler) {
    +  public function __construct(LinkManagerInterface $link_manager, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, FileSystemInterface $file_system) {
    

    This makes no sense at all.

  15. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -49,8 +41,6 @@ public function __construct(EntityManagerInterface $entity_manager, ClientInterf
       public function normalize($entity, $format = NULL, array $context = array()) {
         $data = parent::normalize($entity, $format, $context);
    -    // Replace the file url with a full url for the file.
    -    $data['uri'][0]['value'] = $this->getEntityUri($entity);
     
         return $data;
       }
    

    Looks like we can remove this altogether now, since this merely calls the parent and returns the result?

  16. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -59,12 +49,28 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +    // File content can be passed base64 encoded in a special "data" property.
    

    So this is a property *under* the file reference field, right? Otherwise, you couldn't send multiple files at once.

  17. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -59,12 +49,28 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +    $file_data = $data['data'][0]['value'];
    

    Let's put this 'data' key in a class constant.

  18. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -59,12 +49,28 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +    // Decode and save to file if it's a new file.
    +    if (!isset($context['request_method']) || $context['request_method'] != 'patch') {
    

    Why do we not need to decode and save when updating?

  19. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -59,12 +49,28 @@ public function normalize($entity, $format = NULL, array $context = array()) {
    +      file_prepare_directory($dirname, FILE_CREATE_DIRECTORY);
    +      if ($uri = file_unmanaged_save_data($file_contents, file_build_uri(drupal_basename($entity->getFilename())))) {
    

    This is using lots of deprecated functions. Look at the FileSystem service.

  20. +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
    @@ -34,14 +34,18 @@ public function testFileDenormalize() {
    +    $data = file_get_contents($file_params['uri']);
    +    $data = base64_encode($data);
    

    Nit: Let's make this a single line.

  21. +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
    @@ -34,14 +34,18 @@ public function testFileDenormalize() {
    +    // Adding data to the entity.
    
    +++ b/core/modules/hal/tests/src/Kernel/EntityNormalizeTest.php
    @@ -203,4 +205,39 @@ public function testComment() {
    +    $normalized['data'][0]['value'] = $data;
    

    This reads far too generically. But the constant I asked for above would already make this much, much clearer.

  22. +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
    @@ -34,14 +34,18 @@ public function testFileDenormalize() {
    +    // Use 'patch' to avoid trying to recreate the file.
    
    +++ b/core/modules/hal/tests/src/Kernel/EntityNormalizeTest.php
    @@ -203,4 +205,39 @@ public function testComment() {
    +    // Use PATCH to avoid trying to create new file on denormalize.
    

    That shouldn't be the reason we do this; the reason should be that we test both POST and PATCH.

  23. +++ b/core/modules/hal/src/Tests/FileDenormalizeTest.php
    @@ -49,27 +53,6 @@ public function testFileDenormalize() {
    -    // Try to denormalize with the file uri only.
    

    Why did we remove this?

  24. +++ b/core/modules/hal/tests/src/Kernel/FileFieldNormalizeTest.php
    @@ -0,0 +1,115 @@
    +   * Tests that file field is identical before and after de/serialization.
    ...
    +   * Tests that image field is identical before and after de/serialization.
    

    "de/serialization" -> "(de)serialization"

  25. +++ b/core/modules/hal/tests/src/Kernel/FileFieldNormalizeTest.php
    @@ -0,0 +1,115 @@
    +    // Create a file.
    ...
    +    // Create a file.
    

    Pointless comments.

  26. +++ b/core/modules/hal/tests/src/Kernel/NormalizerTestBase.php
    @@ -130,15 +131,14 @@ protected function setUp() {
    +      new ContentEntityNormalizer($link_manager, $this->container->get('entity.manager'), $this->container->get('module_handler')),      new EntityReferenceItemNormalizer($link_manager, $chain_resolver),
    

    Nit: too many spaces.

  27. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -108,9 +108,14 @@ public function post(EntityInterface $entity = NULL) {
    +      // body. Verify if canonical path exists.
    +      if ($entity->hasLinkTemplate('canonical')) {
    

    Good call! :)

  28. +++ b/core/modules/rest/src/Tests/FileTest.php
    @@ -0,0 +1,130 @@
    +class FileTest extends RESTTestBase {
    

    This is testing using HAL+JSON. But this is the REST module, not the HAL module.

    This test should be moved to the HAL module, and a similar one should be created for the REST module. In this new test, we should test both JSON and XML.

    Then the test moved to HAL can hopefully (but not certainly) reuse parts of this new test.

  29. +++ b/core/modules/rest/src/Tests/FileTest.php
    @@ -0,0 +1,130 @@
    +    $this->enableService('entity:' . $entity_type, 'POST', 'hal_json');
    +    $this->enableService('entity:' . $entity_type, 'GET', 'hal_json');
    +    $this->enableService('entity:' . $entity_type, 'PATCH');
    +    $this->enableService('entity:' . $entity_type, 'DELETE');
    

    Why don't we need hal_json for PATCH?

    (For DELETE, it makes sense, for PATCH, not really.)

    This feels like we have either a bug or missing test coverage.

  30. +++ b/core/modules/rest/src/Tests/FileTest.php
    @@ -0,0 +1,130 @@
    +    // Remove non-accessible fields.
    +    unset($normalized_data['status']);
    +    unset($normalized_data['changed']);
    

    They're accessible, they're just not modifiable?

  31. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    @@ -258,7 +258,7 @@ protected function entityValues($entity_type) {
    -    $settings = array();
    +    $settings = $resource_type ? $config->get('resources') : [];
    

    Is this change here to allow appending?

  32. +++ b/core/modules/serialization/src/Tests/NormalizerTestBase.php
    @@ -3,6 +3,7 @@
    +use Drupal\hal\Normalizer\FileEntityNormalizer;
    

    Unnecessary change.

Wim Leers’s picture

tedbow’s picture

Status: Needs work » Needs review
FileSize
33.33 KB
13.42 KB

Ok here is patch that address some of @Wim Leers recommendations in #202

Tried to fix the quick stuff and will investigate the other issues next. I will need to catch up on the issue better

1. just a note, no change
2. fixed comment
3,4 skipping for now
5. removed
6. fixed
7. fixed
8. reset position of service arguments
9. skipping
10.
$field_uri => array($embedded + $properties[$field_name][0]),
Split the array merging into a line above with comment.
11. add FQCN
12. added Strict equality.
13. fixed wording
14. Fix constructor to match services arguments changed back in 8
15. remove normalize function
16. updated comment
17. added constant
18. skipping, having looked into it yet
19. only saw 1 function that was deprecated. fixed
20. changed to single line
21. used new constant from 17
22, 23. skipping, having looked into it yet
24. fixed wording
25. removed comments
26. fixed spacing
27. just a note - no fix needed
28, 29. skipping, having looked into it yet
30. fixed comment
31. skipping, having looked into it yet
32. It look like this import is need to me. left it

Berdir’s picture

Re #203: I've been trying to argue for a while that this issue is only about correctly *serializing and deserializing* a file entity. And not about actually making it work for rest.module. Then we could do this issue and the other one separately.

Question is which issue is going to add actual rest.module integration tests. Could be the one that happens to land second or a new issue for which this and the other one are child issues. Or we make permission depend on this one first and then add tests. I *think* this is closer as the permission issue still has some rather big unanswered questions (from me) AFAIK.

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
33.67 KB

Ok, in #204 I didn't see that FileEntityNormalizer was instantiated in the tests. I fixed the arguments to the constructor.

tedbow’s picture

  1. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -87,4 +87,12 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    +    // @todo Remove this override after https://www.drupal.org/node/2310307 is fixed.
    +    return AccessResult::allowed();
    +  }
    +
    

    I added this back because the parent::checkCreateAccess depends on getAdminPermission which I don't will work until #2310307: File needs CRUD permissions to make REST work on entity/file/{id} is finished.

  2. +++ b/core/modules/rest/src/Tests/FileTest.php
    @@ -65,7 +66,7 @@ public function testCrudFile() {
    -    $normalized_data['data'][0]['value'] = $data;
    +    $normalized_data[FileEntityNormalizer::FILE_DATA_KEY][0]['value'] = $data;
    

    Using the new class constant

Wim Leers’s picture

#205/@Berdir: I think it doesn't make sense for this issue to land without integration tests, so IMO it'd be more sensible for #2310307: File needs CRUD permissions to make REST work on entity/file/{id} to land first then. Because it sounds like the FileAccessControlHandler changes here are only being done to make this issue actually work, and cannot actually be committed: those changes must happen in #2310307: File needs CRUD permissions to make REST work on entity/file/{id} ?

Berdir’s picture

Well, integration for what exactly? We have integration test for serialization. This does nothing rest specific, so why do we need tests for that, here?

This issue alone already enables use cases, like default_content. rest.module isn't the only thing that uses serialization.

dawehner’s picture

Issue tags: -Needs tests

Well, integration for what exactly? We have integration test for serialization. This does nothing rest specific, so why do we need tests for that, here?

Yeah this issue is only about serialization

  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -272,4 +273,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +   * @see ::baseFieldDefinitions()
    

    It actually turns out that @see :: is not supported by API providers, nor IDE vendors, see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

    Let's not introduce one even yes, we have used it in quite some places now.

  2. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -43,6 +45,16 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    +      if ($account->hasPermission('administer nodes') || $account->id() == $file_uid[0]['target_id']) {
    

    Don't we have to encode the owner somehow in the access result object cache contexts?

  3. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -20,51 +26,55 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    +      if ($uri = file_unmanaged_save_data($file_contents, file_build_uri($this->fileSystem->basename($entity->getFilename())))) {
    

    I should not have looked up file_unmanaged_save_data()

amateescu’s picture

I could only find a few small coding standards issues:

  1. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -63,4 +75,24 @@ protected function getFileReferences(FileInterface $file) {
    +    // @todo Remove this override after https://www.drupal.org/node/2310307 is fixed.
    

    Comment needs to be wrapped to 80 cols.

  2. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -20,51 +26,55 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    +        throw new RuntimeException('failed to write ' . $entity->getFilename());
    

    Capital F :)

  3. +++ b/core/modules/hal/tests/src/Kernel/EntityNormalizeTest.php
    @@ -203,4 +206,39 @@ public function testComment() {
    +  }
     }
    

    Missing empty line.

  4. +++ b/core/modules/hal/tests/src/Kernel/FileFieldNormalizeTest.php
    @@ -0,0 +1,113 @@
    +  public static $modules = [
    +    'entity_test',
    +    'field',
    +    'image',
    +    'hal',
    +    'system',
    +    'file',
    +  ];
    

    We usually put these in a single line.

  5. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
    index fc272f7..c2a2ab3 100644
    --- a/core/modules/serialization/src/Tests/NormalizerTestBase.php
    
    +++ b/core/modules/serialization/src/Tests/NormalizerTestBase.php
    @@ -3,6 +3,7 @@
    +use Drupal\hal\Normalizer\FileEntityNormalizer;
    

    Not needed. Note that this is in the serialization module, to not be confused with the class that has the same name from the hal module.

Wim Leers’s picture

Woot, great to have @amateescu reviewing this patch!

tedbow’s picture

Address review in #212 and #211 except item 3.

@dawehner wasn't sure what you meant

I should not have looked up file_unmanaged_save_data()

file_unmanaged_save_data is not deprecated.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -50,7 +50,7 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       if ($account->hasPermission('administer nodes') || $account->id() == $file_uid[0]['target_id']) {
-        return AccessResult::allowed();
+        return AccessResult::allowed()->addCacheableDependency($account);

IMHO we need to add the permission as cache context here, so use cachePerPermissions

file_unmanaged_save_data is not deprecated.

This was just a humorist comment ...

tedbow’s picture

Status: Needs work » Needs review
FileSize
33.2 KB

@dawehner I didn't know aobut cachePerPermissions.

Updated to use cachePerPermissions

Wim Leers’s picture

I still have nits, but I have only 3 critical, commit-blocking questions:

  1. Does this work for files that are larger than PHP's memory limit?
  2. +++ b/core/modules/file/src/Entity/File.php
    index f9569ec..ee21486 100644
    --- a/core/modules/file/src/FileAccessControlHandler.php
    

    Is it okay for these changes to happen here, or do we want #2310307: File needs CRUD permissions to make REST work on entity/file/{id} to happen first? This does the same, but not exactly the same.

    Let's document very clearly in the IS what this issue does and why, and what #2310307: File needs CRUD permissions to make REST work on entity/file/{id} then does, and why it's okay for this to go in first. Especially considering this does not add test coverage.

  3. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -13,6 +14,11 @@
       /**
    +   * Key used to store actual file data.
    +   */
    +  const FILE_DATA_KEY = 'data';
    

    This sounds like it's impossible to POST an entity that has multiple files attached.

    Let's add a test that proves me wrong.

Berdir’s picture

1. No. This obviously has limitations, base64 also needs 30% or so more memory than the binary data.
2. I still believe that permissions do *not* belong in here. Who knows what kind of side effects this has, there could be code there that's relying on create permissions and might expose things to anyone that weren't exposed before (rest still needs a separate permission, but there could be other services or so).
3. This is the file entity. You can only post one file, just like you can only post one node at a time. You can however post 3 files with 3 separate requests and then a node that references all of them with the 4th.

damiankloip’s picture

I agree with berdir here, I do not understand why we're bringing REST module into this. REST != serialization component necessarily. E.g. I maintain serialization, NOT REST :)

damiankloip’s picture

  1. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -20,51 +26,55 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    -    $file_data = (string) $this->httpClient->get($data['uri'][0]['value'])->getBody();
    

    I think the current/old method is kind of valid too... would be nice if there was a way to incorporate both.

  2. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -20,51 +26,55 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    +    if (!isset($context['request_method']) || $context['request_method'] != 'patch') {
    

    Why should denormalization care about the request method like this? Seems rather hackish. Should this not work based on whether there is encoded binary data present or not?

I am also not clear on why this doesn't just move to serialization module itself? This bakes everything into hal module, so it's not usable with other regular type of decoding, like JSON... If it was the other way, hal could easily use this normalizer anyway.

Wim Leers’s picture

Status: Needs review » Needs work

Moving to NW for #220, especially:

I am also not clear on why this doesn't just move to serialization module itself? This bakes everything into hal module, so it's not usable with other regular type of decoding, like JSON... If it was the other way, hal could easily use this normalizer anyway.

+100

Also, #2310307: File needs CRUD permissions to make REST work on entity/file/{id} landed, this needs to be rerolled.

tedbow’s picture

Assigned: Unassigned » tedbow

Working on re-roll

tedbow’s picture

Status: Needs work » Needs review
FileSize
32.4 KB

Just a re-roll

Berdir’s picture

  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -232,6 +232,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
           ->setLabel(t('User ID'))
           ->setDescription(t('The user ID of the file.'))
    +      ->setDefaultValueCallback('Drupal\file\Entity\File::getCurrentUserId')
           ->setSetting('target_type', 'user');
    

    Related: #1979260: Automatically populate the author default value with the current user

  2. +++ b/core/modules/file/src/FileAccessControlHandler.php
    @@ -96,8 +106,6 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
         // (e.g. an image for a article). A contributed module is free to alter
         // this to allow file entities to be created directly.
    -    // @todo Update comment to mention REST module when
    -    //   https://www.drupal.org/node/1927648 is fixed.
         return AccessResult::neutral();
       }
    

    The question is what we're going to do with this now. Just removing the @todo won't be enough :)

    Per discussion in related issues, we're apparently considering again to upload files as part of field values of e.g. the node, which would change 90% of this patch. It would be simpler, but it would also be more limited.

    I guess we could say that if you need more. you can use file_entity. Or some other module, there's at least another one started to contain various serializers.

Wim Leers’s picture

tedbow’s picture

The question is what we're going to do with this now. Just removing the @todo won't be enough :)

So what are options here? It seems like we could make file entity type core permissions

  • Administer Files
  • CRUD File permissions

But these would be only be used by REST module. So I think this would be confusing for site administrators.

  1. It could confused easily with confused with permissions dealing with File Field permissions(is this the only way to upload files with REST?).
  2. To configure REST permissions on the FILE Resource you would have to set both the File Entity permission and the File REST Resource permissions

While #2 is true for other entity types with other entity types at least the entity type permission have some meaning with just Drupal Core outside of REST.

return AccessResult::neutral();

I did confirm locally with this returning allowed() all the tests pass so I don't think the re-roll introduced any new/unknown problems.

tedbow’s picture

Assigned: tedbow » Unassigned
Wim Leers’s picture

Assigned: Unassigned » Berdir

Berdir has the most experience and insight on this, so I'd say: let's let Berdir decide.

dawehner’s picture

Per discussion in related issues, we're apparently considering again to upload files as part of field values of e.g. the node, which would change 90% of this patch. It would be simpler, but it would also be more limited.

I'm just trying to be curious here.
Would that cause issues with something like a maximum payload you could manage?

Wim Leers’s picture

#230:

  • #217.1: Does this work for files that are larger than PHP's memory limit?
  • #218.1: No. This obviously has limitations, base64 also needs 30% or so more memory than the binary data.
Berdir’s picture

Assigned: Berdir » Unassigned

Yes, but the point is that uploading as part of the node then means that max applies to the sum of all files. Imagine creating a gallery node with 20 images for example. That's just not going to work. While creating them separately will take a while but there's a chance that it will work ;)

I don't feel like I can decide here. All I can offer are some suggestions/ideas.

#227 has the same questions as I asked in #2310307: File needs CRUD permissions to make REST work on entity/file/{id} and there we basically avoided that problem, resulting in not actually solving it for this issue, which was my understanding of the split between the two.

As mentioned before, this is all working quite fine with file_entity, because it has those permissions and also a version of this serializer.

I know that it would derail this even more, but one idea would be to actually bring some of the features of file_entity into core, which is something that @slashrsm and I talked about. Like being able to upload files "into the media library" as a standalone thing. Then we'd need a permission for that and it wouldn't be weird anymore if rest would allow the same. It would also force us to think once more about the whole file usage and auto-delete situation, which is still not really resolved (upload a file, then use it (file usage is now 1), then stop using (file usage is now 0 and it is removed).

Another idea is the the same permission concept that we have for private files. By default, you can see an private file if you can see at least one of the entities that are using it. So we could say you can upload a file if you have access to at least one entity with a file/image field. obviously, that could be a pretty slow access check, if we'd need to do create access checks for 10 different node types until we find one that you're allowed to use. But it would kind of reflect the situtation in the UI. And we already enforce that files are temporary until you actually use them.

But even then, one of the problems that we haven't solved so far is file validation. In core, that's all tied to file/image fields. They control what file types you may upload, how big they can be and so on..

tedbow’s picture

re #224"

Per discussion in related issues, we're apparently considering again to upload files as part of field values of e.g. the node, which would change 90% of this patch. It would be simpler, but it would also be more limited.

If anybody has links to these issues can you post them here so we can see what discussion is going on? Thanks

Would this be an either or situation? Could we support both? I could see clients wanting to do both in different situations.

As @Berdir says if you are uploading a gallery node you are likely to run into upload limit if you have serializing all files into 1 node POST. But then again if you are just posting an article with an image having to post the image first and then post the article which includes a reference to the just posted image seems is 2 posts which could just be 1.

re @Berdir #232

I know that it would derail this even more, but one idea would be to actually bring some of the features of file_entity into core, which is something that @slashrsm and I talked about.

It seems like this will probably happen eventually, especially since @dries mentioned Media as an initiative in Drupalcon NOLA.

One option would be to postpone this issue until core had the ability to upload files separate from file reference fields. Presumably at that point there would be permission for uploading files themselves and then rest could just check that permission.

In the meantime we could open a separate issue for letting file be serialized as part of file reference field.

Then after both issues were finished there would 2 ways to upload files and clients could use which was easier for a situation.

I think the problem with doing this 1 now and adding new CRUD permissions for file entities is that we run into a situation where we will want to change the way permissions work for file entities and then have to introduce a backward compatibility layer. For instance once something like File Entity is in core then we would probably not want file entity level CRUD permissions but rather file bundle level permissions.

Imagine creating a gallery node with 20 images for example. That's just not going to work. While creating them separately will take a while but there's a chance that it will work ;)

If this issue did get postponed but the serializing on file fields got committed would it still be possible to attach 20 images to a single node(or other entity) via PATCH requests? Obviously it wouldn't be ideal versus individual file entity posts and 1 post to the referencing entity.

skyredwang’s picture

I am an Android developer. I agree with @Berdir issue. Not being able to manage many files doesn't sound right. Also, I'd like to point out that for mobile dev, design for offline is a priority, in other words, uploading files based on connectivity conditions is must. Therefore, I would never want to combine 1 article and 1 image into 1 HTTP post to Drupal. It's always ideal to separate them into two requests.

bc’s picture

re-rolled patch for 8.2.x c9e8e141435fc916c6ead204dc48df132e7c5df5

tedbow’s picture

Status: Needs work » Needs review

@bc thanks for re-roll
Setting to "needs review" to trigger tests

bc’s picture

darn it all, i uploaded a patch made against drupal-core; here's a more testbot-friendly one:

tedbow’s picture

@bc was #238 just a re-roll of #223 or where there other changes. I am wondering about the extra failing tests.

Thanks

bc’s picture

Just a re-roll -- it's pretty strange. I'm double checking my work now.

bc’s picture

Missed a line in my reroll. Ugh. Let's see how the tests go this time ;)

tedbow’s picture

@skyredwang re:

I agree with @Berdir issue. Not being able to manage many files doesn't sound right.

and

Therefore, I would never want to combine 1 article and 1 image into 1 HTTP post to Drupal. It's always ideal to separate them into two requests.

I don't disagree with you here but if there is a way to manage files separate from any entity they are attached to then we would need some sort of file permissions.

So then we would have to pick a way to implement those permissions.

Option 1: Create file CRUD permissions and have REST respect those permissions

This has several problems

  1. Do we create these permissions in the File module though without REST enabled these permissions have no meaning(through core web interface)
  2. Do we create these permissions in the REST module for the file entity?
  3. How to avoid the confusion for the site admin that "File Edit permission" actually has no functionality outside rest, i.e. it doesn't apply to File fields?

Option 2: Create REST specific verb permissions for file methods

The also introduces problems

  1. There is already: #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources when this lands file REST methods would be different from all other Entity Resources
  2. When media library functionality does get into core as @Berdir suggests in #232 it would introduce File CRUD permissions then we would likely want to remove the File verb permissions we added here and create a backwards compatibility layer like #2664780 is doing for other entities. These just seems confusing for the site admin.(imagine maintaning/developing multiple 8.x some using this bc layer and some not)

Option 3: Treat similar to private allow uploading temporary files based on entity access

From @Berdir comment in #232 again

So we could say you can upload a file if you have access to at least one entity with a file/image field. obviously, that could be a pretty slow access check, if we'd need to do create access checks for 10 different node types until we find one that you're allowed to use. But it would kind of reflect the situtation in the UI. And we already enforce that files are temporary until you actually use them.

Again this option has problems

  1. Slow access checks. Checking create access are across all entity types that have a bundle that has a file field.
  2. What if the user only has edit access then presumably you would need to check every entity that has a file field :( ?

One option, that doesn't seem very clean, to get around the slow access checks would be to have the file upload specify the entity or entity type that will be created that user will be attaching the file to and base the access check on that entity or create operation. But I don't know how you would cleanly specify that in REST post(a custom header?)


I don't think it is ideal to only be able to upload files as part of a field on an entity but we do have to solve the permissions issue we are going to manage them directly.

Once something like file/media library is then this would much easier because it would add file permissions.

But I don't think it is good idea delay file uploads via REST

Thoughts on those options or others?

Of course the other option is upload file as part of file field on another entity. While that doesn't introduce permissions problems it does introduce other problems mention in this issue.

skyredwang’s picture

https://github.com/fago/entity/pull/9 will land soon, I am wondering if it will help?

tedbow’s picture

@skyredwang no that is the Entity API contrib module and won't affect Drupal core

Also building the permissions is not the hard part. The hard part is choosing one of the options I described in #245 or another that we haven't thought of. And then dealing with the downsides of whatever choice we make(they all have downsides)

garphy’s picture

Concerns about the maximum size of the actual "upload" were raised multiple times in this issue but were never actually really addressed.

Maybe the vast majority of current contributors to this issue are not used to manage big files but on the majority of projects I work on, we regularly upload files which size vary from several 100s MB to several GBs.

It's obvious that the approach of trying to base64_decode the payload in RAM then write it to disk won't work, not only because of PHP memory limitation but also because of underlying system itself memory limitations.

And it's worth mentioning that REST client will also have the same issue when trying to build the request. It's probably doable (but tedious) to "stream" the base64 encoded payload from the client to the server, but I can't actually think of a "streamed" approach on the server-side. We need to have the complete JSON (or whatever) request payload before trying to find the base64 payload.

The natural alternative approach would be to address this concern using "plain old" multi-part POST request in which we could have the file payload (obviously) and the required additional metadata needed to create a file_entity. This would (maybe) "break" REST semantics but I would be usable, even for very large files.

I would be more than happy to read followups on that :)

tedbow’s picture

@garphy

Maybe the vast majority of current contributors to this issue are not used to manage big files but on the majority of projects I work on, we regularly upload files which size vary from several 100s MB to several GBs.

Thanks for the input. I agree the serialization method would not work well for files of this size.

I would think though that serialization method would work for the majority of sites. Also just because we add this method does not preclude us adding another method later. Of course work could be done in contrib to prove a method of using REST resources to handle files of the the size you are talking about.

My opinion is that we shouldn't change this issue just because it doesn't work for every use case. Especially if as you say "vast majority of current contributors to this issue" haven't run into this use case. If true that probably points to it being a not very common use case.

garphy’s picture

Ok, I'll then go to implement a custom solution to accept multipart/form-data request that would upload file data and create the corresponding file entity.

What would be great is to be able to "hook" this solution with core REST module by making it accept multipart/form-data requests. Any thought on this ?

garphy’s picture

Status: Needs work » Needs review
FileSize
31.95 KB

Re-rolled the latest patch

bc’s picture

Here's how I added a POST resource for multipart/form-data uploads: https://gist.github.com/bnchdrff/a388b763bc97f7a1c6107698652cc58d

Berdir’s picture

@bc: your approach still loads the file contents into memory s o it actually has pretty much the same limitations, minus the base64 overhead.

And sure, something like that is possible. But doing that through the REST/serialization framework is harder.

bc’s picture

I'm curious how many people are uploading files as serialized data at this point vs how many people are either giving up on d8 or writing hacky custom endpoints like me :)

In the case of a mobile client uploading an image to a d8 server, it's really klutzy and inefficient to serialize the photo on the client -- many phones will lock up / OOM.

garphy’s picture

Here's my approach so far to upload & create a file entity using multipart/form-data and avoid loading the file in memory :

https://www.drupal.org/sandbox/garphy/2770603

It tries to reuse existing pieces like file_save_upload(), so the key for the file in POST data has to be "files[file]".

bc’s picture

Garphy the link doesn't work! I'm curious how you've solved it -- also if anyone else on this issue has stopgap solutions maybe they can be used to inform how the d8 core api could function.

garphy’s picture

Sorry for the wrong link...

Sandbox project page : https://www.drupal.org/sandbox/garphy/2770603
Repo : http://cgit.drupalcode.org/sandbox-garphy-2770603/tree/

The only important piece of code is the controller which basically does

  function handle(Request $request){

    $destination = 'private://uploads';
    file_prepare_directory($destination, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
    $file = file_save_upload('file', array(), $destination, 0);
    return JsonResponse::create(['fid' => $file->id()]);

  }

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Pedro J. Fernandez’s picture

Hi everyone!

I got this working but the uploaded file is always uploaded in the /sites/default/files folder. I tryed to specify the destination folder adding the "uri" attribute to the hal request, or writting the uri in the filename attribute instead only the name. i.e. public://some_existing_path/filename. For any reason, the file is always created in the /sites/default/files and the destination folder is ignored. Please, I don't know where is the problem. Please, help.

This is the POST body:

{
  "_links": {
    "type": {
    	"href" : "http://www.example.com/rest/type/file/file"
     }
  },
  "filename":[{"value": "public://photos/test11.jpg"}],
  "filemime":[{"value":"image/jpeg"}],
  
  "data": [{ "value" : "/9j/4Rd9RXhpZgAASUkqAAg........ }]

}

The file is created and this is the response: 201 Created

{
  "fid": [
    {
      "value": "77"
    }
  ],
  "uuid": [
    {
      "value": "777ebf25-5f4f-42f3-a570-ac5bfceefa2b"
    }
  ],
  "langcode": [
    {
      "value": "en"
    }
  ],
  "uid": [
    {
      "target_id": "1",
      "target_type": "user",
      "target_uuid": "3096cd62-05b9-417e-841e-d2072c9e93f4",
      "url": "/es/user/1"
    }
  ],
  "filename": [
    {
      "value": "public://photos/test11.jpg"
    }
  ],
  "uri": [
    {
      "value": "public://test11.jpg"
    }
  ],
  "filemime": [
    {
      "value": "image/jpeg"
    }
  ],
  "filesize": [
    {
      "value": 297880
    }
  ],
  "status": [
    {
      "value": false
    }
  ],
  "created": [
    {
      "value": 1471384327
    }
  ],
  "changed": [
    {
      "value": 1471384327
    }
  ]
}

And the file is created in /sites/default/files instead of /sites/default/files/photos.

eugene.ilyin’s picture

@Pedro

I got this working but the uploaded file is always uploaded in the /sites/default/files folder. I tryed to specify the destination folder adding the "uri" attribute to the hal request, or writting the uri in the filename attribute instead only the name. i.e. public://some_existing_path/filename. For any reason, the file is always created in the /sites/default/files and the destination folder is ignored. Please, I don't know where is the problem. Please, help.

I've specified uri:

"uri" : [{"value":"public://project/sad_photo.jpg"}],
and my file has beed saved according to path.

P.S. As I see according to code in patch, now I cannot upload multiple files in one request? Only one file per request. Am I correct?

I need to save bunch of files into the Node and would be nice to avoid 20 requests.

Pedro J. Fernandez’s picture

@eugene.ilyin

Thanks for your fast answer! But I have tryed this and I get the same result.

{
  "_links": {
    "type": {
    	"href" : "http://www.example.com/rest/type/file/file"
     }
  },
  "filename":[{"value": "test122.jpg"}],
  "filemime":[{"value":"image/jpeg"}],
  "uri":[{"value": "public://photos/test122.jpg"}],
  "data": [{ "value" : "/9j/4Rd9RXhpZgAASUkqAAgA .......   "}]
}

And I get this as result:

{
  "fid": [
    {
      "value": "81"
    }
  ],
  "uuid": [
    {
      "value": "217abec0-699a-4b03-a9a2-08d9401627d0"
    }
  ],
  "langcode": [
    {
      "value": "en"
    }
  ],
  "uid": [
    {
      "target_id": "1",
      "target_type": "user",
      "target_uuid": "3096cd62-05b9-417e-841e-d2072c9e93f4",
      "url": "/es/user/1"
    }
  ],
  "filename": [
    {
      "value": "test122.jpg"
    }
  ],
  "uri": [
    {
      "value": "public://test122.jpg"
    }
  ],
  "filemime": [
    {
      "value": "image/jpeg"
    }
  ],
  "filesize": [
    {
      "value": 297880
    }
  ],
  "status": [
    {
      "value": false
    }
  ],
  "created": [
    {
      "value": 1471436748
    }
  ],
  "changed": [
    {
      "value": 1471436748
    }
  ]
}

The patch I have applied is the 1927648_216.patch, not the last one, over a drupal 8.1.8. May be this is the problem.

To mitigate any kind of doubt, I have applied the last patch available (serialize_file_content-1927648-251.patch) in a brand new drupal 8.1.8. I have updated my drupal solution with it and the I tried again.

I have to say that every time I made a change in code, I creared the cache before testing again.

First of all, 403 Forbidden message appeared. I remaind that I have to change something to get it work last time, concretely this in the file core/modules/file/src/FileAccessControlHandler.php:

  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    // The file entity has no "create" permission because by default Drupal core
    // does not allow creating file entities independently. It allows you to
    // create file entities that are referenced from another entity
    // (e.g. an image for a article). A contributed module is free to alter
    // this to allow file entities to be created directly.
    //return AccessResult::neutral();     <-- Remove neutral and leave allowed here
    return AccessResult::allowed();
  }

Now, 201 Created received, but same behaviour. Files are created in the public:// root folder but destination folder is ignored. Nothing has changed compared with the previous patch.

There are more patches I have to apply apart from this?
I'm a bit frustrated with this issue :-(

eugene.ilyin’s picture

@Pedro, I've used patch from #251

benjy’s picture

FileSize
23.89 KB

I re-rolled this against 8.1.x as that's what i'm currently developing against. I got this working but note that since #2310307: File needs CRUD permissions to make REST work on entity/file/{id} you need to handle enabling the "create" permission for file entities. I presume installing the File Entity module will solve that, for my use case I created a simple module that allowed access to create file entities to anyone with the restful post entity:file permission. Module is simple for anyone that is interested:

// In the module file
function MODULE_entity_type_alter(array &$entity_types) {
  $entity_types['file']->setAccessClass('Drupal\your_module\FileAccessControlHandler');
}

// In FileAccessControlHandler
<?php

namespace Drupal\your_module;

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Session\AccountInterface;
use Drupal\file\FileAccessControlHandler as CoreFileAccessControlHandler;

class FileAccessControlHandler extends CoreFileAccessControlHandler {

  /**
   * {@inheritdoc}
   */
  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    return AccessResult::allowedIfHasPermission($account, 'restful post entity:file');
  }
}

Might also be worth pointing out that you have to use application/hal+json as the Content-Type, I was previously using JSON and that doesn't work because the FileNormalizer is in the hal module and hal_json is the only supported format for the normalizer. Finally an example JS payload might help others:

      let response = await fetch(this.baseUrl + '/entity/file?_format=hal_json', {
          method: 'POST',
          headers: {
            'Content-Type': 'application/hal+json',
            'X-CSRF-Token': token,
          },
          body: JSON.stringify({
            _links: {
              type: [{href: this.baseUrl +'/rest/type/file/file'}],
            },
            filename: {
              value: file_name,
            },
            filemime: {
              value: file.content_type,
            },
            data: [{
              value: file.data
            }],
          }),
        });
benjy’s picture

Status: Needs work » Needs review
FileSize
24 KB

Here's an 8.2.x version

tinom’s picture

I have applied patch 1927648-264.patch to 8.1.x successfully, added File Entity module but now the error is:

rest/type/file/file does not correspond to an entity on this site.

Any idea what I am missing here?

Also, I want to upload the image not as a standalone entity but to a specific object. How can that be done?

larowlan’s picture

Issue tags: +Default content
tedbow’s picture

@Pedro J. Fernandez, @eugene.ilyin, @benjy, @tinom please don't use this issue for support requests for a patch in progress, or post backported patches to previous core versions that this issue does not relate to.

This issue is over 260 comments and it still needs work. It makes it very difficult for people who want to come work on the current patch to understand the issue because they have to read through so many comments. This is even worse if they have to weed through comments that are not about developing the current patch.

Thanks

benjy’s picture

Well, this is the canonical place for the problem of uploading files via REST and 8.1 is still the stable version of Drupal, so if we don't post patches here I'm not sure where else people would find such patches.

kylebrowning’s picture

Im actually going to make a case for switching this to use multipart form encode or maybe BSON, https://en.wikipedia.org/wiki/BSON

- base64 encoding makes file sizes roughly 33% larger than their original binary representations
- base64 encoded data may possibly take longer to process than binary data

This results in loss of data/compression in 2 areas, and strings will be huge. For Example a 1.6kb image is a string 1700 characters long, making a payload to upload a 5 mg image just insane.

What this means is that you will need more memory (in your browser or mobile device) than before just to upload an image/video.

With Mobile devices getting bigger and bigger cameras, this will cause OOM errors in many places. And its not just with mobile, browsers will have this issue too.

With multipart, we can send a fully represented binary object, and save the heavy memory footprint because its not a string. Of course the dev will still resize cause they can't upload a 200 mg video, or 20 mg picture.

#252 addresses this in a very nice way. Im going to provide a patch based on that.

garphy’s picture

#271: +1

I would add :
- the memory consumption is also a server-side issue, you cannot require the server to allocate the file size of each upload in memory, it would obviously not be scalable ;
- there's no way (of I'm aware of) to provide a file upload progress status to the user when submitting a JSON blob.

Evgeny_Yudkin’s picture

I tried attached patches:
1) There are issues with jsonapi (type errors into the serializer constructor);
2) file_entity module worked for me (no need any patches);

By the way, all patches affects "hal" module, but what is somebody wants to create file via json/xml/any other format?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kim.pepper’s picture

Re-roll of #265 for 8.2.x

* core/modules/hal/tests/src/Kernel/EntityNormalizeTest.php was removed in #2824576: Delete old REST test coverage: (Read|Create|Update|Delete)Test, deprecate RESTTestBase

Wim Leers’s picture

Status: Needs review » Needs work

Thanks! But #271 still needs to be addressed, so back to NW…

Wim Leers’s picture

Category: Bug report » Task

This is not a bug. It's something very important capability that is missing. Technically, this is a Feature request, but because it's such a glaring omission, I think Task is more appropriate.

kylebrowning’s picture

Should we update the title too Wim?

"Allow REST File uploads via binary."

bradjones1’s picture

The re-roll in #276 no longer applies, I tried some manual re-working but the array syntax patch for core paired with some changes in the test classes in the interim made it a bit unwieldy.

I would agree with assessing the issues re: this only applying to HAL at the moment, along with the concerns over Base64 vis-a-vis upload size. In the interim, rather than waiting on core support for this I believe the FileEntityNormalizer that appeared in these patches could live in custom/contrib land for the time being, albeit without the tight integration with core.

damiankloip’s picture

OK, here is a new approach. I think it was discussed previously in part. However, it's a new approach from the standpoint that there is new code here. For now, I am disregarding the previous patches. We can revisit/re-integrate stuff needed to this. #280 is correct too, there are a lot of changes to reconcile anyways...

I have spoken to a few people about this. The only way I see to solve this properly is to expose a custom route that only deals with POSTing binary file data. We can then stream the data properly (I think) without always loading the whole file contents into memory. Using this approach locally, I was easily able to save files much much larger than my PHP memory limit. To do this, we obviously need a stand-alone endpoint only for binary data. So creation of the file entity would need to be done in a separate request, so you could obtain an ID. I think that is acceptable though

This patch is still very rough, but for now it just provides a new route and controller to handle POSTing binary data only. So E.g. if you were using curl to do this:

curl -v --header "Content-Type:application/octet-stream" --data-binary @/path/to/your/file http://d8-dev/file/upload/{FID}

Wim Leers’s picture

So creation of the file entity would need to be done in a separate request, so you could obtain an ID. I think that is acceptable though

Yes.

Especially if this can be combined with https://www.drupal.org/project/subrequests.

damiankloip’s picture

Here is a bit more work on this. Thinking about it, I think we need to do this (which this patch starts to do):

  • As in the previous patch - have an endpoint only for dealing with uploading actual binary file data, requiring a file entity and access to it
  • Remove the hal FileEntityNormalizer, it does the HTTP fetching we don't really want, doesn't use anything from the parent normalizer, and uses ->create() directly to create the entity instance. We have really tried to avoid this, as field level denormalization is never called. This will now make file entities behave like any other content entity, you update fields on it. So REST endpoints would behave as normal, disregarding the actual file data

Problems we need to work out here still:

  • Still decide who is allowed access to this endpoint, via file update access checking or additional/alternative custom logic. We might want to remove the changes to the FileAccessControlHandler logic made in this patch too?
  • Still need field item level normalization I think, as E.g. hal will show the 'self' href as the actual file URL only, we would want this to be the REST file endpoint instead I think? regular formats like JSON should be OK - we get target_id, target_uuid, and URL - so still usable

This obviously all still needs some additional test coverage too!

damiankloip’s picture

Couple of other improvements to the upload controller.

dawehner’s picture

  1. +++ b/core/modules/file/file.routing.yml
    @@ -4,3 +4,14 @@ file.ajax_progress:
    +  requirements:
    +    _entity_access: 'file.update'
    

    Is there no file.create entity access?

  2. +++ b/core/modules/file/src/Controller/FileUploadController.php
    @@ -0,0 +1,96 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('file_system')
    +    );
    +  }
    +
    +  /**
    +   * Constructs a FileUploadController instance.
    +   *
    +   * @param \Drupal\Core\File\FileSystem $file_system
    +   */
    +  public function __construct(FileSystem $file_system) {
    +    $this->fileSystem = $file_system;
    +  }
    

    Personally I prefer to have the constructor first, given that its more important and the create() method is just an implementation detail.

  3. +++ b/core/modules/file/src/Controller/FileUploadController.php
    @@ -0,0 +1,96 @@
    +    if ($request->headers->get('Content-Type') !== 'application/octet-stream') {
    +      throw new HttpException(400, '"application/octet-stream" content type must be used to send binary file data');
    +    }
    

    Isn't that a classical 415 error?

  4. +++ b/core/modules/file/src/Controller/FileUploadController.php
    @@ -0,0 +1,96 @@
    +  protected function streamUploadData(File $file) {
    

    Should we use FileInterface here?

  5. +++ b/core/modules/file/src/Controller/FileUploadController.php
    @@ -0,0 +1,96 @@
    +      while(!feof($file_data)) {
    

    Nitpick: Missing space after the while

  6. +++ b/core/modules/file/src/Controller/FileUploadController.php
    @@ -0,0 +1,96 @@
    +      // Move the file to the correct location based on the file entity,
    +      // replacing any existing file.
    

    I'm wondering whether its the right thing to replace existing files.

  7. +++ b/core/modules/file/src/Controller/FileUploadController.php
    @@ -0,0 +1,96 @@
    +      if (!file_unmanaged_move($temp_file_name, $file->getFileUri(), FILE_EXISTS_REPLACE)) {
    +        throw new HttpException(500, 'Temporary file could not be moved to file location');
    ...
    +      $this->getLogger('file system')->error('Temporary file "%path" could not be opened for file upload', ['%path' => $temp_file_name]);
    +      throw new HttpException(500, 'Temporary file could not be opened');
    

    Why do some of those exceptions cause a logging, and some don't?

damiankloip’s picture

Hey Daniel!

so...

1. This is only for updating file data, not the file entities, you would have to already have a file entity at this point. So it's only ever an update kind of?
2. fair
3. Yes! This is the kind of thing that needs to be tidied up. Unsupported type is correct.
4. Yep, meant to use that!
5. fair
6. If you don't replace them, you would get lots of orphaned files? and you would need to update the actual file entity fields too? the size, type, name, everything could change. So then you are back in to create territory?
7. file_unmanaged_move generates its own logging upon failure.

damiankloip’s picture

Let's just make most of those changes now to keep on top of things.

Berdir’s picture

File upload in the UI in core always happens *in the context of something* that control upload validation. File validation is configured on the field. Allowed extensions, size, ...

Somehow, we need to be able to respect that also in REST, otherwise you can upload whatever you want. file_entity does have a global file upload, but it also has permissions for that, list of allowed file extensions and so on. All of that is completely non-existent in core.

So IMHO, we need to limit file create/upload to the context of something, we could hardcode a path that contains entity_type/bundle/field/.. or we could try something more generic, but not sure how to do that.

Another idea would be to be able to upload raw binary data into a very temporary state that is *not* in any way accessible, and then you can reference that from the serializer and we create and validate the file entity at that point.

damiankloip’s picture

Good points @berdir, this is certainly something we need to think about, not sure how best to solve that at this point. I guess the trouble is that we need the file content before we can do anything with it, and allowing changing the extension etc.. could pose some security weaknesses?

This is maybe slightly different to file_entity in that case... You could currently upload anything you want for the file data BUT you cannot modify the file name or anything else about the file (entity). So maybe if that was the case, we don't need to validate anything on the upload path? Maybe just if the file entity field values are modified via REST? We could for example, validate the content-length is not above the max upload size or something? As it currently stands, you could upload any binary data you want, but you could not change file path or extension.

Or do you mean we need to think about this regular REST operations on the file entity? E.g. just to be allowed to create a file entity with x extension type.

damiankloip’s picture

WIP on some test coverage for the file data upload portion.

damiankloip’s picture

Trying to change the test to use BrowserTestBase like most REST tests do. The actual controller code seems to work ok, I am not sure what's going wrong with authenticating a user though. For now the access it just 'TRUE'.

jibran’s picture

Status: Needs work » Needs review
+++ b/core/modules/file/src/Controller/FileUploadController.php
@@ -0,0 +1,99 @@
+    $temp_file_name = $this->fileSystem->tempnam('temporary://', 'file');

Shouldn't this be some random name?

damiankloip’s picture

tempnam() creating a file with a unique name isn't enough?

Also, intentionally didn't mark it as needs review because we don't need the bot to run at this point...

jibran’s picture

tempnam() creating a file with a unique name isn't enough?

Yeah, that's fine. I forgot about that.

we don't need the bot to run at this point.

Sorry about that.

damiankloip’s picture

ok, and ok. I guess :)

damiankloip’s picture

It seems like the best way forward (IMO)may be to not provide this in the context of a node or parent entity (we could do this, but seems like the wrong direction - to try and bring in field level validation amongst other things, seems strange from a REST perspective). File entity adding it’s own endpoint for creating files seems like a good plan for core too. It could then just rely on a new file permission like ‘create file entities’ or something? We could then have a way to create the actual file entity. Then use the endpoint in this patch to post larger amounts of file data if needed? Should we allow smaller amounts of file data to be POSTed inline still (I vote no)?

To achieve this, (I think) we would need:

- The new file permission
- Adjustments to the FileAccessControlHandler to take this into account
- Provide our own REST resource implementation for File entities?
- A way (as berdir mentioned above) to configure a list of allowed file extensions

This would make files kind of a special case I guess, but that seems like a necessary evil if we want some functionality.

dagmar’s picture

Just to add some extra thoughs on this. The binary upload approach is something that is already implemented by other API services like contentful: https://www.contentful.com/developers/docs/references/content-management...

Another idea would be to be able to upload raw binary data into a very temporary state that is *not* in any way accessible, and then you can reference that from the serializer and we create and validate the file entity at that point.

This is what contentful does too.

From contentful docs:

If the association and processing steps are not executed successfully within 24 hours after uploading, the file and its metadata will expire and be deleted from the storage area.

garphy’s picture

TL;DR: My point is that we should have one single API endpoint which handle the file upload (using "real" PHP upload semantics) AND create a temp File entity in one single atomic request.


Good finding @dagmar ! The approach taken by Contentful is IMHO the right one and it's basically how Drupal is already handling file upload through HTML forms when using ajax uploads : the file is uploaded through ajax, stored temporarily and if the form itself is not successfully submitted within a certain time, theses temporary files are/can be deleted. But it's at a higher API level though : it's about creating a temporary File entity upon file upload (in one single request), then associate the File entity to node (or not).

As this issue is currently going, we end up with two options :
- create a temporary File entity which references no actual file, then upload a file by passing the fid in the request ;
- or, upload the file and get a "temporary upload id", then creating a File entity with this upload id.

But maybe these two requests approaches (one for creating a file entity and one for uploading) are equally flawed(ish) no matter the ordering.

Do we actually discussed the fact of having one single API endpoint which handle both the file upload (in a way we get streaming, progress, ...) and file entity creation (like the approach it was suggested in #252 and #258) ? It's not the route we're currently on, but I don't think we properly discussed and dismissed (if needed) this approach.

@Wim Leers, in #282, I don't get if you agree that the current approach needs two requests or if you consider it acceptable. IMHO, you couldn't combine those two requests, even using subrequests, in a way which allow for a streamed upload . I think that you would end up on the same issue again : base64 (or other) encoding, memory consumption, no upload progress, ...

I'd love everyone's feedback there :)

damiankloip’s picture

Well, the main thing the current approach is trying to implement is the binary upload of data part. The rest is up for discussion as far as I'm concerned. Once we have the binary upload working (it does now) the main concern is how we handle permissions around the file creation - this is the same problem to solve regardless of the direction we go in. We also still have the validation issue - but hopefully file validation can happen when the entity is validated and the file validators run at the field level for the file used. I.e. you could upload any file you like, then reference it on a node or something, but if it doesn't match the validation criteria for that field configuration the parent entity would fail validation.

If we go with the temporary file approach, how would you then switch the status of the file entity? You would still need another request I think? So then you could need 3 requests? Or would saving the entity after denormalization then update the file status as it has now been referenced? The other problem with that approach is how we deal with the file name? You would still need another request to rename it, even if we did give it a temporary name before the file is used? OR our endpoint could be /file/create/{filename} or something?

I don't think we should work with temporary file IDs, then create a file entity, I think we should always create the file entity, just with a status of FALSE if we're going down that route.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
15.47 KB
2.42 KB

Restoring the hal file entity normalizer so we can have to full file URL back instead.

ibustos’s picture

I can confirm #307 is working great. It would be neat if the response included the file being created/updated instead of just an "Ok" though. Submitting binary data is so much better than base 64. Quick question though, shouldn't this functionality be part of the Rest module instead of the File module?

ibustos’s picture

I was thinking something along the lines of this.

ibustos’s picture

FileSize
1.88 KB

Forgot to include interdiff.

Status: Needs review » Needs work

The last submitted patch, 309: 1927648-309.patch, failed testing.

damiankloip’s picture

Thanks for testing! Yes, returning the serialized file data makes sense too. I was thinking similar, just waiting to see if we want to create the file entity too, as per comment #306.

I was also going to move this to REST module I think, it started out in file module whilst I was scratching around. I think we need it in REST module, with the endpoint/route being added when file module is enabled (pretty much always).

damiankloip’s picture

+++ b/core/modules/file/src/Controller/FileUploadController.php
@@ -59,7 +65,9 @@ public function upload(Request $request, FileInterface $file) {
+    $normalized_file = $this->serializer->normalize($file);
+    return new Response($this->serializer->encode($normalized_file, 'json'));

Instead of this, we should just call serialize()

damiankloip’s picture

The only thing that was bugging me a bit (and why I just returned the string before) is its kind of weird to send a binary request and receive a json response? I guess that's an ok default.

Oh and..

Submitting binary data is so much better than base 64

YES!

damiankloip’s picture

Thinking about this some more today, I think this might be a good path to try and go down:

- /file/upload endpoint also creates files, but they are temporary only : To solve - how do we name the file (path parameter, query string, header)?
- Make sure FileItem validation can happen on save (of the parent entity, through constraints), this will allow us to arbitrarily use/reference our uploaded files, but then they could still be validated in the context of where they are being used as things like max file size, and extensions are field specific
- When the parent is saved (FileItem?) if it's referencing a file that is temporary, it needs to set the status for the file entity to make it a permanent file. Could also move file if needed at this point too?
- Create new permissions in file module specifically for uploading temporary files

So the flow would then be:

- Create new temporary file at /file/upload and store the file ID you get as a result
- Use this in a file reference field on a parent entity

Also, me and dawehner were talking, and agreed that it does makes sense to keep the FileUploadController and route definition in the file module, as it is not really implementing anything from REST module.

dabito’s picture

@damiankloip @ibustos

- /file/upload endpoint also creates files, but they are temporary only : To solve - how do we name the file (path parameter, query string, header)?

How do we deal with security here? Where would we get the constraints from for this arbitrary upload?

One solution is to add the entity/bundle/field information to the request, but I'm worried about tightly coupling to the field module.

Another solution is to pass constraints in the request itself, but this could create a sensible vulnerability.

Maybe add a config which defaults to a list of accepted mime types for all uploaded temp files?

Berdir’s picture

Yes, I brought that up in #291 as well, some responses in #292 and #299.

I'm not sure yet. I think having good validation for this is absolutely critical and we need to be extra careful here. I get the point about not tying too much to entity/field, but REST is all about entity/field (validation) and field validation falls back to upload validators too.

Either we need to validate it there and upload in context of some field, or we need to ensure the validation happens while it is added to an entity *before* the file is made publicly available.

damiankloip’s picture

Yes, we need to be super careful here. The latter is the only sane way to go I think (I don't know a lot of these systems as well as you do). I am hoping we can only allow uploads of temporary files, they can then only be made into 'real' files once attached to a parent and referenced. If we could have file validation running when entities are saved (or just validated) via some constraints, I think you could E.g. attach a temp file via the ID to your article, then have it validate based on (E.g.) the image field configuration on that. Is that possible?

Currently a lot/all of the validation is just at the form level. If the file is only temporary, and validation happens like it does now. We should be able to utilise current mechanisms for validation of the actual file? Also, this is not going to be a publicly accessible endpoint. If we can, it would be good to keep any specific field knowledge out of these requests.

As berdir suggested a while ago, I think, we could do something similar to file entity module, the endpoint to allow uploads has a whitelist of extensions too. So it could be limited there, and further validated at the field level. It was discussed previously, and I think it's a good option.

dabito’s picture

@Berdir @damiankloip @ibustos

So we basically have the contextualized upload and the arbitrary upload. While the arbitrary upload is decoupled, it is way more insecure. Adding context to this upload process lets us ensure only validated files ever make it into the server.

With the contextualized upload we would require an owner, an entity/bundle and a field.

Entity/bundle could be part of the URL, so the endpoint URL could be /file/entity/bundle/field/upload

I would agree with Berdir's #291 since the above URL looks too long. So maybe just /entity/bundle/field and use PUT request?

Original file name could be passed using the content-disposition header (Do we need to change to multipart/form-data?).
Content-Disposition: file; filename="file1.txt";

damiankloip’s picture

Either way, we still don't have a proper way to validate the files being uploaded right now, as it's all done at the form layer. If we have a whitelist of allowed extensions, I don't think that would be way less secure? Seems like it could be ok.

How do you see the flow working (request wise) if we go down the field context route? Upload file data for /entity/bundle/field get your file ID, then make another request to use that file ID on your parent field reference field? You could still POST data for field X, then attach it to field Y once you have the file ID if you wanted to anyway (unless we move file validation as I mentioned in #318)?

I think Content-Disposition is a response header really?

working on some changes to the patch currently. Won't change too much until we settle on some of these other points.

ibustos’s picture

So the way it would work IMHO, would be as follows:
1. Have users POST to say /file/upload/{entity_type}/{bundle}/{field_name} while optionally specifying a file name (I liked the idea of using Content Dispoition).
2. All validation for that field should be done at this point for security purposes (we don't want users uploading .exe files and the like). Once we have the file we run all validations. If we need to save the file for this purpose, the file must be set to TEMPORARY.
3. Once the file has passed validation, create the File entity (and move it to PERMANENT?), if it doesn't pass, attempt to remove the file.
4. Since users using this functionality are must likely using rest or Jsonapi, in order to append the new file to a field, all they would have to do is just do another POST or PATCH request to that entity endpoint to assign a value for that field.

What are your thoughts on this?

ibustos’s picture

This patch adds an endpoint to create files. The filename is being retrieved from the header using @dabito's suggestion for the time being. Also there is no validation whatsoever yet.

dagmar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 322: 1927648-322.patch, failed testing.

damiankloip’s picture

But POSTING to an endpoint like /file/upload/{entity_type}/{bundle}/{field_name} will be fine, but then what's stopping you then using that file for another entity/field instead? Not much, I don't think. This is why I prefer the idea of having a whitelist of file extensions on the endpoint, then actual validation on the referenced file.

Berdir’s picture

File/image field entity validation should cover everything that upload covers as well. See \Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator

The problem is to figure out a way to actually validate a file while it's still in temporary:// and *after* that move it to public. That is however actually quite complicated, as we have no such logic right now, we'd need to add something, like a preSave() in FileItem, but based on what should it act?

The advantage of uploading for a specific field is that we don't have to worry about temporary:// IMHO. Because we can just create it as a normal public:// (or whatever is configured on that field) temporary (the status, not the location) file, just like when you upload in the UI. It would also fix the permission problem, because we can easily check edit access to that specific field for that node type. That means you can really only upload files if there's at least one file/image field on an entity type/bundle that you are allowed to edit.

damiankloip’s picture

Nice, thanks berdir. Missed that validator! That is good news.

Yes, it's the moving of the file that could be really problematic indeed. So we should still have an endpoint but the access can check field access for the user instead. I guess you could still just use the file somewhere else, but that doesn't seem to bad, as long as it's valid (which seems will be taken care of already).

@berdir, do you have any preferences about the how we allocate a file name during the upload to this endpoint? As mentioned in #320, it seems the content-disposition header is not really a good choice, as that's really meant to be a response header only.

Wim Leers’s picture

I've caught up on this issue starting at #291, i.e. from around the time I last reviewed this issue. This issue was re-kick-started around the end of DrupalCon Baltimore (which was April 24–28, 2017, comment #294 was on April 25, 2017), and after that I was on vacation.

I commented on the comments where I think I can add something meaningful, express my agreement, or disagreement.


#304: thanks for pointing at the Contentful example. That's another indicator we should take this approach. Twitter does something similar: https://dev.twitter.com/rest/reference/post/media/upload — and it explicitly returns the number of seconds until it expires. Although it also supports multi-step uploads:://dev.twitter.com/rest/reference/post/media/upload-init.html + https://dev.twitter.com/rest/reference/post/media/upload-append + https://dev.twitter.com/rest/reference/post/media/upload-finalize — for a concrete example of how to use that, see https://blog.twitter.com/2015/rest-api-now-supports-native-video-upload.


#305: I think two requests is acceptable. You're right that my suggestion in #282 was misguided, because when using https://www.drupal.org/project/subrequests, you wouldn't be able to use streamed uploads. So, +1 for two separate requests: one for uploading a file, another for associating it. Or, if uploading many files, N+1 requests: N file upload requests, 1 request to associate them all with the same entity.


#306:

I.e. you could upload any file you like, then reference it on a node or something, but if it doesn't match the validation criteria for that field configuration the parent entity would fail validation.

+1

If we go with the temporary file approach, how would you then switch the status of the file entity?

I think the act of referencing the temporary file from an entity's file field would cause that file to be marked permanent.

Or would saving the entity after denormalization then update the file status as it has now been referenced?

Yes.

The other problem with that approach is how we deal with the file name? You would still need another request to rename it, even if we did give it a temporary name before the file is used? OR our endpoint could be /file/create/{filename} or something?

I think the "upload binary file" request should indeed specify the filename. How exactly, I don't know. Possibilities are:

  1. URL path part (this is what Dropbox does: https://www.dropbox.com/developers-v1/core/docs#files_put
  2. request header
  3. URL query argument.

I don't think we should work with temporary file IDs, then create a file entity, I think we should always create the file entity, just with a status of FALSE if we're going down that route.

AFAICT that'd be okay. But then we'd still need to specify the file name in that request.

The advantage of a separate endpoint for just "raw uploads" would be that we could have more strict (and better tested) clean-up procedures for unused temporary files. But if we feel that this already is sufficiently reliable, well-tested, and so on, then I agree that ideally we'd just reuse the "temporary file while uploaded and made permanent when referenced" strategy that core has already been using for file uploads for many years.


#312:

I was also going to move this to REST module I think, it started out in file module whilst I was scratching around. I think we need it in REST module, with the endpoint/route being added when file module is enabled (pretty much always).

I don't understand this. If it's specifically tied to File entities, then why should it not be provided by the file module?
EDIT: hah, you already came to this conclusion after talking to @dawehner in #315 :)


#315++


#316 + #317 + #318: RE: security. I think this is the answer:

we need to ensure the validation happens while it is added to an entity *before* the file is made publicly available.

… which is to say, this is once again just the "normal" field validation constraints being applied: when saving e.g. a Node that has a file field, then this is what happens:

  1. Upload a file using this new binary file upload resource, which returns a file ID, and stores the file in the temporary:// stream wrapper
  2. POST (or PATCH) a Node that lets the file field reference the file ID from point 1
  3. During this POSTing (or PATCHing), the entity is validated, which causes each modified field to be validated, which includes the file field.
    The file fields' validation constraints run, which are identical to the validation that is ran during file uploads for the "regular form file upload widget".

We should be able to utilise current mechanisms for validation of the actual file?

We should indeed. If we can't, then as part of this issue (or as a blocker of this issue), we should refactor the existing form-tied logic into proper Field Validation Constraints. I'm quite hopeful though, because \Drupal\file\Plugin\Field\FieldType\FileItem has this:

 * @FieldType(
 *   id = "file",
 *   label = @Translation("File"),
 *   description = @Translation("This field stores the ID of a file as an integer value."),
…
 *   constraints = {"ReferenceAccess" = {}, "FileValidation" = {}}
 * )

and \Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator has this:

    $validators = $value->getUploadValidators();

#319: I honestly don't yet see why we'd want/need to tie this to an entity/bundle and a field. What matters, is that a certain file field is allowed to use a particular file. Therefore the validation of the entity that contains the file field is what/when/where the validation should happen.


#326:

File/image field entity validation should cover everything that upload covers as well. See \Drupal\file\Plugin\Validation\Constraint\FileValidationConstraintValidator

Yay, glad to see what I wrote confirmed :)

The problem is to figure out a way to actually validate a file while it's still in temporary:// and *after* that move it to public.

Why is this a problem? Surely we already need to do this for our form-based file uploads too?

That is however actually quite complicated, as we have no such logic right now, we'd need to add something, like a preSave() in FileItem, but based on what should it act?

Apparently we don't?! :O :O :O

The advantage of uploading for a specific field is that we don't have to worry about temporary:// IMHO. Because we can just create it as a normal public:// (or whatever is configured on that field) temporary (the status, not the location) file, just like when you upload in the UI.

… but … that can easily cause you to end up with lots and lots of unused "permanent" files? And it's also semantically wrong/strange/confusing?

Also, the downside is that you have to somehow figure out the particular magical URL for file uploads for every single field + entity type + bundle combination… which is a bad DX.

WRT the Content-Disposition header: it seems it's actually allowed for POST requests when uploading files, see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Dispos.... For a concrete example of how that can be implemented for a REST API, see https://github.com/SparkPay/rest-api/blob/master/uploading_files.md.

Note that another consequence of using multipart request bodies is that we can support uploading multiple files in a single request. But I don't know if it's compatible with application/octet-stream?

Berdir’s picture

There are two different kinds of "temporary" concepts. One is the *status* of the file entity. A freshly uploaded file with status temporary is still in public:// and publicly available. Otherwise we wouldn't be able to show that file or e.g. a thumbnail of it.

The other is a file that is in the internal, non-public/accessible temporary:// stream-wrapper/directory. We have no support to automatically move those files. We would just make it a permanent file (file that is not being deleted after 6h) but it's still in the temporary folder and not publicly accessible.

That's why my suggestion is to enforce the upload in context of a field, because then it is the same as upload in the UI. We can make it a public, temporary file that will get deleted after 6h (we don't even need to do anything custom for that), and actually referencing it will make it permanent.

damiankloip’s picture

OK, spoke to Wim and Berdir in IRC for a while and here are some more changes, we all agreed in the end that keeping in bundle and field specific is the way to go. So we can then just create the file in place and don't need to worry about moving it etc... This is still a WIP, just ran out of time today.

This also converts things back to one route/endpoint that can create files, no update only endpoint, as well as converting to a rest resource instead. We should then get serialization handled for us.

There are still some todos, we need the validation in place based on the field definition being used, amongst other things.

damiankloip’s picture

And a couple of other fixes.

Wim Leers’s picture

we all agreed in the end that keeping in bundle and field specific is the way to go.

I did not agree with that.

The uploading should be agnostic of entity type/bundle/field. Then when you reference the uploaded file from a file field, the necessary validation should run.

damiankloip’s picture

ok, I thought that we agreed it made sense for now to go with entity/bundle/field as we can place the file in it's actual location - like file uploads happen in the UI. If you want to try and also solve how we would move files when they are referenced, let me know :) Otherwise, all uploads would have to just sit in public:// or something. That said, I would still be happy to just upload temp (status = 0) files to public that would be either referenced and made permanent, or deleted on cron runs.

Ideally that would be the case, but I'm not sure we really have much of anything in place to validate and move temporary files when a parent is saved.

Berdir’s picture

The uploading should be agnostic of entity type/bundle/field. Then when you reference the uploaded file from a file field, the necessary validation should run.

No, uploading is not agnostic of that. There's not just validation, the field also controls where the file is stored (private:// or public://, but it could also be on amazon s3 through flysystem module or who knows where) as well as the folder (media/YYYY/MM for example).

It is important that we respect that configuration. if this happens to be your CV that you upload to a job application form the you don't want that to end up somewhere public :)

And doing all those things *only* when attaching to an entity is complicated, it is possible to re-use files in multiple places even if those settings are not the same (they only matter for the initial upload), so we would need to figure out when to move and when not to move files and so on.

This is a first step and it reflects how file uploading works in drupal core. After that, we can think about improving security, also for the UI (which is a lot harder than you might think and there's a reason it wasn't done yet), there are ways to integrate better with media enties so you don't need 3 requests there, and contrib modules like file_entity could offer a standalone upload resource. And a module like flysystem_s3 could even offer a REST resource to create a file that was directly uploaded to S3, it already has ajax integration for that.

dabito’s picture

Great job with the rest resource, I really like that strategy. I've two comments:

1) If we're going the Content-Disposition way for filename, we should be using multipart as that's the only way the header should be used in requests. Also there must be a better way to obtain the attribute other than the regex, as the todo mentions.

2) Regarding the agnostic upload, what do you guys think of the quarantined file strategy mentioned earlier in the thread? The full approach could look like this:
- single upload endpoint is requested *
- if it has context then follows the existing path *
- if there is no context, though, it could create a quarantined file entity which is hidden (random or hashed filename located in tmp)
- this file entity can be the upgraded to a permanent file managed entity upon parent entity create / update
- a cron job will delete all quarantined files older than 1 hr
* what we have so far

damiankloip’s picture

Thanks :) I think the rest resource could be a good way to go. It's currently a bit strange with how REST handles allowed content types though... so we might need to look at that a bit more.

1. I don't think we should go with that header, I just left it in for now, along with the todo. I think we should just go with something else that only consists of a filename for now. I think we should stick to the single file per request for now, we need the simplest implementation in IMO, so parking for now is multipart is sensible.

2. I think that might be easier said than done. I'm not sure we can move files like that when a parent is updated at this stage. It get's tricky, and nothing like that currently exists in core. If it was a random or hashed filename, renaming it on parent create is also something that in no way exists. So we would need a way to rename it again, or you need to add another field to file entities.

kylebrowning’s picture

I agree, do single requests for now, this is the last feature IMO for a fully backend capable Drupal. Keep it simple we can always add more later.

Just my opinion though.

garphy’s picture

Basically, Drupal core currently offers no way (through UI) to manage file entities directly. Everything is done through File/Image fields.

So we stuck with either :

  • implement an upload endpoint which need contextual info about the targeted field, or
  • allow the creation of a file entity on its own (but it's a wider scope than just adding an API endpoint)

Providing contextual info about the field seems fragile, as already noted : nothing would prevent from referencing the created file from another entity/field. Still, I do not see clearly how we would manage the temporary/permanent status when POSTing the entity referencing the file.

Allowing the creation of a file entity
is another can of worms but it's probably the best way ultimately. It's basically what the media initiative is working on but I did not manage to find an existing issue on the specific topic of adding a new file to Drupal (adding a "new file" button to admin/content/file).

In the end, we have to face that files are entity as nodes are, we cannot keep on handling it like a piece of content which only belong to one entity like text fields. We have to decouple the lifecycle of a file entity from the lifecycle of its referencing entity (and figure out what happens to validation).

(BTW, the issue title seems pretty outdated now :)

garphy’s picture

Well, per #2825215: Media initiative: Essentials - first round of improvements in core (if it lands), it seems that we are gonna deprecate File/Image field in favor of using the Media entity type with plain Entityreference fields.

So maybe it's better to focus our effort on "how to upload a file which will create a Media entity referencing a File entity" ?

damiankloip’s picture

I think still being able to upload a file is useful too. Media can then build on that (discussed briefly with Berdir yesterday).

The trouble with the standalone file upload endpoint (which I would prefer too) is that we have to decide where the file will live, so we could default to public:// for example, but then a field could be using a completely different storage location (or even different backend, like s3), and moving the file when the parent is saved seems like a no-go for now. Otherwise, as mentioned up somewhere, we just have a whitelist of file extensions (or maybe just validators that run) for file uploads. Then they can be used, and the parent entity would fail validation if the file did not meet the validation criteria.

Ideally files would be more standalone. but Drupal just doesn't seem to be there just yet. So it seems sensible to go with Berdir's suggestion for now, and implement something more akin to how file uploads work in the UI.

Wim Leers’s picture

#333: I agree that it's better to have entity type/bundle/field-bound file uploads is better than not having at all. The problem with that though is that we have to support it forever, if we commit it and ship it in 8.4.0. I do definitely think that it's a great first step for this patch, I'm only saying we should very carefully considering the implications of shipping a Drupal release that supports it. But let's worry about that later.

#334: Ugh, I totally forgot about where it may be stored (which can be anywhere when using flysystem). Which suggests that we'll need to ship with it working in the way as described in #330 anyway :(

#335: in #330, the context is provided via the URL path. "Contextless file uploads" are therefore impossible in that approach. And per #334, it seems like it's highly undesirable/unsupportable/will require massive rearchitecting to support contextless file uploads.

#337: Keep it simple we can always add more later. This is the guiding principle for me in my comments. And I was under the impression that contextless file uploads would be simpler, but #334 explains convincingly why that's not.

#338: I very much agree that files with context are fragile, because if a file is uploaded via an image field to a particular file directory and then later referenced from a file field that by default uses a different file directory… then it's still inconsistent in the end. But that's a problem we already have, and would require massive rearchitecting to fix/make clearer. We can't fix that here.

All in all, #340 captures it well:

Ideally files would be more standalone. but Drupal just doesn't seem to be there just yet. So it seems sensible to go with Berdir's suggestion for now, and implement something more akin to how file uploads work in the UI.

We all would like to have contextless/standalone files… but we can't, because that's not how it's been designed to work, and we can't/shouldn't block REST-based file uploads on resolving that architectural problem, because it will likely take years.

So, let's move forward with #331! Any reason why that's not marked as "needs review"? I'm curious what testbot has to say!

damiankloip’s picture

I didn't trigger the bot just yet as the FileUploadTest I wrote for the previous behaviour has not yet been converted to account for this now being a rest resource. I'll take a look at that. I'm not sure it makes sense to extend ResourceTestBase, it provides a lot of stuff we don't really want to use here I think?

Wim Leers’s picture

#2869387: Subclasses of ResourceTestBase for non-entity resources are required to add pointless code cleans up ResourceTestBase quite a bit. Once that's in, why wouldn't you want to base test coverage off of that test? The current test coverage is not yet testing 4xx responses (except for one 415, but with insufficient rigor). It's also not yet testing in multiple formats.

Very high-level, very rough review below:

  1. +++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
    @@ -524,6 +524,7 @@ public function testAutocreateValidation() {
    +      'uid' => 1,
    
    +++ b/core/modules/file/src/Entity/File.php
    @@ -234,6 +234,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setDefaultValueCallback('\Drupal\file\Entity\File::getCurrentUserId')
    
    @@ -274,4 +275,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +  /**
    +   * Default value callback for 'uid' base field definition.
    +   *
    +   * @see \Drupal\file\Entity\File::baseFieldDefinitions::baseFieldDefinitions().
    +   *
    +   * @return array
    +   *   An array of default values.
    +   */
    +  public static function getCurrentUserId() {
    +    return array(\Drupal::currentUser()->id());
    +  }
    

    Should we move this into a separate issue? This looks like a simple enough change that can go in independently?

  2. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,227 @@
    + *     "canonical" = "/file/upload/{entity_type_id}/{bundle}/{field_name}",
    

    Does it even make sense to have a canonical link relation for this? This REST resource plugin only provides a post() method after all…

  3. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,227 @@
    +  public function calculateDependencies() {
    +    return [
    +      'module' => ['file']
    +    ];
    +  }
    

    This should not be necessary, because this plugin is provided by this module, hence this would be calculated automatically?

  4. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,227 @@
    +    $field_definitions = $this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle);
    +
    +    if (!isset($field_definitions[$field_name])) {
    +      throw new BadRequestHttpException(sprintf('Field "%s" does not exist', $field_name));
    +    }
    +
    +    // @todo check the definition is a file field.
    +    $field_definition = $field_definitions[$field_name];
    +
    +    // Check access.
    +    if (!$field_definition->access('create')) {
    +      throw new AccessDeniedException(sprintf('Access denied for field "%s"', $field_name));
    +    }
    

    I think all this would benefit from being moved into a helper method. All of this is related to access checking.

  5. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,227 @@
    +  protected function validateOctetStream(Request $request) {
    

    Nit: The name suggests this is validating far more than just the Content-Type request header.

    Also: can be static.

  6. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,227 @@
    +   * Determines the URI for a file field.
    ...
    +  protected function getUploadLocation(array $settings) {
    

    Nit: mismatch.

    Also: can be static.

damiankloip’s picture

Here is a bit more work on the actual resource validation. I'll wait on the test conversion to see which is the best class to extend from. I have copied the logic from FileItem::getUploadValidators, and maybe a couple of things from file_save_upload. I think maybe we should extract some of that logic into separate functions that are used internally by file_save_upload.

damiankloip’s picture

I didn't know about that issue, if it makes less assumptions about all the things a resource should test, then I have no issues using that. However, it looks like it doesn't really change too much? I'm not sure what benefit we will get from testing things like different formats, they should all fail? unless we use the accept header to determine the format of the response? Would be nice if it could test 403 responses etc.. for us. Seems like a lot of test duplication though. If it's a rest resource don't we already know that it will be protected by a permission?

garphy’s picture

We all would like to have contextless/standalone files… but we can't, because that's not how it's been designed to work, and we can't/shouldn't block REST-based file uploads on resolving that architectural problem, because it will likely take years.

The issue may be that exposing an upload feature as a REST resource of file entities feels like it provides contextless upload.

We could also postpone this until some bits from the media initiative lands in code (specifically the media entity) and provide upload a this level, as the media entity will definitively expose standalone media (including local files) management.

It's just a thought but it may be more relevant than shipping an upload feature bound to file/image field context when we can anticipate that those may be deprecated in favor of "plain" entity reference field targeting media entities.

Or we keep going with the current approach and we'll add a contextless upload alongside media entity later, but we'll have to support both methods for a while then.

Wim Leers’s picture

We could also postpone this until some bits from the media initiative lands in code (specifically the media entity) and provide upload a this level, as the media entity will definitively expose standalone media (including local files) management.

We should not wait for Media.

Berdir’s picture

We could also postpone this until some bits from the media initiative lands
in code (specifically the media entity) and provide upload a this level, as
the media entity will definitively expose standalone media (including local
files) management.

No, it actually doesn't.

An image media bundle is 100% identical to a node type with an image field. It's an entity with an image field. The field controls where the file is saved, validation settings and so on.

So with this approach, what you will need to do to create an article with a media element is:
1. Upload the file in context of media bundle image field.
2. Create the media entity referencing the file.
3. Create the node that references the media entity.

Yes, complicated, but it will also just work. As I already suggested earlier, what we could do is abstract part of that complexity and allow to merge step 1 and 2 together, so that you can directly create a media entity with binary data (you still need to provide the bundle, as you can have multiple image media bundles with different settings). But that's optional and there are quite a few things that still need to be answered there:
* Media entities can have all kinds of metadata, some of that could even be required, so how would you submit that?
* media entities could support multiple images in the same bundle (could be a gallery, or could be different versions of the same file or so)

So it would not replace this, it would just build upon it and it would probably not work for all sites/use cases. This however should.

Wim Leers’s picture

I'm not sure what benefit we will get from testing things like different formats, they should all fail?

Because in case of file uploads, we just have binary data in the request body, not hal_json or json? Does that mean this is the first request body format-agnostic REST resource in core?

unless we use the accept header to determine the format of the response?

Right, we do normalize the created File entity in the response, which means it's not actually format-agnostic. The request body is, the response body is not.
The request body format is determined by the Content-Type request header, the response body format is determined by the ?_format URL query argument. So I'm not sure I understand what you're getting at?

If it's a rest resource don't we already know that it will be protected by a permission?

No, only REST resource plugins that do not override the default \Drupal\rest\Plugin\ResourceBase::permissions() implementation are guaranteed to have a permission. REST resource plugins that override this can use whichever access control mechanism they like.
For example, \Drupal\rest\Plugin\rest\resource\EntityResource::permissions() overrides it so that it can choose to rely on Entity Access instead. The REST resource being introduced here should do so too.

damiankloip’s picture

OK, here is a new patch with mostly changes from comments in #343:

  1. Reverted that. Should we also revert the change in the access control handler too?
  2. I used the canonical resource in the annotation too so it shows in a couple of places (and rest UI) - it seems canonical URI is used in these kinds of contexts?
  3. Removed
  4. Moved into a validateAndLoadFieldDefinition() method
  5. Renamed to validateOctetStreamContentType()
  6. This method has the token dependency properly now, so probably best to leave it as non-static IMO. I'm not sure I'm a fan of making methods static on a class that is usually an instance. You want to signify it doesn't really have any dependencies?

So, regarding the request and response content type; I guess it makes sense to allow a ?_format to be used, it's just kind of weird as the Content-Type would still have to be application/octet-stream but this also makes sense. Ideally we would just have an Accept header but ... :) So guess it's a strange one, for the actual request we don't really care about any serialization formats, but response we do.

We still need to look at the tests, to convert my initial test coverage to some kind of REST based test. @WimLeers, can you give me some pointers here. I'm not sure if it's best to extend ResourceTestBase or not? We need to set up a file field on a test entity, users with perms to create a parent entity. The requests need to use the method I already have to do a proper request to send the data, but this is already pretty close to what's in ResourceTestBase, so I don't think this should be a problem.

Wim Leers’s picture

#350:

  1. Eh… don't you need those changes for this patch to work? Or perhaps it was a remnant from an earlier iteration?

    Similarly, if this patch can do without the changes made in FileAccessControlHandler, then that'd be splendid, yes. But I suspect we'll eventually need some changes.

  2. The REST UI module has some significant flaws/bugs that it needs to fix; we shouldn't provide a link relation type URI path if we don't use it! What other places is this relevant for?
  3. Yay!
  4. Yay!
  5. Yay!
  6. Ah, yes, then leave it. And yes, that's what I want to signify. See http://drupal4hu.com/node/416.html.

I don't see what's weird about sending a request with the request body in one format and the response body being in another format, a format that you can specify? Yes, it's uncommon, because 99% of the time, the format of the request body and response body match. But in this case, that wouldn't make sense, would it? Getting a application/octet-stream response body wouldn't actually be able to convey information in a very useful way :D
So this is a place where it's natural for the formats of the request body and response body to differ.
Hopefully you're nodding along while reading this — if not, please elaborate!

On the subject of tests:

  1. Yes, I do think it's desirable to extend ResourceTestBase, because that'll make it easy to create tests in json and hal_json formats and for different authentication providers.
  2. Concretely: implement FileUploadResourceTestBase extends ResourceTestBase, then implement FileUploadJsonAnonTest, FileUploadJsonBasicAuthTest, FileUploadJsonCookieTest and FileUploadHalJsonAnonTest, FileUploadHalJsonBasicAuthTest, FileUploadHalJsonCookieTest. With all of those 6 extending FileUploadResourceTestBase.
  3. Those 6 classes can then each be very thin: see RoleJsonAnonTest + RoleJsonBasicAuthTest + RoleJsonCookieTest as an example.
  4. In a secondary phase (once the above is done, and you have the existing basic test coverage ported to ResourceTestBase and working for all the formats + authentication provider combinations), probably look at EntityResourceTestBase::testPost() to see what kinds of edge cases you want to test. I'm happy to do that for you when we reach that point.
slashrsm’s picture

I agree with @Berdir on that. As Media entity is adopted in core files will become even more hidden part of the system. When the switch is made to fully use media entity files won't appear outside of its scope any more. That said, I think that we need to treat them as such.

As I already suggested earlier, what we could do is abstract part of that complexity and allow to merge step 1 and 2 together, so that you can directly create a media entity with binary data (you still need to provide the bundle, as you can have multiple image media bundles with different settings).

This would make a lot of sense IMO.

damiankloip’s picture

RE #351:

1. So that was needed before as the file entity was created more inline. Now we are controlling things as we have a resource that handles the creation of the file and its data only. So as the patch now does, we just pass the uid from the current user that is injected into the resource.
2. I can't remember where else I thought this might be a problem now, I'll just remove the canonical and we'll see how things go.

Yes, it's only weird really because we use _format as a replacement for content-type and accept depending on the case. Otherwise, it would totally not be weird. So this is just Drupal... otherwise, yes, I'm nodding along :)

Working on the tests now. I'll see how far I get.

dawehner’s picture

  1. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,341 @@
    +      'status' => 0,
    

    INHO we should add an explanation why we use status: 0

  2. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,341 @@
    +    return new ModifiedResourceResponse($file, 201);
    

    Can we add the file ID as location header, just like we do in \Drupal\rest\Plugin\rest\resource\EntityResource::post

  3. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,341 @@
    +      // Move the file to the correct location based on the file entity,
    +      // replacing any existing file.
    +      if (!file_unmanaged_move($temp_file_name, $destination_uri, FILE_EXISTS_REPLACE)) {
    

    Does that mean some user can override the file of some other user? I guess this is not the case, because tempnam will generate a unique filename anyway? Do we need FILE_EXISTS_REPLACE then?

  4. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,341 @@
    +    // Check access.
    +    if (!$field_definition->access('create')) {
    +      throw new AccessDeniedException(sprintf('Access denied for field "%s"', $field_name));
    +    }
    

    Mh, are we sure we don't want to throw a 404 exception? This exception is not implementing \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface

  5. +++ b/core/modules/file/src/Plugin/rest/resource/FileUploadResource.php
    @@ -0,0 +1,341 @@
    +    if (!empty($errors)) {
    +      $message = "Unprocessable Entity: file validation failed.\n";
    +      $message .= implode("\n", $errors);
    +
    +      throw new UnprocessableEntityHttpException($message);
    +    }
    

    We should add a todo for #1916302: RFC 7807: "Problem Details for HTTP APIs" — serve REST error responses as application/problem+json, so we actually produce machine readable errors at some point.

  6. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -55,16 +44,4 @@ public function normalize($entity, $format = NULL, array $context = []) {
     
    -  /**
    -   * {@inheritdoc}
    -   */
    -  public function denormalize($data, $class, $format = NULL, array $context = []) {
    -    $file_data = (string) $this->httpClient->get($data['uri'][0]['value'])->getBody();
    -
    -    $path = 'temporary://' . drupal_basename($data['uri'][0]['value']);
    -    $data['uri'] = file_unmanaged_save_data($file_data, $path);
    -
    -    return $this->entityManager->getStorage('file')->create($data);
    -  }
    -
    

    Isn't that a BC break when we remove this functionality?

damiankloip’s picture

Here is a WIP start on the test coverage. Aside from needing to be tidied up and implemented properly (inc some more edge cases etc..) the first main blocker (after a little debugging) is that ContentTypeHeaderMatcher matches the '_content_type_format' requirement that is added to all REST routes, so our route is then blocked out I think. So we actually get a 415 back in that case. I am also still getting a 403 for the first case, so maybe I am doing something wrong with setting up the test user possibly..

damiankloip’s picture

Daniel!

1. We could just remove this, the default behaviour of a File is to start with status as FALSE.
2. Yeah! added. do we want to use url() like I have done for files, or should we use the same code (pretty much) that EntityResource has. I guess the question is do we like to the file entity or the file itself? I don't think file entities implement the templates needed for that to work.
3. Yeah, we wanted this before when it was only overwriting/updating but don't want it now!
4. Changed to AccessDeniedHttpException - it was meant to be that anyway - good catch
5. What should the @todo say? I'm not sure.
6. Meh. Well, that functionality doesn't really work anyway, it only saves files in temp. I would be very surprised if this was being used, and I think we should definitely not support it either. It has some of the same failings that is a big reason we cannot just have a generic upload endpoint.

Made some of those changes now quickly to keep things up to date with reviews.

Wim Leers’s picture

#353:

  1. Ok!
  2. Great :)

yes, I'm nodding along :)

:)


#354.2++ — see \Drupal\rest\Plugin\rest\resource\EntityResource::post() for code to copy/paste :)
#354.5: agreed, but seems kinda out of scope here; this is a pre-existing problem. It's okay to be explicit, but at the same time it's not guaranteed this will end up being the solution (it is likely though).


#355:

.) the first main blocker (after a little debugging) is that ContentTypeHeaderMatcher matches the '_content_type_format' requirement that is added to all REST routes, so our route is then blocked out I think. So we actually get a 415 back in that case.

Oh that's interesting! I think it's fair to say that file uploads are the very very very rare exception to the rule. Indeed, file uploads don't want the json, hal_json or whatever formats. You want the application/octet-stream MIME type… for which no format has been defined yet! \Symfony\Component\HttpFoundation\Request::initializeFormats() also doesn't define it.
So, we'll want the File module (or perhaps core) to define a format for it (name TBD, but probably something like 'binary' => ['application/octet-stream']?). Then we have two options:

  1. Either we want a RoutingEvents::ALTER subscriber whose priority is lower than \Drupal\rest\Routing\ResourceRoutes, so that it runs later, and can set $route->setRequirement('_content_type_format', 'binary').
  2. Or we want to update \Drupal\rest\Routing\ResourceRoutes::getRoutesForResourceConfig() so that it doesn't set a _content_type_format requirement if and only if such a route requirement already exists. This then allows us to implement FileUploadResource::getBaseRouteRequirements(), so that we can specify '_content_type_format' => 'binary' there.

I like option 2. Because it puts the control with the @RestResource plugin, and therefore allows self-contained implementations, rather than requiring additional event subscribers to be set up.

damiankloip’s picture

Yes, I totally prefer options 2 also :)

damiankloip’s picture

OK, a few more fixes and a few more problems :):

- \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::getResponseFormat() uses the '_content_type_format' for acceptable requirements only, for responses. This doesn't quite fit our use case here. I have added our service provider and other changes to the rest route building to do what was suggested in #357. However, when we get to the response, we have issues. We can't the use the format (or default to JSON?) but the only acceptable type is 'bin', as that is specified in the '_content_type_format'. Not sure of the best fix yet, but the assumptions in this code on't seem 100% correct, shouldn't we be checking either the _format or the Accept header or something? Otherwise we can never accommodate the valid case we mentioned above for this; POSTing in binary but receiving the serialized file entity in JSON etc.. in the response. Thoughts on this?
- In order to not have the request content normalized for us, we need a new controller method to handle the request, so I made a new handleRaw method for now. This is just c/p from handle() but with the normalization removed. So if we like this idea, we can refactor/tidy it up a bit. Otherwise, we will need a way for the plugin to tell the controller method that it does not need to be serialized. Could be something on the annotation or something.
- My test is failing on the field 'create' access check, not sure why yet. Need to look into this. Probably insufficient perms granted for the test user...

On the plus side, we can remove the custom validation for the application/octet-stream content type as the '_content_type_format' access check can cover that for us now.

damiankloip’s picture

+++ b/core/modules/file/src/FileServiceProvider.php
@@ -0,0 +1,24 @@
+<?php
+
+namespace Drupal\file;
+
+
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\DependencyInjection\ServiceModifierInterface;
+use Drupal\Core\StackMiddleware\NegotiationMiddleware;
+
+/**
+ * Adds 'application/octet-stream' as a known (bin) format.
+ */
+class FileServiceProvider implements ServiceModifierInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function alter(ContainerBuilder $container) {
+    if ($container->has('http_middleware.negotiation') && is_a($container->getDefinition('http_middleware.negotiation')->getClass(), NegotiationMiddleware::class, TRUE)) {
+      $container->getDefinition('http_middleware.negotiation')->addMethodCall('registerFormat', ['bin', ['application/octet-stream']]);
+    }
+  }
+
+}

Sorry, somehow I added this file to the commit for my previous patch and not in my latest interdiff! This also got added.

Wim Leers’s picture

However, when we get to the response, we have issues. We can't the use the format (or default to JSON?) but the only acceptable type is 'bin', as that is specified in the '_content_type_format'. Not sure of the best fix yet, but the assumptions in this code on't seem 100% correct, shouldn't we be checking either the _format or the Accept header or something? Otherwise we can never accommodate the valid case we mentioned above for this; POSTing in binary but receiving the serialized file entity in JSON etc.. in the response. Thoughts on this?

That's because your POSTing to /file/upload/entity_test/entity_test/field_rest_file_test and not to <code>/file/upload/entity_test/entity_test/field_rest_file_test?_format=json — i.e. you're not yet specifying the acceptable response format! :)
Fixing that causes the response to not be HTML, but json:

-    $uri = Url::fromUri('base:' . static::$postUri);
+    $uri = Url::fromUri('base:' . static::$postUri, ['query' => ['_format' => static::$format]]);
Otherwise, we will need a way for the plugin to tell the controller method that it does not need to be serialized. Could be something on the annotation or something.

What about checking if the format is bin? So:

  $format = $request->getContentType();

+ if ($format === 'bin) {
+   // don't decode nor denormalize…
+ }
+ else {
+   // current decode + denormalize logic…
+ }
On the plus side, we can remove the custom validation for the application/octet-stream content type as the '_content_type_format' access check can cover that for us now.

:)

damiankloip’s picture

Right, But I tried it adding format to the query too, and it seems the same problem happens (forget about access for now): Serialization for the format bin is not supported ...

Which I think is being enforced due to this in \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::getResponseFormat:

$acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
    $acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];
    $acceptable_formats = $request->isMethodCacheable() ? $acceptable_request_formats : $acceptable_content_type_formats;

So the acceptable formats will use the content type formats, as it is a POST method, but, REST resource plugins only add a _format requirement on GET and HEAD routes, so the acceptable request formats would not be populated for our route yet anyway.

I'm not sure about checking the 'bin' format explicitly in the ResourceHandler itself, then we kind of couple a format defined in the file module to the REST module?

I'm not sure what I'm doing wrong with the access check, I thought if the permission to create the entity was correct it should be ok, but maybe the access checking is not correct?

Wim Leers’s picture

So the acceptable formats will use the content type formats, as it is a POST method, but, REST resource plugins only add a _format requirement on GET and HEAD routes, so the acceptable request formats would not be populated for our route yet anyway.

D'oh!

So the problem is this:

    $acceptable_formats = $request->isMethodCacheable() ? $acceptable_request_formats : $acceptable_content_type_formats;
…
    if (in_array($requested_format, $acceptable_formats)) {
      return $requested_format;
    }

Because $acceptable_request_formats === ['json'] and $acceptable_content_type_formats === ['bin'], but due to the logic, $acceptable_formats is calculated to be ['bin'].

So my answer is: is this a bug in \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::getResponseFormat()? Remember, that was written when we had no use cases yet for the requested response format not matching the request body format!

I think this change would make sense:

diff --git a/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
index df77281..58a863e 100644
--- a/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
+++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -96,7 +96,7 @@ public function getResponseFormat(RouteMatchInterface $route_match, Request $req
     $route = $route_match->getRouteObject();
     $acceptable_request_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];
     $acceptable_content_type_formats = $route->hasRequirement('_content_type_format') ? explode('|', $route->getRequirement('_content_type_format')) : [];
-    $acceptable_formats = $request->isMethodCacheable() ? $acceptable_request_formats : $acceptable_content_type_formats;
+    $fallback_acceptable_formats = $request->isMethodCacheable() ? $acceptable_request_formats : $acceptable_content_type_formats;
 
     $requested_format = $request->getRequestFormat();
     $content_type_format = $request->getContentType();
@@ -104,7 +104,7 @@ public function getResponseFormat(RouteMatchInterface $route_match, Request $req
     // If an acceptable format is requested, then use that. Otherwise, including
     // and particularly when the client forgot to specify a format, then use
     // heuristics to select the format that is most likely expected.
-    if (in_array($requested_format, $acceptable_formats)) {
+    if (in_array($requested_format, $acceptable_request_formats)) {
       return $requested_format;
     }
     // If a request body is present, then use the format corresponding to the
@@ -114,8 +114,8 @@ public function getResponseFormat(RouteMatchInterface $route_match, Request $req
       return $content_type_format;
     }
     // Otherwise, use the first acceptable format.
-    elseif (!empty($acceptable_formats)) {
-      return $acceptable_formats[0];
+    elseif (!empty($fallback_acceptable_formats)) {
+      return $fallback_acceptable_formats[0];
     }
     // Sometimes, there are no acceptable formats, e.g. DELETE routes.
     else {
damiankloip’s picture

Status: Needs work » Needs review
FileSize
33.85 KB
3.7 KB

Hmm, I have made that change (looks sensible) but then we need to add the _format requirement for the resource, then I get 404's somewhere. So I guess _format is being checked/enforced somewhere else earlier in the routing system?

Status: Needs review » Needs work

The last submitted patch, 364: 1927648-364.patch, failed testing.

Pages