Support deserialization of file entities in HAL

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.phpundefined
@@ -0,0 +1,44 @@
+    $file_data['uri'] = system_retrieve_file($data['uri']);
+

I think actually if you use system_retrieve_file($url, NULL, TRUE) it will do the file/entity saving for you.

This also assumes that all files would go in public directory, regardless of where they came from. Not sure how this would work for temporary or remote (YouTube or Amazon S3 files).

Regardless, this needs to account for a FALSE on entity save or HTTP failure.

Oh duh, I read entity_create() as entity_save().

I'm not sure how system_retrieve_file() would work for private files either.

#1: 1988426-01-file-entity-deserialize.patch queued for re-testing.

All entities are NG now. This is unblocked AFAICT. Any changes needed?

I think the normalization from the EntityNormalizer gives us what we need there? Made a couple of other changes.

Hmm, we will have to make sure that the class/interface exists in the check if we start adding normalizers for other core modules in hal.

So we could do this..

Alternatively, this normalizer could live in file module. But then it will have a soft dep on hal module which doesn't seem right at all.

1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
   @@ -0,0 +1,53 @@
   +   */
   +  protected $supportedInterfaceOrClass = 'Drupal\file\Entity\File';
   

   I guess we should rather go with the the FileInterface instead.

2. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/NormalizerBase.php
   @@ -48,7 +48,7 @@ public function supportsNormalization($data, $format = NULL) {
   +    if (in_array($format, $this->formats) && (class_exists($this->supportedInterfaceOrClass) || interface_exists($this->supportedInterfaceOrClass))) {
   

   Do we actually have to support classes at all?

I changed this supported interface to FileInterface, although implementing that interface will not guarantee that denormalizing code will work as you could spit out something totally different form the properties, it's pretty specific to the File class behaviour.

We should definitely support classes too. People should be able to target interfaces or specific classes that serialization works for.

#1892320: Add deserialize for JSON/AJAX moves stuff to the base class, so this patch is fine and in scope.

My stance is that Serializer should alter state as little as possible, so I think saving the file to the temp directory is the desired behavior.

I don't think that's what this patch does though. system_retrieve_file() called without a $destination saves to public://, not to temporary://. Should we add tests here for:
- deserializing without it being followed by a $entity->save()
- deserializing with it being followed by a $entity->save()
?

Also, what do we expect in terms of idempotence? If we post the same payload twice, seems like this patch would create two separate local copies of the file: is that what we want? In either case, seems like we should have a test for that as well.

Yep, seems this should save files to tmp only, if anything. Ideally serializer should never save anything :-)

It looks like we just have to pass 'temporary://' as 2nd param to system_retrieve_file(). I would reroll but not on my pc at the moment.

I think you're right Moshe. Here we go.

20: 1988426-20.patch queued for re-testing.

1. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
   @@ -0,0 +1,53 @@
   +    $file_data = $data;
   +    unset($file_data['_links']);
   +    $file_data['uri'] = system_retrieve_file($data['uri'][0]['value'], 'temporary://');
   +
   +    return $this->entityManager->getStorageController('file')->create($file_data);
   

   Maybe we should explain what is going on here.

2. +++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
   @@ -0,0 +1,53 @@
   +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
   ...
   +   * @var \Drupal\Core\Entity\EntityManager
   ...
   +  public function __construct(EntityManager $entity_manager) {
   

   ... we do have an interface now.

Ok, so I've added a test for this too.

There were are couple of issues with the current patch that certainly needed fixing, like 'Drupal\file\FileInterface' and not 'Drupal\file\Entity\FileInterface'. As well as adding a normalize() method to create a file url. Otherwise there is no way it can be consumed in denormalize by system_retrieve_file().

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
@@ -0,0 +1,64 @@
+    // Unset '_links' property, it's not needed for file entities.
+    unset($data['_links']);

Not needed is a bit odd, does that mean it is actually not expected to be there, or would it be just ignored?

True, let's just remove that, it will be ignored if it's not needed, like any other properties.

Why are we using the filename druplicon.txt. I think this is kind of confusing since it isn't a druplicon.

Come on! :) It is not that confusing, plus this same pattern is used in a few other places.

So here, it seems we are showing that you can normalize a file entity and get the same data when you denormalize.

Not just that, mainly that a temporary file is created.

However, I think the functionality that we really want to test is what happens when you provide a URI which doesn't already have a corresponding file entity.

Why does this matter? We shouldn't care about saving the actual file entity once it has been denormalized. I don't see any problems with already having this saved in that case? The uri provided by me in the test is the one we are using, as we need to generate a full uri like http://mytestsite.com/wherever/the/file/is anyway.

I can try to remove the saving. It will make it slightly harder to test and won't give us any benefits though :/ (at least I think not).

OK, me and Lin spoke about this yesterday. It is a good plan to add tests for just a uri, as it could be a perfectly valid case that people will only have the file uri but want to post this data and create a file entity.

Hah, I was going to suggest that we then need to make sure that File::preCreate() sets the default for mime time, but we already do that, nice :)

What's a bit inconsistent is that mimetime is set on preCreate and size on preSave but both are ensured to be set.

Yes, I was pleasantly surprised to find out I didn't really have to do very much here :)

Code and new tests look good to go.

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
@@ -0,0 +1,62 @@
+    $data['uri'] = system_retrieve_file($data['uri'][0]['value'], 'temporary://');

I wrote a longer comment but lost it. Short version, system_retrieve_file() doesn't look fit for purpose here - for example it dsm()s rather than logs errors. That might be the best function we have to use here, but at least could use a follow-up to make it a bit less rubbish?

+++ b/core/modules/hal/lib/Drupal/hal/Normalizer/FileEntityNormalizer.php
@@ -0,0 +1,62 @@
+   */
+  public function denormalize($data, $class, $format = NULL, array $context = array()) {
+    $data['uri'] = system_retrieve_file($data['uri'][0]['value'], 'temporary://');
+

Let's just rely Guzzle instead of using that function :)

See http://stackoverflow.com/questions/16939794/copy-remote-file-using-guzzle

Probably needs drupal_basename() call or something like that to set the target file name, but should be fairly easy other than that?

Or do we need to use some of our crazy wrappers to avoid problems with strange file system?

Alright, alright :)

This changes it to inject the Guzzle client instead, and use that. This doesn't do any exception handling, I would guess we just want them to be thrown? Then you would know you HTTP request went sour pretty quick.

37: 1988426-37.patch queued for re-testing.

Rerolled after changes to hal service injected went in.

Not 100% sure if need the unmanaged file API, but it doesn't hurt.

Thanks, that looks better. Committed/pushed to 8.x, thanks!