Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization)

This reminds me of my idea somewhere to automatically expose the entity label of a reference field in a REST response.

Of course, that again reminds my of my recent argument that adding computed properties to field types isn't free.. it results in always instantiating a class (a plugin, going through typed data manager/factory and so on) currently, even when it is not accessed.

Ok I am kind of confused with the issue summary.

We could add a 'url' property to \Drupal\file\Plugin\Field\FieldType\FileItem::propertyDefinitions

But I am pretty sure this would be overwritten in normalization because in \Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer::normalize

if ($url = $entity->url('canonical')) {
        $values['url'] = $url;
      }

So this would overwrite any 'url' property, correct?

Is 'url' what this issue was meant for adding anyways?
Here is the output I get already:

"field_file": [
{
"target_id": 1,
"display": true,
"description": "",
"target_type": "file",
"target_uuid": "93eba630-9208-4219-91a6-bc1895087c98",
"url": "http://drupal.test/d8/sites/default/files/2018-02/interdiff-5-6.txt"
}
],
"field_private_file": [
{
"target_id": 2,
"display": true,
"description": "",
"target_type": "file",
"target_uuid": "cd4db23d-2c82-4a1d-9566-68ae45690368",
"url": "http://drupal.test/d8/system/files/2018-02/interdiff-61-64.txt"
}
]

So do we still need this issue?

Hm, interesting problem.

The thing is that url is using the deprecated url() method. And only that url method returns the return to the physical file as it does not actually have a canonical link template. So if we'd at the same time convert it to use toUrl() if there is a canonical link template, then it would continue to work as-is for files and other entity types and we would not result a change while getting rid of the deprecated all and custom override of file entity.

The only problem is when combining with file_entity which does add a canonical link template, then we'd end up with overwriting it again. What if we expose this as file_url? Means we can't remove the deprecated call for file, but we could officially deprecate it, possibly with a hidden setting like other things we did?

What if we expose this as file_url? Means we can't remove the deprecated call for file, but we could officially deprecate it, possibly with a hidden setting like other things we did?

So if we did this the REST response for FileItems would have

1. 'url' which is from \Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer::normalize()
2. 'file_url' which is from the new computed field added in \Drupal\file\Plugin\Field\FieldType\FileItem::propertyDefinitions()

So without File Entity installed these would both be the same value? It seems like that would be confusing to consumers. How would they they know which one to use?

'url' for file entities would only be exposed anymore if serialization.settings:bc_expose_file_item_url (or something like that) would be set, which like other settings, would only be set to TRUE by default on existing installations.

Can't you do a pre-emptive release of file_entity for 8.5.x that detects the Drupal core version and if so, behaves differently on 8.6, to account for this change landing in Drupal 8.6? Because the file_entity module is filling a gap in Drupal core. Once core no longer has that gap, the need for the file_entity module disappears (I know the module does much more than only providing a canonical route though.)

Behave differently how? file_entity really *has* a canonical link template, at /file/ID, which is where you can view the file entity just like you can view a node at /node/ID, with the fields.

If we go with file_url then this becomes a non-issue.

Also, I think file_entity should really be officially marked as deprecated now in favor of media, but it will not go away any time soon.

I think as part of this issue, we should remove \Drupal\file\Entity\File::url() (i.e. revert #2277705: Files don't have URI nor href). But of course, there are BC implications to that :(

As (I think?) I already wrote, the thing is that url() already *is* deprecated. We don't need to remove it now, we can just let it stay a behavior specific to url(), which will be removed in in 9.x, and mark #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation as a won't fix.

The harder part is indeed getting rid of the call in the normalizer. That's what I meant with:

> Means we can't remove the deprecated call for file, but we could officially deprecate it, possibly with a hidden setting like other things we did?

By setting, I mean we would add another bc setting to serializer that would skip that call, similar to hal.settings:bc_file_uri_as_url_normalizer (that's why I would have liked to put both behind the same setting, as they are closely related, now we need two different settings).

So as usual with those things, existing sites continue to export url and file_url as the same thing. New sites only expose file_url. If you install file_entity, you also get the url as a real canonical link.

> Do you mean it'd only skip that call for the File entity type?

I think we'd simply make it trigger using toUrl()->toString() instead of url(), which would then automatically no longer call the special url() method on file entity. So no, it would not be specific, we need to get rid of that call for all entity types.

Not sure if we want to name it specifically about file entity or something more generic.

Re #19, yes basically that, but ComputedFileUri is specifically for the file uri base field and gets it from the field parent, while here we want to get it from the referenced entity in the file, small but important difference (->entity vs. getEntity(), yes I know how confusing this is). So I had to add a second class.

Working on the test fails, we've made a bit of a mess.

So in #3009854: Fix "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility." deprecation error, I pointed out that there is no file url left after that change. That is correct, but it's only correct for hal_json I believe, "regular json" has a fallback in \Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer::normalize(), but that doesn't apply to hal_json. Yes, I totally get the irony that this is the point that Wim always makes and I'm reluctant about (adding everything on the typed data level instead of normalization because then it works for all formats).

The relevant code was changed in #2922487: Follow-up for #2910211: fix all deprecation warnings but it was kept like that as it was before for BC, and it has a @todo pointing here, except that we'd like to add here here with a different property. I guess we'll have to name it url then here to continue that BC :-/.

Also interesting that ComputedFileUri explicitly makes it root-relative whlie the existing code there is absolute, so to keep BC there too, we need to make ComputedFileItemUri and ComputedFileUri inconsistent :-/.

So I tried to make that BC by renaming to just url and adding it as an absolute URL.

Also, something about the Hal tests is weird... I think extra properties in entity reference fields are still broken there. Fact is that the Hal tests weren't failing unlike Json and XML based tests and also now, no changes seem to be necessary, when they actually should fail as we are supposed to be adding extra stuff to the output. Double-irony or something, extra computed properties only work if the normalization layer actually cares about that... :)

Getting that jsonapi test working was pretty icky, the hardcoded file ID's for example are pretty tricky to get right. Also hardcoded ids in general is a bad idea, because it will fail on environments that skip autoincrement values. Would be better to load the files based on a unique filename instead. But I went with the existing logic in the test.

Also the flow is sometimes quite hard to follow, which test fail is getting its values from where, that's the downside of those generic tests.

Also, seems like it would be useful to test with actual values for display/description in that file field.

#3066751: Add resolvable_uri property to LinkItem for APIs is doing the same thing for the link field. @Berdir do you think #3066751-8: Add resolvable_uri property to LinkItem for APIs is an explorable option?

Some new things that I found while looking at this again and also things to check/discuss based on the new direction of #3009854: Fix "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility." deprecation error.

That logic there will automatically be applied to hal_json normalization as well, which is good. But, regular json doesn't have that reusable logic and has hardcoded/duplicated it, with another file fallback as mentioned earlier. I would assume it would only make sense to apply that behavior from hal_json also to json, possibly even as the same setting, if we move the BC setting up into serialzation.

If we do that, then we basically also lose the file URL in json, just like we do now in hal_json. But that might actually be a good thing because we do then not have to respect the existing behavior and can instead make the file url on file/image item an the file entity consistent: file_url property, root-relative URL.

Also, something about the Hal tests is weird... I think extra properties in entity reference fields are still broken there. Fact is that the Hal tests weren't failing unlike Json and XML based tests and also now, no changes seem to be necessary, when they actually should fail as we are supposed to be adding extra stuff to the output. Double-irony or something, extra computed properties only work if the normalization layer actually cares about that... :)

Just manually confirmed that, steps to reproduce:
1. Install standard
2. Enable rest, hal, basic_auth (to get the optional default config imported). Optionally allow cookie in rest.resource.entity.node
3. Create an article with image and alt text.
4. Visit node/1?_format=hal_json

You don't see the alt text. And the file obviously also doesn't contain it. That is because \Drupal\hal\Normalizer\EntityReferenceItemNormalizer::normalize() specifically only does the embedded uuid + lang + self/link. But we also need to include the stuff from \Drupal\hal\Normalizer\FieldItemNormalizer::normalizedFieldValues() (again minus target_id) somehow. top-level, new nested key? I think in _embedded doesn't make sense.

So my regular question.. how is anyone using this for something real if you can't possible access alt text, file description, ...

If we can find a name for a setting we could possibly bring that all under one BC setting. Because we do need all of it to eventually be able to have consistency and direct access to file URL's on node/media entities. I'd vote for "serialization_file_insanity: true". I'll show myself out now...

We could actually add those extra attributes like any other field

Still needs some work, but this:

diff --git a/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
index 48dbb81b47..31f91e7bb6 100644
--- a/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
+++ b/core/modules/hal/src/Normalizer/EntityReferenceItemNormalizer.php
@@ -66,8 +66,12 @@ public function __construct(LinkManagerInterface $link_manager, EntityResolverIn
   public function normalize($field_item, $format = NULL, array $context = []) {
     // If this is not a fieldable entity, let the parent implementation handle
     // it, only fieldable entities are supported as embedded resources.
+    $normalized =parent::normalize($field_item, $format, $context);
     if (!$this->targetEntityIsFieldable($field_item)) {
-      return parent::normalize($field_item, $format, $context);
+      return $normalized;
+    }
+    else {
+      unset($normalized[$field_item->getParent()->getName()][0]['target_id']);
     }
 
     /** @var $field_item \Drupal\Core\Field\FieldItemInterface */
@@ -95,7 +99,7 @@ public function normalize($field_item, $format = NULL, array $context = []) {
     $field_name = $field_item->getParent()->getName();
     $entity = $field_item->getEntity();
     $field_uri = $this->linkManager->getRelationUri($entity->getEntityTypeId(), $entity->bundle(), $field_name, $context);
-    return [
+    return $normalized + [
       '_links' => [
         $field_uri => [$link],
       ],

Results in this:

Needs some more cleanup at least to get rid of empty entries with nothing but the langcode (the url is coming from the patch here)

> #31: I don't think dealing with those other properties in this issue is in scope of this issue. But if it makes things easier, then … sure!

I wish. file_uri is a property too. So either we deal with it generically, or we special case file_uri now. Either way, we need to do *something*.

#24: Yeah, that comment is the result of hours of debugging I think, my brain might not have been fully sane anymore at that point. Take your time with that comment ;)

See updates on #3009854: Fix "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility." deprecation error and #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL, I think this is 9.1 material at this time, so we can take our time with it. Or if we decide by then to remove hal.module from core, which might not be the worst idea. As mentioned before, my only use case is default_content.module, and as discussed before, it might more sense for that to just do its own format that just has the raw content, we don't need any fancy normalization, $entity->toArray(), a bit of metadata and converting that to json or yaml should mostly do the trick :) The main thing to figure out is the dependency tree, we'll need to keep those referenced-entity uuid's something. That's the number #1 selling point of default_content compared alternatives.

Will try to find some time to get back to this after the two related issues are done.

One comment on this though:
> default_content does not need any URIs at all.

That isn't quite so simple. It doesn't use URI's anymore, but it very much requires content relationships. That's the most important functionality of default_content IMHO. It builds a dependency tree of all content that is to be imported, and makes sure that it creates terms referenced by nodes first, authors of nodes also first, comments on those nodes afterwards and so on.

That's why hal+json was chosen as a format, because it provides the necessary information for that. Initially it used URL's but that was changed to using the UUID's that are provided for each relationship.

+1 for this. It should be similar with "processed" computer property from rich text fields.

This module would benefit from such a new property, see #3446770: How to map image field?