Problem/Motivation

This is a sister issue of #2517030: Add a URL formatter for the image field. That issue solved it for image entities exposed via Views REST export displays. This issue must solve it at the serialization/normalization level, so that all REST resources (as well as JSON API) benefit.

Proposed resolution

  1. Blocked on #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts
  2. Blocked on #2910211: Allow computed exposed properties in ComplexData to support cacheability.
  3. Blocked on #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler
  4. Fix normalization of \Drupal\file\Entity\File by adding a computed url property to a File's uri field. Then the value property will contain the stored value (public://lama.jpg), and url will contain the computed root-relative file URL (/sites/default/files/llama.jpg).

Remaining tasks

  1. BC layer.
  2. BC layer tests (blocked on #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler).

User interface changes

None.

API changes

  1. A new url computed property is added to the File entity's uri field.
  2. This causes all normalizations/formats (json, xml, hal_json, JSON API and GraphQL in contrib) to have that new url property to show up in their normalizations. But this is only a new key in the normalization, so by definition does not break BC. It will contain a root-relative file URL (/sites/default/files/llama.jpg).
  3. The HAL normalization of $file->uri on existing sites continues to behave in the strange way it does in HEAD: the value property continues to contain an absolute file URL (https://example.com/sites/default/files/llama.jpg), rather than the stored value (a file URI: public://llama.jpg).
  4. The HAL normalization of $file->uri on new sites behaves in the sensible way: it now contains the stored value (public://lama.jpg) rather than an absolute file URL.
  5. Existing sites can opt in to the new behavior by setting the bc_file_uri_as_url_normalizer key in hal.settings to false.

Data model changes

A new url computed property is added to the File entity's uri field.

CommentFileSizeAuthor
#186 2825487-186.patch23.37 KBWim Leers
#186 interdiff.txt857 bytesWim Leers
#185 2825487-185-interdiff.txt648 bytescburschka
#185 2825487-185.patch23.26 KBcburschka
#181 2825487-181-interdiff.txt581 bytescburschka
#181 2825487-181.patch23.25 KBcburschka
#178 2825487-178.patch23.39 KBWim Leers
#178 interdiff.txt4.26 KBWim Leers
#175 interdiff-2825487-175.txt1.57 KBdamiankloip
#175 2825487-175.patch22.99 KBdamiankloip
#174 2825487-174.patch22.98 KBWim Leers
#174 interdiff.txt1.05 KBWim Leers
#170 interdiff-2825487-170.txt2.12 KBdamiankloip
#170 2825487-170.patch23.1 KBdamiankloip
#165 interdiff-2825487-165.txt2.16 KBdamiankloip
#165 2825487-165.patch23.06 KBdamiankloip
#156 interdiff-2825487-156.txt7.03 KBdamiankloip
#156 2825487-156.patch24.04 KBdamiankloip
#152 2825487-152.patch23.26 KBWim Leers
#152 interdiff.txt2.76 KBWim Leers
#146 2825487-146.patch23.62 KBWim Leers
#146 interdiff.txt4.51 KBWim Leers
#145 2825487-145.patch19.31 KBWim Leers
#145 interdiff.txt761 bytesWim Leers
#144 2825487-144.patch18.67 KBWim Leers
#144 interdiff.txt1.01 KBWim Leers
#137 interdiff-2825487-137.txt783 bytesdamiankloip
#137 2825487-137.patch17.74 KBdamiankloip
#135 interdiff-2825487-135.txt4.07 KBdamiankloip
#135 2825487-135.patch17.73 KBdamiankloip
#134 2825487-133.patch18.51 KBWim Leers
#134 interdiff.txt2.05 KBWim Leers
#131 interdiff-2825487-131.txt8.65 KBdamiankloip
#131 2825487-131.patch16.1 KBdamiankloip
#130 interdiff-2825487-130.txt6.65 KBdamiankloip
#130 2825487-130.patch16.4 KBdamiankloip
#127 interdiff-2825487-127.txt745 bytesdamiankloip
#127 2825487-127.patch15.9 KBdamiankloip
#123 interdiff-2825487-123.txt1.22 KBdamiankloip
#123 2825487-123.patch15.17 KBdamiankloip
#121 interdiff-2825487-121.txt1.95 KBdamiankloip
#121 2825487-121.patch15.37 KBdamiankloip
#119 interdiff-2825487-119.txt7.92 KBdamiankloip
#119 2825487-119.patch15.3 KBdamiankloip
#117 interdiff-2825487-117.txt518 bytesdamiankloip
#117 2825487-117.patch9.97 KBdamiankloip
#113 2825487-113.patch9.54 KBWim Leers
#113 interdiff.txt1.16 KBWim Leers
#110 interdiff-2825487-110.txt5.26 KBdamiankloip
#110 2825487-110.patch8.41 KBdamiankloip
#106 interdiff-2825487-106.txt1.31 KBdamiankloip
#106 2825487-106.patch7.73 KBdamiankloip
#102 interdiff-2825487-102.txt2.08 KBdamiankloip
#102 2825487-102.patch7.75 KBdamiankloip
#99 interdiff-2825487-99.txt490 bytesdamiankloip
#99 2825487-99.patch6.77 KBdamiankloip
#97 interdiff-2825487-97.txt3.77 KBdamiankloip
#97 2825487-97.patch6.71 KBdamiankloip
#87 interdiff-2825487-87.txt551 bytesgarphy
#87 2825487-87.patch2.97 KBgarphy
#82 2825487-82.patch2.94 KBdamiankloip
#72 interdiff-2825487-72.txt1.68 KBdamiankloip
#72 2825487-72.patch4.38 KBdamiankloip
#71 2825487-71.patch2.7 KBdamiankloip
#61 2825487-61.patch5.49 KBtstoeckler
#61 2825487-57-61.txt4.03 KBtstoeckler
#58 interdiff_57_48.txt4.38 KBdpovshed
#57 2825487-57-uri_normalizer.patch5.32 KBdpovshed
#53 2825487-53-uri_normalizer.patch1.92 KBMunavijayalakshmi
#48 2825487-48-uri_normalizer.patch2.06 KBtedbow
#47 2825487-47.patch1.55 KBtedbow
#33 add_filenormalizer_the-2825487-33.patch3.5 KBgarphy
#29 add_filenormalizer_the-2825487-29.patch3.61 KBgarphy
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Berdir’s picture

image style url? What would the ImageItem know about that? The images style is a widget-level configuration.

To be able to return an image style, we would need to know which and have configuration for that.

garphy’s picture

Should the FileItemNormalizer belong to the file module or the serialization module ? I'm not kinda sold on making either of them depend on the other.

Berdir’s picture

It's not necessary for them to depend on it each, it can be optional, requires that the service is registered dynamcially, like file_entity is doing that already: http://cgit.drupalcode.org/file_entity/tree/src/FileEntityServiceProvide...

tstoeckler’s picture

@Berdir: For JSON API they expose all possible image style URLs, which admittedly is a bit verbose, but still very useful for frontends.

dawehner’s picture

We could add support to filter the image style URLs using an additional setting in $context. When the response is gzipped adding additional image style URLs shouldn't matter that much, as they look mostly the same.

Wim Leers’s picture

Issue tags: +Needs followup

image style url? What would the ImageItem know about that? The images style is a widget-level configuration.

You're right. I mean that this should return all image style URIs. Which I now realize is what #6 said :)

Also #7++ of course. But nothing in core is using context right now, so it's probably best to make the "filter to particular image styles" functionality a follow-up.


All excellent feedback, thank you so much, Berdir, garphy, tstoeckler & dawehner!

tstoeckler’s picture

Title: The FileItem normalizer should expose the public URL, ImageItem normalizer should expose the public image style URL » The FileItem normalizer should expose the public URL, ImageItem normalizer should expose all public image style URLs

;-)

Wim Leers’s picture

Thanks :)

dawehner’s picture

Also #7++ of course. But nothing in core is using context right now, so it's probably best to make the "filter to particular image styles" functionality a follow-up.

I agree

Wim Leers’s picture

Title: The FileItem normalizer should expose the public URL, ImageItem normalizer should expose all public image style URLs » The FileItem normalizer should expose the file URL
Component: image.module » file.module
Issue tags: -Needs followup

garphy pinged me in IRC that he'd love to work on this but would need some guidance to get started. This should help him get started :)

Let's first deal with only FileItem. Perhaps we can split off ImageItem to a separate issue altogether. Besides ImageItem extends FileItem, so this will already help ImageItem — it's just that ImageItem will not yet provide image style URLs yet then. Opened #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs for that. And opened #2825814: [PP-1] Allow URL query argument to limit the image style URLs returned by ImageItemNormalizer for #7/#11.


So, looking at how to do this for FileItem.

  1. this cannot extend FieldItemNormalizer or NormalizerBase because this is not something that needs to be denormalized: the file URL really is a computed field
  2. hence this needs an approach that only implements NormalizerInterface. An example of that can be found in the patch for #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata. That patch is actually the perfect example, because the processed property of a TextItem also happens to be computed :)
  3. This made me realize that there's a problem: there's not yet a computed property on FileItem to represent this! So, the first step needs to be the updating of \Drupal\file\Plugin\Field\FieldType\FileItem::propertyDefinitions(). It needs a computed property, much like \Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions(), which does this:
      public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
        $properties['value'] = DataDefinition::create('string')
          ->setLabel(t('Text'))
          ->setRequired(TRUE);
    
        $properties['format'] = DataDefinition::create('filter_format')
          ->setLabel(t('Text format'));
    
        $properties['processed'] = DataDefinition::create('string')
          ->setLabel(t('Processed text'))
          ->setDescription(t('The text with the text format applied.'))
          ->setComputed(TRUE)
          ->setClass('\Drupal\text\TextProcessed')
          ->setSetting('text source', 'value');
    
        return $properties;
      }
    

    Doing it correctly like this ensures that it will actually also work in JSON API and even in GraphQL.

  4. This property should probably be called file_url, and its computed value would be a root-relative file URL: it should use the file_url_transform_relative(file_create_url(…)) pattern. Just like #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send did for all of Drupal core.
  5. A complication is that actually FileItem extends EntityReferenceItem. So we should keep the existing \Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer behavior. But actually, that one already only supports normalization, so that's great.

So there's basically two steps:

  1. Add a file_url computed property to FileItem
  2. ImageItemNormalizer extends EntityReferenceFieldItemNormalizer, with something like this:
      public function normalize($field_item, $format = NULL, array $context = []) {
        $values = parent::normalize($field_item, $format, $context);
        $values['file_url'] = …
        return $values;
      }
    
garphy’s picture

Assigned: Unassigned » garphy
Wim Leers’s picture

Thanks for taking this on, garphy!

garphy’s picture

Assigned: garphy » Unassigned

I'm actually a bit puzzled about what we're trying to achieve.

I thought it was about getting the URL to the file when serializing an entity which has a file reference field.

Given a fresh install of latest 8.3.x, with rest enabled, if I try to request a node which have a file reference field, or retrieve a file entity directly, I get in either case a "url" property with the full URL to the file.

So what is the actual situation where we need the url and we don't have it ?

garphy’s picture

Assigned: Unassigned » garphy

I'm still eager to tackle this actually :)

Wim Leers’s picture

#15: Isn't what you're getting a public://foobar.png URL?

Berdir’s picture

No, you don't, you get the file URL. In core. with file_entity, it points to file/ID.

That's because of \Drupal\file\Entity\File::url(), which was added in #2277705: Files don't have URI nor href. But I dislike that it was done like that and it's inconsistent (we only override a deprecated method and it only works because rest.module hasn't been updated yet to call the non-deprecated one. We have issues to unify it, but we can only unify it to be consistently weird, compared to all other entity types). That's what #2402533: No link template "canonical" found for the "file" entity type is doing.

Wim Leers’s picture

That's not what I'm seeing:

HTTP GET http://d8/entity/file/1

{"fid":[{"value":"1"}],"uuid":[{"value":"ea8215ba-b4d6-4382-84c3-43af543edcf7"}],"langcode":[{"value":"en"}],"uid":[{"target_id":"1","target_type":"user","target_uuid":"a0ba4449-b309-47a6-90fc-8ef77e8c141a","url":"\/user\/1"}],"filename":[{"value":"apcu_status_report--apcu_enabled_default.png"}],"uri":[{"value":"public:\/\/2016-11\/apcu_status_report--apcu_enabled_default.png"}],"filemime":[{"value":"image\/png"}],"filesize":[{"value":"7672"}],"status":[{"value":"1"}],"created":[{"value":"1478778911"}],"changed":[{"value":"1478778914"}]}

i.e.:
"uri":[{"value":"public:\/\/2016-11\/apcu_status_report--apcu_enabled_default.png"}]

Berdir’s picture

That's the file itself, that has nothing to do with *FileItem* normalizer. that would be a File entity normalizer. FileItem currently falls back to the standard entity reference handling, which adds uuid and url.

garphy’s picture

So two points :
- should we rework the way the "url" property is currently added ;
- is there a need to work on a "FileNormalizer" (& ImageNormalizer) to add the full URL and/or URL of the derivatives.

Berdir’s picture

I don't think we could do that actually, since this is field type URI but it's actually not a valid URI, it is a stream wrapper. might look similar, but it is actually not one :) (different rules for spaces, for example). So not sure we can define a normalizer that applies.

Maybe we could add it on the entity level.

Wim Leers’s picture

Title: The FileItem normalizer should expose the file URL » Add FileNormalizer: the File entity should expose the file URL

UGH yes, I totally confused \Drupal\file\Entity\File and \Drupal\file\Plugin\Field\FieldType\FileItem :( Sorry!

So this already works fine for FileItem because of two things:

  1. \Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer::normalize() does this:
    
          // Add a 'url' value if there is a reference and a canonical URL. Hard
          // code 'canonical' here as config entities override the default $rel
          // parameter value to 'edit-form.
          if ($url = $entity->url('canonical')) {
            $values['url'] = $url;
          }
        }
    
  2. \Drupal\file\Entity\File::url() does this:
      public function url($rel = 'canonical', $options = array()) {
        return file_create_url($this->getFileUri());
      }
    

So, rescoping this issue.


#21:

- should we rework the way the "url" property is currently added ;

That's out of scope here, and we must fix the issues mentioned in #18 to fix that.

- is there a need to work on a "FileNormalizer" (& ImageNormalizer) to add the full URL and/or URL of the derivatives.

Yes, rescoped this issue to that. We need an entity-level normalizer for that.

Wim Leers’s picture

17:19:22 <berdir> WimLeers: we can't have difference normalizer classes for images and other file entities
17:19:37 <berdir> WimLeers: normalizers are class based and that is the same class/entity type
17:19:58 <berdir> WimLeers: which means that that doesn't really work as image stuff is in image.module, we can't have file depend on that
17:56:52 <WimLeers> berdir: why would "supported class === \Drupal\file\Entity\File" not work?
17:57:07 <berdir> WimLeers: because it is the same class for images?
17:57:20 <berdir> WimLeers: so when do you return image styles and when not and where does the code live?
18:01:28 <WimLeers> berdir: IMO: File entity == file URL only (not yet the case). FileItem == file URL only (already the case). ImageItem == file URL + image style URLs (not yet)
18:04:03 <berdir> WimLeers: ok, fine with me if that's enough
18:04:50 <berdir> WimLeers: so we'd have a FileNormalizer and an ImageItemNormalizer
18:12:47 <WimLeers> berdir: yep
garphy’s picture

Potential gotcha :

18:20 < berdir> one thing to consider is that this has the potential to break file_entity because we already define a normalizer there to deal with the file content/base64 stuff
Wim Leers’s picture

Thanks for posting that, garphy. Hopefully https://www.drupal.org/project/file_entity maintainers (which includes Berdir) can then help out here, to ensure that does not happen.

Berdir’s picture

I fear the only thing that those maintainers will be able to do is deal with it in file_entity.

There can only be one normalizer per thing. Not sure if file_entity.module will win or file.module, possibly file_entity (as it is later when sorted alphabetically I guess), then our functionality continues to work but the logic provided by core won't and we'll have to extend from the new class to keep that working and depend on 8.3 once it is out

Wim Leers’s picture

Note that the JSON API contrib module currently is also adding a work-around for this:

  1. A \Drupal\jsonapi\Plugin\FileDownloadUrl class
  2. Plus this:
    /**
     * Implements hook_entity_base_field_info().
     *
     * @todo This should probably live in core, but for now we will keep it as a
     * temporary solution. There are similar unresolved efforts already happening
     * there.
     *
     * @see https://www.drupal.org/node/2517030
     */
    function jsonapi_entity_base_field_info(\Drupal\Core\Entity\EntityTypeInterface $entity_type) {
      $fields = [];
      if ($entity_type->id() == 'file') {
        $fields['url'] = BaseFieldDefinition::create('uri')
          ->setLabel(t('Download URL'))
          ->setDescription(t('The download URL of the file.'))
          ->setComputed(TRUE)
          ->setQueryable(FALSE)
          ->setClass('\Drupal\jsonapi\Plugin\FileDownloadUrl')
          ->setDisplayOptions('view', array(
            'label' => 'above',
            'weight' => -5,
          ));
      }
      return $fields;
    }
    
garphy’s picture

Status: Active » Needs review
FileSize
3.61 KB

I like the way it's solved in jsonapi, by adding an additional computed field at entity-level so it does not require a specific Normalizer. What would justify to add a new Normalizer for file entity beside that ? If we keep it this way, there will be no impact on file_entity module and a minimal impact on jsonapi.

As a first attempt, I mainly backported the code from jsonapi but I used absolute URL rather than relative. Any thoughts on this point ?

The code as it is now converts the "uri" property into a URL but the File entity has a url() method, shouldn't we use that instead ?

Last point : to add the "url" additional computed field, the code use hook_entity_base_field_info(), which is understandable when done in a contrib. But as we're now in the module providing the entity type, I'm wondering if there's a more direct way of achieving this (through the ContentEntityType annotation, maybe ?)

I still have a huge amount of knowledge to ingest there...

NB: Not adding tests yet, I'll do when we're settled on the approach.

Status: Needs review » Needs work

The last submitted patch, 29: add_filenormalizer_the-2825487-29.patch, failed testing.

tedbow’s picture

Last point : to add the "url" additional computed field, the code use hook_entity_base_field_info(), which is understandable when done in a contrib.

I think you would update \Drupal\file\Entity\File::baseFieldDefinitions and add the field there. Since you are updating the module that provides the entity.
I think the update process takes care of updating fields. Though I know there is
drush entity-updates
So maybe there is something else you need to do.

UPDATE:
Looks like you have to manually install the field in a hook_update_N
Check out Write update functions for entity schema updates, automation removed
and you would also add it in \Drupal\file\Entity\File::baseFieldDefinitions

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.

garphy’s picture

Status: Needs work » Needs review
FileSize
3.5 KB

I moved the definition of the "url" field to \Drupal\file\Entity\File::baseFieldDefinitions.

As I'm not sure of what is the best way of testing this, I just added a bit of testing in \Drupal\file\Tests\DownloadTest to check that the value of $file->get("uri")->data equals what is returned by file_create_url().

I still need to address the hook_update_N() part.

garphy’s picture

Title: Add FileNormalizer: the File entity should expose the file URL » File entity should expose the file URL as a computed base field

Better title for the issue.
We could also use a summary update.

Status: Needs review » Needs work

The last submitted patch, 33: add_filenormalizer_the-2825487-33.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs tests

I think you would update \Drupal\file\Entity\File::baseFieldDefinitions and add the field there. Since you are updating the module that provides the entity.

+1

  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -247,6 +247,13 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +      ->setLabel(t('Download URL'))
    +      ->setDescription(t('The download URL of the file.'))
    ...
    +      ->setClass('\Drupal\file\Field\FileDownloadUrl');
    

    Perhaps "public URL" would be better than "download URL"?

    I'm NOT saying to reroll the patch and change this. I'm asking :)

  2. +++ b/core/modules/file/src/Field/FileDownloadUrl.php
    @@ -0,0 +1,69 @@
    +    foreach ($this->getEntity()->get('uri') as $uri_item) {
    

    What is actually causing this to work? How is this populated?

  3. +++ b/core/modules/file/src/Field/FileDownloadUrl.php
    @@ -0,0 +1,69 @@
    +      $url_item->setValue(file_create_url($uri));
    

    You forgot file_url_transform_relative().

  4. RE: how to test: see #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler. Feel free to mark that issue as a duplicate of this and then add the test coverage here. It has plenty of examples :)
Wim Leers’s picture

Berdir’s picture

I'm not really sure about this approach. Can't we do this as an operation/link or something like that?

Wim Leers’s picture

A link (as in link relation type as in the API added by #2113345: Define a mechanism for custom link relationships) could work.

A new operation (as in view, edit and now something like download) would not work AFAICT: how would ->access('some new operation') actually be able to add a URL to the normalization?

Berdir’s picture

I'm not sure what I meant with operation exactly but definitely not access :) That said, the download access operation actually exists.

Wim Leers’s picture

Indeed that operation already exists, which made me all the more confused :P I guess I'm glad you don't know what you meant yourself then :P

e0ipso’s picture

I think that a link would indeed be the more appropriate approach. The current implementation in the JSON API module is rather hacky (and leads to all the false expectations that computed / virtual fields do).

garphy’s picture

What would be the JSON output of a link-based approach ?
Is there any example of this approach I can study to rework the patch ?

tedbow’s picture

@garphy I am not sure there is an example.#2113345: Define a mechanism for custom link relationships allows creating custom relationship links which I assume we would add "download" as but this would not automatically pick this up for json output.

garphy’s picture

@garphy I am not sure there is an example.#2113345: Define a mechanism for custom link relationships allows creating custom relationship links which I assume we would add "download" as

I'll take a look.

but this would not automatically pick this up for json output

That's the original point of this issue : getting the URL of the file when querying the file entity over REST. So if we go down the "link route", we should figure out a way to put this in REST output.

garphy’s picture

@elipso regarding #42,

The current implementation in the JSON API module is rather hacky (and leads to all the false expectations that computed / virtual fields do)

What are those false expectations ?

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.55 KB

Ok here is patch that uses the #2113345: Define a mechanism for custom link relationships and adds a custom "download" relationship.

Had to override toUrl in \Drupal\file\Entity\File::toUrl() because \Drupal\Core\Entity\Entity::toUrl() expects the link template to correspond to route with the entity type id in it AFAIK.

Also not sure what to put in the link template in the entity annotation so for now:

*   links = {
 *     "download" = "/fake-pattern"
 *   }

This is usually the pattern of the route which for file download doesn't apply.

Also there is no rest entity test for the file resource. I can work on that if this is the correct direction.

tedbow’s picture

I think a big problem with the approach in #47 is that get the download URL the client would have to look in the header vs the body of the response. Is this intuitive? If you were building client would you think to look there.

I think this especially confusing because if you are dealing with individual file fields you would see the URL in the body of the response but if you were dealing with the file entity itself you would have to know to look in the header.

I am not making clients but just thought I would point that out if others think that would be confusing.

Also every entity that used a uri item would have to do that same thing as File entity.

Another option would be handle it on the URLItem level.

Here is a patch that does this. Basically it it adds 'url' if creating the URL produces a different value than uri value. I add to add this check because \Drupal\aggregator\Entity\Feed::baseFieldDefinitions uses uri field but this already stores the direct http url.

So for any uri field item that doesn't store the http url but uses a scheme like "public" or "private" where the uri value needs to be converted into a URL this would add 'url'.

Wim Leers’s picture

Assigned: garphy » Berdir

I asked Berdir to review this. I think he's the person best able to choose between #47 and #48.

garphy’s picture

I think #48 is better from a DX point of view.

The File entity is not the file itself but a bunch of metadata about the file. It's not one of the representation of the file.

IMHO, the URL of the file is definitively a (computed) property of the File entity, so the serialization of a File entity should have an URL property.

Wim Leers’s picture

So this issue still needs test coverage. I think this would need a unit test, much like \Drupal\Tests\serialization\Unit\Normalizer\PrimitiveDataNormalizerTest.

dpovshed’s picture

I'll try to create a test for this during #DrupalDevDays .

Just a note: while patch #47 is applied nicely, #48 seems not to be valid and probably needs rerolling.

Munavijayalakshmi’s picture

Rerolled the #48 patch.

Wim Leers’s picture

Issue tags: +DevDaysSeville

@dpovshed Can you please assign the issue to yourself? :)

dpovshed’s picture

Assigned: Berdir » dpovshed

@Munavijayalakshmi, thanks for your try to help by rerolling a patch. However, if you provide a reroll or update of existing patch, good practice is to create an interdiff as well.

@wim-leers , sure, thanks! :-D

Wim Leers’s picture

@dpovshed++

dpovshed’s picture

The content of the attached patch:
- rerolled patch #48;
- draft of a test class UriNormalizerTest , where help is greatly appreciated with 'prophesizing' a value. Now it is giving false-negative result like this:

Failed asserting that Array &0 (
    'value' => null
    'url' => 'file_create_url:public://some-file.pdf'
) is identical to Array &0 (
    'value' => 'public://some-file.pdf'
    'url' => 'file_create_url:public://some-file.pdf'
).

@wim, could you please ping someone who can help us here :)

dpovshed’s picture

FileSize
4.38 KB

and interdiff is follows, of course :)

Status: Needs review » Needs work

The last submitted patch, 57: 2825487-57-uri_normalizer.patch, failed testing.

Wim Leers’s picture

tstoeckler’s picture

This should be green.

The problem was that since #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods we now normalize the value property of the URI item instead of just fetching it directly. This was not covered by the prophezised serializer so that it would just return NULL.

Since that prophecy has to be set up in the serializer directly, this means we can no longer have a single serializer for all test cases, but need a test-case specific one. So I had to refactor some parts. I also fixed some minor docs issues I found and the coding standards violation that DrupalCI found.

dpovshed’s picture

Tobias, thank you, I like very much what I see! Now we need independent reviewer here?

Wim Leers’s picture

Assigned: dpovshed » damiankloip
Issue tags: -Needs tests

Thanks!

So now we have a patch that has taken the #48 approach to completion, hurray!

I'd like this to be reviewed by @tedbow and @damiankloip.


In the meantime, here's my feedback. Keeping at NR, because the overall approach itself also needs further review.

  1. +++ b/core/modules/serialization/serialization.services.yml
    @@ -99,3 +99,11 @@ services:
    +      # Set the priority lower than the hal entity reference field item
    +      # normalizer, so that we do not replace that for hal_json but higher than
    +      # this modules generic field item normalizer.
    +      # @todo Find a better way for this in https://www.drupal.org/node/2575761.
    +      - { name: normalizer, priority: 8 }
    

    This is a c/p remnant from serializer.normalizer.entity_reference_field_item. Needs to be updated.

  2. +++ b/core/modules/serialization/src/Normalizer/UriItemNormalizer.php
    @@ -0,0 +1,33 @@
    +    $url = file_create_url($object->get('value')->getValue());
    

    This should be wrapped in a file_url_transform_relative() call.

damiankloip’s picture

Status: Needs review » Needs work

Couple o' comments:

  1. +++ b/core/modules/serialization/src/Normalizer/UriItemNormalizer.php
    @@ -0,0 +1,34 @@
    + * Adds direct download URL.
    

    Adds a ..

  2. +++ b/core/modules/serialization/src/Normalizer/UriItemNormalizer.php
    @@ -0,0 +1,34 @@
    +class UriItemNormalizer extends FieldItemNormalizer {
    

    Do we need a hal equivalent for this too to keep parity?

  3. +++ b/core/modules/serialization/src/Normalizer/UriItemNormalizer.php
    @@ -0,0 +1,34 @@
    +    // Only add the download url if it is not the same as the field item value.
    +    if ($url !== $object->get('value')->getValue()) {
    +      $data['url'] = $url;
    +    }
    

    This seems inconsistent if we conditionally add this 'url' key. Consumers would always have to check for its presence? Seems better to always include it, even if it is the same?

    Also, as we're dealing with a UriItem, should the key be 'uri' instead?

Lastly, is there a reason we do not have the URI as a computed property on UriItem? Normalization could then just use this without any work at all really.

Wim Leers’s picture

Lastly, is there a reason we do not have the URI as a computed property on UriItem? Normalization could then just use this without any work at all really.

That's the approach taken in #29 + #33.

Then in #38, that approach was questioned. A new kind of operation and hence a new Link header was suggested instead. This was implemented in #47.

But then that approach too was questioned in #48 — because the expectation is that all relevant data is in the response body. This would be the sole deviation of "critical information" (and not just "meta information") living in the response headers instead of the response body. All patches since then, including the one you reviewed in #61, are a continuation of this approach: an additional property added by the normalizer, but that does not live on the File entity as a computed property on the UriItem field.


I just went over all comments again, and I proposed exactly what you proposed, in #12.3: I proposed not a computed field, but a computed property. Except I got most of that wrong, and kept talking about FileItem (which is a specialized entity reference, for pointing to File entities specifically), rather than talking about a File entity's uri field, which indeed uses UriItem as you say.

However… UriItem is not just for file URIs (i.e. public://blah.png, private://blah.png, and so on). It's for any URI: also for mailto:security@drupal.org, https://www.drupal.org,a nd so on). So… I'm afraid a new computed property wouldn't work.

So then we're back to adding a new computed field on File entities, which requires adding a new base field definition (which is a soft BC break?).

I wonder if it'd be better to instead change

    $fields['uri'] = BaseFieldDefinition::create('uri')
      ->setLabel(t('URI'))
      ->setDescription(t('The URI to access the file (either local or remote).'))
      ->setSetting('max_length', 255)
      ->setSetting('case_sensitive', TRUE)
      ->addConstraint('FileUriUnique');

to:

    $fields['uri'] = BaseFieldDefinition::create('file_uri')
      ->setLabel(t('URI'))
      ->setDescription(t('The URI to access the file (either local or remote).'))
      ->setSetting('max_length', 255)
      ->setSetting('case_sensitive', TRUE)
      ->addConstraint('FileUriUnique');

And then adding FileUriItem? It'd only allow http://, https:// and any of the registered stream wrappers. If it's a registered stream wrapper, the computed property would indeed require generating a file URL (using file_url_transform_relative(file_create_url($file->get('uri')->value)), otherwise it'd simply be set to $file->get('uri')->value.


IOW: File entities are incompletely defined, and lack validation of their uri field (the only constraint is that they must be unique).

tstoeckler’s picture

So then we're back to adding a new computed field on File entities, which requires adding a new base field definition (which is a soft BC break?).

I don't see how that would break BC. In fact since computed properties don't have a field storage definition it would even require an update path (but for a cache clear).

I'm not knowledgeable about all this to have an opinion on whether a computed field is a good idea or not, just wanted to point that out. I.e. #2846554: Make the PathItem field type actually computed and auto-load stored aliases also is allowed for 8.x and that goes even a step further.

Wim Leers’s picture

Title: File entity should expose the file URL as a computed base field » Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field
Issue summary: View changes

This issue is currently fixing the normalization of \Drupal\file\Entity\File, i.e. the File entity. But we also need this issue to fix the normalization of \Drupal\file\Plugin\Field\FieldType\FileItem, i.e. the File entity reference field.

The IS even says this:

Add FileItemNormalizer.
Add ImageItemNormalizer.

For ImageItemNormalizer, we now have #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs. Updated IS.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

#66: agreed that adding a new base field (computed or not) is possible.

The point I was trying to make in #65 (but clearly failing to do so!) was that I think it's very wrong/very disruptive to add

+++ b/core/modules/serialization/src/Normalizer/UriItemNormalizer.php
@@ -0,0 +1,34 @@
+class UriItemNormalizer extends FieldItemNormalizer {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $supportedInterfaceOrClass = UriItem::class;
+
+  /**
+   * {@inheritdoc}
+   */
+  public function normalize($object, $format = NULL, array $context = []) {
+    $data = parent::normalize($object, $format, $context);
+    /** @var \Drupal\Core\Field\Plugin\Field\FieldType\UriItem $object */
+    $url = file_create_url($object->get('value')->getValue());
+    // Only add the download url if it is not the same as the field item value.
+    if ($url !== $object->get('value')->getValue()) {
+      $data['url'] = $url;
+    }
+
+    return $data;
+  }
+
+}

This code only makes sense for UriItems on File entities! But UriItem can be used anywhere!

In fact, the following use it:

  1. Feed's url, link and image base fields
  2. Item's link base field
  3. Comment's homepage base field
  4. Link's uri base field

All of those would be negatively impacted by this.


Therefore I think it's less disruptive to update

    $fields['uri'] = BaseFieldDefinition::create('uri')
      ->setLabel(t('URI'))
      ->setDescription(t('The URI to access the file (either local or remote).'))
      ->setSetting('max_length', 255)
      ->setSetting('case_sensitive', TRUE)
      ->addConstraint('FileUriUnique');

like this;

diff --git a/core/modules/file/src/Entity/File.php b/core/modules/file/src/Entity/File.php
index a68b9ba..30fc3a1 100644
--- a/core/modules/file/src/Entity/File.php
+++ b/core/modules/file/src/Entity/File.php
@@ -240,7 +240,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setLabel(t('Filename'))
       ->setDescription(t('Name of the file with no path components.'));
 
-    $fields['uri'] = BaseFieldDefinition::create('uri')
+    $fields['uri'] = BaseFieldDefinition::create('file_uri')
       ->setLabel(t('URI'))
       ->setDescription(t('The URI to access the file (either local or remote).'))
       ->setSetting('max_length', 255)

instead of like this:

diff --git a/core/modules/file/src/Entity/File.php b/core/modules/file/src/Entity/File.php
index a68b9ba..ddf16fe 100644
--- a/core/modules/file/src/Entity/File.php
+++ b/core/modules/file/src/Entity/File.php
@@ -246,6 +246,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
       ->setSetting('max_length', 255)
       ->setSetting('case_sensitive', TRUE)
       ->addConstraint('FileUriUnique');
+    $fields['something_new'] = BaseFieldDefinition::create('…')
+      …
 
     $fields['filemime'] = BaseFieldDefinition::create('string')
       ->setLabel(t('File MIME type'))

… because the current uri base field on File entities is extremely misleading: \Drupal\Core\Field\Plugin\Field\FieldType\UriItem allows any URI, it doesn't even validate the used URL scheme/protocol. It's a semantic mismatch.

That is why I think changing the field item type of the uri base field on File entities would be the least disruptive approach: it would allow us to add a FileUriItemNormalizer and all would be well.


However, doing so also means that the knowledge of this "public file URL" created by file_create_url() would not be present in the Typed Data metadata, and hence would be missing from the docs generated by https://www.drupal.org/project/schemata, https://www.drupal.org/project/docson or https://www.drupal.org/project/openapi.

So I'm +1 to adding a new computed base field to the File entity.

Wim Leers’s picture

damiankloip’s picture

Here is a start on this, not sure if this is the best way to handle to computed property. Still very rough

damiankloip’s picture

dawehner’s picture

I just tried out to manually set the "file URL" and nothing complained about it, it just allowed to change the value:

>>> $f = \Drupal\file\Entity\File::create(['uri' => 'public://foo.txt']);
=> Drupal\file\Entity\File {#8403
     #fid: Drupal\Core\Field\FieldItemList {#8696},
     #uuid: Drupal\Core\Field\FieldItemList {#8697},
     #langcode: Drupal\Core\Field\FieldItemList {#8694},
     #uid: Drupal\Core\Field\EntityReferenceFieldItemList {#8698},
     #filename: Drupal\Core\Field\FieldItemList {#8699},
     #uri: Drupal\Core\Field\FieldItemList {#8700},
     #filemime: Drupal\Core\Field\FieldItemList {#8701},
     #filesize: Drupal\Core\Field\FieldItemList {#8702},
     #status: Drupal\Core\Field\FieldItemList {#8703},
     #created: Drupal\Core\Field\FieldItemList {#8704},
     #changed: Drupal\Core\Field\ChangedFieldItemList {#8705},
     #url: Drupal\jsonapi\Field\FileDownloadUrl {#8706},
   }
>>> $f->url->value
=> "/sites/default/files/foo.txt"
>>> $f->url->value = 'public://bar.txt';
=> "public://bar.txt"
>>> $f->url->value
=> "public://bar.txt"
>>>

I'm wondering whether we can / should do anything about it?

e0ipso’s picture

One of the problems I faced with #2793809: [FEATURE] Provide direct download URLs for files and images was that in some scenarios the fields are accessed not via the magic method invocation, but via the field item list iterator.

Something like:

foreach($file->url as $file_item) {
  var_dump($file_item);
}

I'm wondering if we face a similar situation here.

dawehner’s picture

Status: Needs work » Needs review

I'd like to test the latest patch ...

Wim Leers’s picture

This should be postponed on #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts too, I was going to write, because then this issue would just need to add something to Typed Data, and it'd work automatically in REST/JSON API/GraphQL/OpenAPI … except then I noticed:

+++ b/core/modules/file/src/Entity/File.php
@@ -247,6 +247,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['url'] = BaseFieldDefinition::create('file_url')

+++ b/core/modules/file/src/FileUrl.php
@@ -0,0 +1,26 @@
+class FileUrl extends TypedData {

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileUrlItem.php
@@ -0,0 +1,48 @@
+class FileUrlItem extends FieldItemBase  {

+++ b/core/modules/file/tests/src/Kernel/FileUrlItemTest.php
@@ -0,0 +1,67 @@
+class FileUrlItemTest extends FieldKernelTestBase {

… that's exactly what this does! So this can merrily continue :)

Berdir’s picture

Status: Needs review » Needs work

I'm not 100% sure yet about the approach with the computed field.

The path on entities is a computed field, we also use it as a widget, it represents actual data stored elsewhere that we expose. This is a new field type that defines widget and formatter, which is pretty bogus.

#74 is also right. Computed fields don't just work yet, that's being discuss in #2392845: Add a trait to standardize handling of computed item lists. In the path issue, I had to go through all kinds of hoops to make field API work transparently, which is supposed to be generalized in that other issue. So we'd definitely need test coverage here, but we don't have the entity resource test version for file entities yet, so we can't easily extend it.

This is "just" a different representation of the uri field and its data. So maybe a better approach would be to make a computed property on the existing uri field? We can't add that to the generic UriItem but we could set a custom class for the file entity class?

And maybe we shouldn't actually go through a computed field or property at all but just handle it in on the normalizer level. As I mentioned a pretty long time ago, #2277705: Files don't have URI nor href did attempt to solve exactly this and it did for HAL for both file references and file entities by exposing the url of file entities to be the URL to the actual file. I think the implementation is wrong and file_entity reverts it, but maybe we can find an approach that makes more sense?

I'd also love to see an API method on the file entity for this, no matter how it internally works exactly.

+++ b/core/modules/file/src/FileUrl.php
@@ -0,0 +1,26 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getValue() {
+    $entity = $this->getParent()->getEntity();
+    $uri = $entity->uri->value;
+    return file_create_url($uri);
+  }

The file entity actually has a getFileUri() method that we could use wherever this code will then exactly live.

Wim Leers’s picture

Wim Leers’s picture

Title: Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field » [PP-1] Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field
Status: Needs work » Postponed

Thanks for the feedback, @Berdir, that's greatly appreciated!

So maybe a better approach would be to make a computed property on the existing uri field? We can't add that to the generic UriItem but we could set a custom class for the file entity class?

I like that!

And maybe we shouldn't actually go through a computed field or property at all but just handle it in on the normalizer level.

The problem with this is that it makes it impossible to auto-generate API docs, in e.g. https://www.drupal.org/project/openapi. It also means that you may have to repeat this for every normalization: "default", hal, jsonapi, a project-specific normalization, and so on. And finally, it means this won't be available in https://www.drupal.org/project/graphql.
This is why it's so valuable to have it as a computed property, and why #76 said this should be postponed on #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts. I don't know why I didn't mark this postponed in #76, but doing so now.

Again +1 for So maybe a better approach would be to make a computed property on the existing uri field? We can't add that to the generic UriItem but we could set a custom class for the file entity class?! 👏

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

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

Wim Leers’s picture

#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is now RTBC, and has had lots of Entity/Field/TypedData API maintainer feedback. Hopefully, it'll therefore land soon.

So let's start working on this patch again — it's also the last blocker of #1927648: Allow creation of file entities from binary data via REST requests, which is otherwise RTBC!

damiankloip’s picture

Status: Postponed » Needs review
FileSize
2.94 KB

OK, so are we thinking something like this? Before I sink too much time into it... (that's a rough patch disclaimer btw ;) )

garphy’s picture

Status: Needs review » Needs work

@damiankloip,

Your patch only deals with the File field side of the issue, not the File entity side, right ?

We're also supposed to add a full "url" property to serialization of file entities, or am I mistaken ?

I'm just asking because adding properties to the field level doesn't expose any information when using JSON API as in this case, file field are relationships and not attributes.

Wim Leers’s picture

AFAICT that'd result in a normalization like this:

…
'uri' => [
  'value' => 'public://llama.png',
  'uri' => 'https://example.com/sites/default/files/llama.png',
],
…

Looks great to me!


Your patch only deals with the File field side of the issue, not the File entity side, right ?

No, see this bit in the patch:

+++ b/core/modules/file/src/Entity/File.php
@@ -250,6 +251,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    // Override the default item class with a file specific implementation that
+    // provides an additional computed URL property.
+    // @see \Drupal\file\FileUriItem::propertyDefinitions()
+    $fields['uri']->getItemDefinition()->setClass(FileUriItem::class);
garphy’s picture

Yeah, you're right. Please disregard my previous comment :)

That said, I'm not actually seeing it with patch from #82 applied (alongside latest patch from #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts).

But I could also be lacking enough coffee for a Friday so I'll double check that.

garphy’s picture

Well... it seems to not be working atm, I still get only "value" sub property for the "uri" property :

{
    "fid": [
        {
            "value": 1
        }
    ],
    "uuid": [
        {
            "value": "fa688746-4b6d-4890-ae94-73ee1855b694"
        }
    ],
    "langcode": [
        {
            "value": "en"
        }
    ],
    "uid": [
        {
            "target_id": 1,
            "target_type": "user",
            "target_uuid": "a3015686-d17a-44db-8231-c4a77fab44b9",
            "url": "/user/1"
        }
    ],
    "filename": [
        {
            "value": "pr_icon.png"
        }
    ],
    "uri": [
        {
            "value": "public://2017-09/pr_icon_0.png"
        }
    ],
    "filemime": [
        {
            "value": "image/png"
        }
    ],
    "filesize": [
        {
            "value": 40874
        }
    ],
    "status": [
        {
            "value": true
        }
    ],
    "created": [
        {
            "value": "2017-09-14T14:22:22+00:00",
            "format": "Y-m-d\\TH:i:sP"
        }
    ],
    "changed": [
        {
            "value": "2017-09-14T14:22:22+00:00",
            "format": "Y-m-d\\TH:i:sP"
        }
    ],
    "url": [
        {
            "value": "/sites/default/files/2017-09/pr_icon_0.png"
        }
    ]
}

Let's see if I can craft a test case for this one.

garphy’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
551 bytes

Ok, I think what's missing is a setInternal(FALSE) because of setComputed(TRUE) which makes the sub-property implicitly considered internal.

So, this new patch needs #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts which provides setInternal().

garphy’s picture

...also, I might use some guidance to find where it's appropriate to add some tests for this. I didn't manage to find where we have test for the current serialization of file entities.

damiankloip’s picture

Yes, that is what we need (as computed properties are internal by default). I was avoiding that for now as it now fails unless we add that patch here too.... We can do a load of other work here before we introduce that dependency.

garphy’s picture

You're right. I should have posted a do-not-test patch.

damiankloip’s picture

Status: Needs review » Needs work

The last submitted patch, 87: 2825487-87.patch, failed testing. View results

Wim Leers’s picture

Title: [PP-1] Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field » [PP-2] Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field
Issue summary: View changes
Related issues: +#2910211: Allow computed exposed properties in ComplexData to support cacheability.

Actually, this is hard-blocked on #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts, but then after that it'll be blocked on #2910211: Allow computed exposed properties in ComplexData to support cacheability. (which is itself blocked on #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts), because a generated file URL also needs cacheability metadata: it varies by the url.site cache context.

This patch should be built on top of both. There's no reason why we can't already work on getting that patch ready! Both of those blocking patches are unlikely to still change significantly.

Wim Leers’s picture

Priority: Normal » Major

Oh and this blocks #1927648: Allow creation of file entities from binary data via REST requests, which is major, meaning this should be major too.

damiankloip’s picture

Yes - sure. I was getting at the new file item class, the computed property class, and tests for those. That we can have done here ready to go hopefully.

Wim Leers’s picture

👍

damiankloip’s picture

Status: Needs work » Needs review
FileSize
6.71 KB
3.77 KB

Here is a unit test for \Drupal\file\FileUrl and a kernel test for using the actual overridden FileUriItem with the overridden 'url' property definition that's added.

Testing the tests.

dawehner’s picture

+++ b/core/modules/file/src/FileUrl.php
@@ -0,0 +1,44 @@
+    $this->url = $this->getParent()->getEntity()->url();

url() is deprecated, it says you should use toUrl instead. Any reason not to?

damiankloip’s picture

Not really - lets make things even more verbose :D

$this->getParent()->getEntity()->toUrl()->setAbsolute()->toString();

Status: Needs review » Needs work

The last submitted patch, 99: 2825487-99.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

Hmm, @dawehner. toUrl() is not really working out with file entities. I think it requires us to set a URI callback or something? Otherwise, using a template doesn't really work. url() works on files becaise it disregards the parameters passed in and just gets the file uri and passes it to file_create_url.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
2.08 KB

Maybe like.

Status: Needs review » Needs work

The last submitted patch, 102: 2825487-102.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Title: [PP-2] Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field » [PP-1] Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field
Wim Leers’s picture

#97:

Any reason not to?

Yes: because File::url() is a special duck, and fixing it is a tricky issue of its own, see #2402533: No link template "canonical" found for the "file" entity type. (That's also what #101 is alluding to.)

So we should go back to #97.


Review of #97:

  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -250,6 +251,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    // Override the default item class with a file specific implementation that
    +    // provides an additional computed URL property.
    +    // @see \Drupal\file\FileUriItem::propertyDefinitions()
    +    $fields['uri']->getItemDefinition()->setClass(FileUriItem::class);
    

    Why not do this instead:

    a/core/modules/file/src/Entity/File.php
    +++ b/core/modules/file/src/Entity/File.php
    @@ -243,7 +243,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
           ->setLabel(t('Filename'))
           ->setDescription(t('Name of the file with no path components.'));
     
    -    $fields['uri'] = BaseFieldDefinition::create('uri')
    +    $fields['uri'] = BaseFieldDefinition::create('file_uri')
           ->setLabel(t('URI'))
           ->setDescription(t('The URI to access the file (either local or remote).'))
           ->setSetting('max_length', 255)
    

    ?

  2. +++ b/core/modules/file/src/FileUriItem.php
    @@ -0,0 +1,30 @@
    +class FileUriItem extends UriItem {
    

    This class would need a @FieldType annotation, if we do what I said in point 1. (UriItem also has a @FieldType annotation.)

  3. +++ b/core/modules/file/src/FileUriItem.php
    @@ -0,0 +1,30 @@
    +    // @todo should this just be a string?
    

    No, I think using the uri @DataType plugin (i.e. \Drupal\Core\TypedData\Plugin\DataType\Uri)) makes sense.

  4. +++ b/core/modules/file/src/FileUrl.php
    @@ -0,0 +1,44 @@
    +class FileUrl extends TypedData {
    ...
    +    $this->url = $this->getParent()->getEntity()->url();
    

    Either:

    1. we have to block this issue on #2910211: Allow computed exposed properties in ComplexData to support cacheability. and make this class implement CacheableDependencyInterface and then have this return the 'url.site' cache context (because file_create_url() returns an absolute URL for the current site in a multisite setup)
    2. or we have to not pass the result of file_create_url(), but that of file_url_transform_relative(file_create_url()), then the resulting value does not need cacheability metadata, and this issue would not be blocked on #2910211: Allow computed exposed properties in ComplexData to support cacheability.!
  5. +++ b/core/modules/file/src/FileUrl.php
    @@ -0,0 +1,44 @@
    +  public function getValue() {
    +    if ($this->url !== NULL) {
    ...
    +  public function setValue($value, $notify = TRUE) {
    +    $this->url = $value;
    

    I find this extremely confusing, but I see the same at \Drupal\datetime\DateTimeComputed and \Drupal\text\TextProcessed.

  6. +++ b/core/modules/file/tests/src/Kernel/FileUriItemTest.php
    @@ -0,0 +1,35 @@
    +   * Tests the file entity override of the Url field.
    

    s/Url/URI/

damiankloip’s picture

Status: Needs work » Needs review
FileSize
7.73 KB
1.31 KB

Agreed, File is a special case, with a very much shoe-horned implementation of url() anyway. Related to this, berdir had a comment in #77 :

I'd also love to see an API method on the file entity for this, no matter how it internally works exactly.

Which suggests adding another method that isn't the deprecated url() method to file entities.

1 & 2. I was going to define a new FieldType originally but then thought it might be good to follow a similar pattern to the User entity with its name field. As it's a one off case, it seemed like a good idea to not define a new plugin (but maybe it's not.. :) ).
3. Yeah, I was heading that way too. Removed that TODO.
4. Hmm, using the relative URI only is interesting, and could be a good idea. Shall we switch to that? What are the benefits of having the full URL? Just makes it easier for consumers to use I guess.
5. Yes, I wasn't 100% on this either but remember seeing the same in both of the examples you mentioned, so added it in.
6. Fixed

We can also add back @garphy's diff from #87 :) Let's see how this gets on. We should maybe decide here what we do with the hal FileEntityNormalizer? #1927648: Allow creation of file entities from binary data via REST requests does make some changes in this space too, but as this normalizer currently overrides the uri property with the full URL; this patch would mean we'll get a uri and a url property. So we'll need to fix that :)

We might want some rest resource coverage for this too.

Status: Needs review » Needs work

The last submitted patch, 106: 2825487-106.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

doh - forgot to change the actual URL code back. I based it on the wrong patch. rerolling.

Wim Leers’s picture

Title: [PP-1] Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field » Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field

As it's a one off case, it seemed like a good idea to not define a new plugin (but maybe it's not.. :) ).

I think there are other entity types that might want to have a field with a file URL, so I think a new @FieldType plugin would be good.

Hmm, using the relative URI only is interesting, and could be a good idea. Shall we switch to that? What are the benefits of having the full URL? Just makes it easier for consumers to use I guess.

Yes, the benefit of having a full URL is that it's easier for consumers. But those consumers already have to know the domain to make their requests against anyway, so it should not be a big deal to process file URLs. On top of that, if they're using a CDN that rewrites their file URLs to a different domain, then they will still get a full URL, because it's no longer relative to the current site (and still doesn't vary by the url.site cache context).
(Note that #2910211: Allow computed exposed properties in ComplexData to support cacheability. just landed too, so this is unblocked either way.)

We can also add back @garphy's diff from #87 :) Let's see how this gets on

+1

We should maybe decide here what we do with the hal FileEntityNormalizer?

Right.

#1927648: Allow creation of file entities from binary data via REST requests does make some changes in this space too, […]

It basically only removes the denormalize() method.

[…] but as this normalizer currently overrides the uri property with the full URL; this patch would mean we'll get a uri and a url property. So we'll need to fix that :)

Right. I think the answer is pretty simple there: also remove the normalize() method.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
8.41 KB
5.26 KB

I think there are other entity types that might want to have a field with a file URL

This would require us to have some more generic logic in the FileUrl class than we have? our naming as assumptions are on methods that the file entity has/returns.

Right. I think the answer is pretty simple there: also remove the normalize() method.

Yes, pretty much! That's what I was thinking. That's all the normalize() does. I modified the FileNormalizeTest but we might end up removing that in favour of the RestResourceTest coverage instead.

I have converted the FileUriItem to its own FieldType plugin. The main reason I didn't do this before.

Also converted to the relative URL. The point about this being easier to support if using a CDN is also a good one.

Status: Needs review » Needs work

The last submitted patch, 110: 2825487-110.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

Note: This reminds me of another reason I didn't go with a new plugin initially; Aside from the widget using a different field, we also need the FileUriFormatter to allow file_uri. Ideally it would only have file_uri as an allowed type but I think it's too much of a BC break to remove that (can can see evidence of this from the FileEntityFormatterTest test fail).

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
9.54 KB
  1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
    @@ -1,13 +1,23 @@
    + * File specific plugin implementation of a URI Item to provide a full URL.
    

    Nit: s/File specific/File-specific/

  2. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -44,17 +44,6 @@ public function __construct(EntityManagerInterface $entity_manager, ClientInterf
    -    $data['uri'][0]['value'] = $this->getEntityUri($entity);
    

    This change is causing the Media tests to fail. Fortunately, it was decided in the issue that added those tests (#2835767: Media + REST: comprehensive test coverage for Media + MediaType entity types) that those tests are present merely to ensure no unknown changes. https://www.drupal.org/project/drupal/releases/8.4.0#media explicitly says:

    Similarly, the REST API and normalizations for Media is not final and support for decoupled applications will also be improved in a future release.

    Therefore it's fine to change the expectations in the Media tests.

    Did that in the attached patch.

Wim Leers’s picture

#112: Can't that be easily remedied with:

diff --git a/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php b/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php
index 055e32d..0facb7e 100644
--- a/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php
+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php
@@ -13,7 +13,8 @@
  *   id = "file_uri",
  *   label = @Translation("File URI"),
  *   field_types = {
- *     "uri"
+ *     "uri",
+ *     "file_uri",
  *   }
  * )
  */

?

Status: Needs review » Needs work

The last submitted patch, 113: 2825487-113.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Issue summary: View changes

This hard-blocks #1927648: Allow creation of file entities from binary data via REST requests and #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler, both of which are super important. But this is already major, and is already tagged blocker, so issue metadata is already up-to-date.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
9.97 KB
518 bytes

@wimLeers - yes, precisely. That's what I was trying to say in #112 :) It just depends if we want to go down this route - that's mainly what I'm getting at. Bleeding into UI and widgets, ever so slightly. This is why the original approach is slightly cleaner IMO. I am ok with both though.

It's just kind of strange now that FileUriFormatter is specific to files, so really should only now use 'file_uri' IMO, but that could break anything trying to use it currently that relies on the underlying field being 'uri' only.

Status: Needs review » Needs work

The last submitted patch, 117: 2825487-117.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
15.3 KB
7.92 KB

Related to removing the hal FileEnityNormalizer::normalize() method, we also need to remove the denormalize method, as it tries to make a request to fetch a file using the URI (public:// or so). We could modify this, but as #1927648: Allow creation of file entities from binary data via REST requests is removing it anyway, it makes sense to remove the entire class here. That issue leaves the class currently, as it keeps the normalize() method :P

So this removes the FileEntityNormalizer, FileEntityDenormalizeTest, and the service definition from hal.services.yml.

Fixed the other fails too hopefully.

Status: Needs review » Needs work

The last submitted patch, 119: 2825487-119.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
15.37 KB
1.95 KB

Fixes.

Getting back to one of @berdir's comments; do we want to add a new method to FileInterface for this? something like getRelativeUrl() ?

Wim Leers’s picture

It just depends if we want to go down this route - that's mainly what I'm getting at. Bleeding into UI and widgets, ever so slightly. This is why the original approach is slightly cleaner IMO. I am ok with both though.

I'd agree, but … it doesn't show up in the UI, because:

*   no_ui = TRUE,

That's in both in the existing UriItem and in the new FileUriItem, so this is a non-issue :)

[…] we also need to remove the denormalize method […] is removing it anyway, it makes sense to remove the entire class here

👍

Getting back to one of @berdir's comments; do we want to add a new method to FileInterface for this? something like getRelativeUrl() ?

@Berdir said this in #77: I'd also love to see an API method on the file entity for this, no matter how it internally works exactly.

I think that's out of scope. There's no reason to block this on that, so I think this belongs in a follow-up issue. Unless Berdir feels strongly that it should be done here/first :)


  1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
    @@ -0,0 +1,40 @@
    + * File specific plugin implementation of a URI Item to provide a full URL.
    

    Nit: s/File specific/File-specific/, s/Item/item/

  2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
    @@ -0,0 +1,40 @@
    + *   default_formatter = "uri_link",
    

    Nit: file_uri

  3. +++ b/core/modules/file/src/Entity/File.php
    @@ -8,6 +8,7 @@
    +use Drupal\file\FileUriItem;
    

    Nit: Can be removed now. :)

damiankloip’s picture

Fixed the 3 points from #122, thanks!

I am also fine with moving any additional file entity methods to a new issue. The naming etc.. can be subject to bikeshedding there instead :P

Another question, I think the UriWidget needs "file_uri" added to it as well, otherwise the default widget would not be valid, not sure if we even need it or not. This then leads us down a dependency type question, this is now meaning that core formatters and widgets, have a file specific field type in their allowed types list..

tstoeckler’s picture

this is now meaning that core formatters and widgets, have a file specific field type in their allowed types list..

In that case let's use hook_field_(formatter|widget)_info_alter() to add the file-specific field from file.module. telephone_field_formatter_info_alter() is the precedent for that.

damiankloip’s picture

Yes, that's the approach I was thinking if we continue down this route too. tstoeckler++

Wim Leers’s picture

Another question, I think the UriWidget needs "file_uri" added to it as well, otherwise the default widget would not be valid, not sure if we even need it or not.

Right. (Even though this is a field type that is only used for a computed field for now, which means there's no need for a widget. But the @FieldType annotation indicates this is a required annotation key. Which means we should do what you suggested!)

And, yes, of course, #124++

damiankloip’s picture

Yes, exactly WimLeers! It't not really 'needed' but.... :)

Wim Leers’s picture

Title: Fix normalization of File entities & File fields: file entities should expose the file URL as a computed base field » Fix normalization of File entities: file entities should expose the file URL as a computed base field
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs change record
Related issues: +#2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization), +#2277705: Files don't have URI nor href

This is looking great now! Feels like we're so close! Which means that it's … thorough review time!, which includes nitpicks and all BC considerations… 😳 😁

  1. +++ b/core/modules/file/file.module
    @@ -48,6 +48,17 @@ function file_help($route_name, RouteMatchInterface $route_match) {
    +  $info['uri']['field_types'][] = 'file_uri';
    

    👍 We do this via an alter hook because the uri widget lives in core/lib/….

  2. +++ b/core/modules/file/src/Entity/File.php
    @@ -243,7 +243,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    -    $fields['uri'] = BaseFieldDefinition::create('uri')
    +    $fields['uri'] = BaseFieldDefinition::create('file_uri')
    

    👍 This is the crucial change. We change this to use the file_uri field type, which extends the uri field type, and just adds a computed property.

  3. +++ b/core/modules/file/src/FileUrl.php
    @@ -0,0 +1,45 @@
    +class FileUrl extends TypedData {
    

    ❓ Should this be called ComputedFileUrl?

  4. +++ b/core/modules/file/src/FileUrl.php
    @@ -0,0 +1,45 @@
    +   * Cached URL.
    

    Nit: s/Cached URL/Computed root-relative file URL./

  5. +++ b/core/modules/file/src/FileUrl.php
    @@ -0,0 +1,45 @@
    +    $uri = $this->getParent()->getEntity()->getFileUri();
    

    ❓ Should we add an assertion to ensure we're dealing with File entities, because if we aren't, then getFileUri() won't be available?

    That'd mean adding this line first:
    assert($this->getParent()->getEntity() instanceof FileInterface);

  6. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/FileUriFormatter.php
    @@ -13,7 +13,8 @@
    - *     "uri"
    + *     "uri",
    + *     "file_uri",
    

    👍 It's okay to add file_uri directly here, instead of via an alter hook, because this formatter is owned by the file module.

  7. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
    @@ -0,0 +1,40 @@
    + *   description = @Translation("An entity field containing a URI and URL for a file path."),
    

    Suggested alternative: An entity field containing a file URI, and a computed root-relative file URL.

  8. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
    @@ -0,0 +1,40 @@
    +      ->setLabel(t('Full URL'))
    

    Suggested alternative: Root-relative file URL.

  9. +++ b/core/modules/file/tests/src/Kernel/FileUriItemTest.php
    @@ -0,0 +1,36 @@
    + * Custom URI field item test.
    

    Nit: s/Custom URI/File URI/

    Also: let's add

    @see \Drupal\file\Plugin\Field\FieldType\FileUriItem
    @see \Drupal\file\FileUrl
  10. +++ b/core/modules/file/tests/src/Kernel/FileUriItemTest.php
    @@ -0,0 +1,36 @@
    +    $this->assertSame(file_url_transform_relative(file_create_url($uri)), $file->uri->url);
    

    I think that in this case, it'd be clearer to not use the API functions to determine the expected value. But instead:

    $this->assertSame(base_path() . $this->siteDirectory . '/files/druplicon.txt', $file->uri->url);
    

    Then at least it's impossible for the expectation to change over time, and it's just a string assembled from parts.

  11. +++ b/core/modules/file/tests/src/Unit/FileUrlTest.php
    @@ -0,0 +1,87 @@
    +    $typed_data = new FileUrl($definition->reveal(), 'test', $parent->reveal());
    ...
    +    $typed_data = new FileUrl($definition->reveal(), 'test', $parent->reveal());
    ...
    +    $typed_data = new FileUrl($definition->reveal(), 'test', $parent->reveal());
    

    Nit: 'test' -> $this->randomMachineName().

  12. +++ b/core/modules/file/tests/src/Unit/FileUrlTest.php
    @@ -0,0 +1,87 @@
    +    $expected = file_url_transform_relative(file_create_url($this->testUrl));
    

    Same remark here as two points earlier.

  13. +++ b/core/modules/file/tests/src/Unit/FileUrlTest.php
    @@ -0,0 +1,87 @@
    +    // Do this a second time to confirm the same value is returned but the value
    +    // isn't retrieved from the parent entity again.
    +    $this->assertSame($expected, $typed_data->getValue());
    ...
    +    // Do this a second time to confirm the same value is returned but the value
    +    // isn't retrieved from the parent entity again.
    +    $this->assertSame($this->testUrl, $typed_data->getValue());
    ...
    +    // Setting the value should explicitly should mean the parent entity is
    +    // never called into.
    +    $typed_data->setValue($this->testUrl, FALSE);
    

    👍 Thorough!

  14. +++ b/core/modules/file/tests/src/Unit/FileUrlTest.php
    @@ -0,0 +1,87 @@
    +    // Setting the value should explicitly should mean the parent entity is
    

    Nit: "should explicitly should" needs fixing :P

  15. +++ b/core/modules/file/tests/src/Unit/FileUrlTest.php
    @@ -0,0 +1,87 @@
    +  public function testSetValueNoNotify() {
    

    ❓ Shouldn't this still assert that the value was set, by calling ->getValue(), just like we do in testSetValue()?

    ❓ Couldn't this be merged with testSetValue() and be updated to use a data provider?

  16. Note that the above fixes it for File entities, NOT file fields! That was the original scope of the issue. The problem to this day, however, is that you cannot get the URL for a File entity at all. And file fields always
    point to a File entity. Therefore, I think changing the normalization of file fields (by adding a computed property to file fields) is a nice-to-have follow-up. This current scope of the patch is the must-have. The current patch allows one to follow a file field's reference to a File entity, get the File entity, and in there the file URL can be found.

    Created #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization) for that.

  17. +++ b/core/modules/hal/hal.services.yml
    @@ -12,11 +12,6 @@ services:
    -  serializer.normalizer.file_entity.hal:
    
    index ec870e9e14..0000000000
    --- a/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    +++ b/core/modules/hal/tests/src/Kernel/FileNormalizeTest.php
    @@ -44,7 +44,10 @@ public function testNormalize() {
    -        ['value' => file_create_url($file->getFileUri())],
    +        [
    +          'value' => $file->getFileUri(),
    +          'url' => file_url_transform_relative(file_create_url($file->getFileUri())),
    +        ],
    

    Then this. This is the really tricky stuff, the crucial stuff when it comes to BC.

    This is the hacky work-around that the HAL normalizer introduced in #2277705: Files don't have URI nor href, for which we now must face the awful consequences. That patch landed with barely any review, and now we must maintain BC with that commit until D9.

    This issue solves the problem generically, for all formats: json and xml (which uses the default normalization), as well as hal_json (which uses the HAL normalizer which is being changed in the cited hunk), as well as JSON API and GraphQL in contrib. That's what #2277705 should have done, but alas…

    The trickiness lies in the fact that we should do the right thing (i.e. what this patch is doing), but we should also maintain BC. In this case, that means:

    1. adding a bc_file_uri_as_url_normalizer key to hal.settings, set to false in default config, but set to true by hal_update_8501() (similar to bc_timestamp_normalizer_unix in serialization.settings + serialization_update_8401())
    2. when that setting is set to false (the default), FileEntityNormalizer::normalize() should do NOTHING. That means the HAL normalization (as well as the normalization for the json and xml formats) will look like this:
      [
        'value' => $file->getFileUri(),
        'url' => file_url_transform_relative(file_create_url($file->getFileUri())),
      ]
      

      aka

      [
        'value' => 'public://llama.jpg',
        'url' => '/sites/default/files/llama.jpg',
      ]
      

      IOW: you can now access both the stored value (a file URI) and the computed URL for it, which you can use to for example embed the file in an article. Both the stored data and the often-needed computed data is available.

    3. when that setting is set to true, FileEntityNormalizer::normalize() should do the same as today
      [
        'value' => file_create_url($file->getFileUri()),
        'url' => file_url_transform_relative(file_create_url($file->getFileUri())),
      ]
      

      aka

      [
        'value' => 'https://example.com/sites/default/files/llama.jpg',
        'url' => '/sites/default/files/llama.jpg',
      ]
      

      IOW: the existing value key's value remains unchanged, we just add the new key: url. Like today in HEAD, there is no way to access the actual file URI. The stored data is never available, the often-needed computed data is available in two slightly different forms. This provides existing applications with an easy migration path.

    Until now, #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler has been marked as blocked on this. Because, ideally, we'd first fix the normalization, and then add comprehensive test coverage. But the unfortunate reality is that it's our responsibility to ensure both the broken-by-design normalization in HEAD and the proper normalization being added in this issue are supported. Because we don't want to break existing applications.

    So I think this actually needs to be blocked on #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler before it can land. This patch can be worked on in the mean time though: to add the necessary BC layer + update path test. Once #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler is in, this patch should need to update the test coverage that that introduced, and should then add an explicit BC layer test (like we've done in the past).

  18. +++ /dev/null
    --- a/core/modules/hal/tests/src/Functional/EntityResource/Media/MediaHalJsonAnonTest.php
    +++ b/core/modules/hal/tests/src/Functional/EntityResource/Media/MediaHalJsonAnonTest.php
    

    👍 The changes here are okay, per #2895573: Decide on normalization of Media module entities and as described in #113.2.

  19. +++ /dev/null
    @@ -1,75 +0,0 @@
    -class FileDenormalizeTest extends BrowserTestBase {
    

    This is being fixed properly in #1927648: Allow creation of file entities from binary data via REST requests, to which this issue is a blocker. If possible, we should revert this change, and leave it up to #1927648 to remove this test and the denormalize() method. i.e. I think we should revert much of #119, to manage scope of this issue appropriately, even though it's the right thing to do.

WHEW! Sorry for the extremely long issue comment, I hope it helps this issue get to the finish line! I also updated the issue title + issue summary.

Wim Leers’s picture

Title: Fix normalization of File entities: file entities should expose the file URL as a computed base field » Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field

Fixed inaccuracy in title.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
16.4 KB
6.65 KB

Nice review!

3. Sounds OK to me - changed.
4. Done
5. Added
7. Done
8. Done
9. Done
10. Done
11. I’m not sure why we need a random machine name here. Doesn’t seem to add anything? Changed anyway.
12. Done
14. Done
15. Sure, why not. Done
19. Hmm, yes - I guess we do. It’s just, like you say, unfortunate. I will add later.
20. This test can be brought back once 19 is done. We will then have to rely on the BC setting for that test. It does make sense, just nice to be rid of it.

Here is a patch with all other changes except 19 and 20.

A point about 19 update path - in the previous string casting issue for serialization, we did the opposite I think. We defaulted the setting to the same as the default. So the new behaviour was always used. Having to opt in to keep the BC setting by changing the config to TRUE. I think we should use the same here maybe, thoughts?

damiankloip’s picture

Ok, this:

- Returns the FileEntityNormalizer
- Returns the FileDenormalizeTest (with added setting of BC config value)
- Adds BC config stuff mentioned above, with the inverting of the logic I mentioned in #130, to match how we handle this in the upgrade path for the serialization stuff we have done previously.

Added a CR too : [https://www.drupal.org/node/2925783. It's referenced in the hal update function in the patch.

Status: Needs review » Needs work

The last submitted patch, 131: 2825487-131.patch, failed testing. View results

damiankloip’s picture

Ah, so the fails:

The "serializer.normalizer.file_entity.hal" service relies on the deprecated "Drupal\hal\Normalizer\FileEntityNormalizer" class. It should either be deprecated or its implementation upgraded

@WimLeers, should we not make EntityNormalizer deprecated? We would ideally be removing this in D9 either way, I think.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.05 KB
18.51 KB

#130
8. was not actually done.
11. we don't need that, but hardcoding to 'test' suggests that this string is important. Passing a random safe string instead means we demonstrate none of the output depends on it.
19. I think you meant 17?
20. I think you meant 19?

A point about 19 update path - in the previous string casting issue for serialization, we did the opposite I think. We defaulted the setting to the same as the default. So the new behaviour was always used. Having to opt in to keep the BC setting by changing the config to TRUE. I think we should use the same here maybe, thoughts?

That's not true, the new behavior was not always used by default.

Thoughts:

  • In both of those existing cases, their names refer to the original behavior. Which is why I proposed to do the same here.
  • Note: for both bc_primitives_as_strings and bc_timestamp_normalizer_unix, we added normalizer behavior. In this case, we're removing normalizer behavior.
  • bc_primitives_as_strings: false is the default, and serialization_update_8302() also sets it to false for existing sites. Because we deemed it extremely unlikely that consuming code would break (consuming code was likely already doing some casting). IOW: sites must opt out.
  • bc_timestamp_normalizer_unix: false is the default, but serialization_update_8401() sets it to true for existing sites. Because the normalization changes significantly, so consuming code would certainly break. IOW: sites must opt in.
  • I think that in this case (proposed name: bc_file_uri_as_url_normalizer), consuming code would also certainly break. IOW: sites must opt in.

#131:

  1. +++ b/core/modules/hal/hal.install
    @@ -31,3 +31,15 @@ function hal_update_8301() {
    +  return t('The REST API will no longer return the full URL for file entity URI values. It will return the URI value itself, with an additional URL property providing a root relative file path. If your site depends on these value being strings, <a href="https://www.drupal.org/node/2925783">read the change record to learn how to enable the BC mode.</a>');
    

    This is copy/pasted from serialization_update_8302(). But it's not the entire REST API that is affected, only HAL+JSON responses.

  2. +++ b/core/modules/hal/hal.services.yml
    @@ -12,6 +12,11 @@ services:
    +  serializer.normalizer.file_entity.hal:
    

    Let's add a deprecated: "…" key-value pair.

    See http://symfony.com/doc/current/service_container/alias_private.html#depr....

  3. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -0,0 +1,84 @@
    +  protected $config;
    

    Wouldn't $hal_settings be better?

  4. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -0,0 +1,84 @@
    +  public function __construct(EntityManagerInterface $entity_manager, ClientInterface $http_client, LinkManagerInterface $link_manager, ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_facotry) {
    ...
    +    $this->config = $config_facotry->get('hal.settings');
    

    s/facotry/factory/

  5. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -0,0 +1,84 @@
    +    if ($this->config->get('bc_file_uri_as_url_normalizer')) {
    

    This is missing

    $this->addCacheableDependency($this->config);
    

    (Possible since #2910211: Allow computed exposed properties in ComplexData to support cacheability..)


#133:
All tests pass, those are just deprecation notices. Also, WOW, I had no idea that just adding @deprecated to a class powering a service would automatically make Symfony trigger these errors!

\Symfony\Component\DependencyInjection\ContainerBuilder::createService() is where this magic happens:

            if (!$definition->isDeprecated() && is_array($factory) && is_string($factory[0])) {
                $r = new \ReflectionClass($factory[0]);

                if (0 < strpos($r->getDocComment(), "\n * @deprecated ")) {
                    @trigger_error(sprintf('The "%s" service relies on the deprecated "%s" factory class. It should either be deprecated or its factory upgraded.', $id, $r->name), E_USER_DEPRECATED);
                }
            }

The solution is to do what I said in my #131.2 review (to indicate that not just the service class is deprecated, but in fact the whole service is), and to then explicitly add it to \Drupal\Tests\Listeners\DeprecationListener::getSkippedDeprecations(). That is new since #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code, and it's also the reason #133 failed at all: using deprecated APIs can now cause test failures!

damiankloip’s picture

Nice! Yes, all the tests were deprecation notices, I also didn't know that would actually fail tests, or about adding deprecations to the listener!

Indeed I totally did miss 8! done now.

Your thoughts on defaulting the BC setting to TRUE in the update handler make sense also, changed!

I had a look at #2910211: Allow computed exposed properties in ComplexData to support cacheability. - didn't actually see that before it was committed. Trying to support cacheability with the serializer is... meh (not really a nice way to do it).

Removing change notice tag as we have https://www.drupal.org/node/2925783

Status: Needs review » Needs work

The last submitted patch, 135: 2825487-135.patch, failed testing. View results

damiankloip’s picture

Status: Needs work » Needs review
FileSize
17.74 KB
783 bytes

Whoops, forgot to pass context.

Status: Needs review » Needs work

The last submitted patch, 137: 2825487-137.patch, failed testing. View results

Wim Leers’s picture

I also didn't know that would actually fail tests, or about adding deprecations to the listener!

Well, that's understandable, because that behavior is brand new!

Trying to support cacheability with the serializer is... meh (not really a nice way to do it).

Yep, the problem is that the return value of a normalizer doesn't carry cacheability metadata (and can't), which is why we have to pass it out-of-band.

Whoops, forgot to pass context.

:) Now the only remaining thing is to add a MediaResourceTestBase::getExpectedCacheTags(), which can call the parent, but must add the config:hal.settings cache tag. For an example, see \Drupal\Tests\hal\Functional\EntityResource\EntityTest\EntityTestHalJsonInternalPropertyNormalizerTest::getExpectedCacheTags().

Wim Leers’s picture

Title: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field » [PP-1] Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field
Issue summary: View changes
Issue tags: +Needs tests, +Needs update path, +Needs update path tests

All my remarks in #128 have been addressed, except for:

  1. BC layer tests (blocked on #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler) → Needs tests + PP-1
  2. Update path. → Needs update path
  3. Update path tests. → Needs update path tests

The two latter ones can already be done.

Wim Leers’s picture

Related: #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL. Ironically, that is a bug reporting exactly what is wrong in HEAD, but we can't fix HEAD because it'd break BC. Which is why I explained in #128 why we need this BC layer (which the patch now has). Confusing? Yes!

So, AFAICT, #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL is unfixable. It can only be fixed on existing sites by having them opt out of the BC layer. This issue will fix it on new sites though.

Thoughts, @damiankloip? Do you agree?

Wim Leers’s picture

Just another small review while looking at this for #2922487: Follow-up for #2910211: fix all deprecation warnings, which is closely related.

  1. +++ b/core/modules/hal/config/install/hal.settings.yml
    @@ -1,3 +1,4 @@
    +bc_file_uri_as_url_normalizer: false
    

    Missing comment. Also, should we comply with #2919845: Create a 'bc' top level item in serialization.settings config object?

  2. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -9,6 +10,8 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    

    8.5.0
    9.0.0

  3. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -26,6 +29,13 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    +   * @var \Drupal\Core\Config\Config
    

    \Drupal\Core\Config\ImmutableConfig

  4. +++ b/core/modules/hal/tests/src/Functional/FileDenormalizeTest.php
    @@ -20,6 +20,17 @@ class FileDenormalizeTest extends BrowserTestBase {
    +  protected function setUp() {
    

    Missing inheritdoc

Wim Leers’s picture

Title: [PP-1] Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field » Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field
Issue summary: View changes
Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
18.67 KB

Here's a rebase (with one conflict fixed) plus the trivial fix for the 3 failing Media entity type REST tests in #137.

Wim Leers’s picture

FileSize
761 bytes
19.31 KB

This will make the tests for the "default" normalization (the one provided by serialization.module, for the json and xml formats) pass.

As you can see, just a new key-value pair that shows up.

(The tests that were added by #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler.)

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs tests
FileSize
4.51 KB
23.62 KB

And this then updates the tests for the HAL normalization (the hal_json format is the only one using it) to test the new default. It also includes tests for BC.

IOW: this added the BC layer tests. This too is trivial.

(I figured I'd add those since I know the entity REST test coverage structure very well, so I can get that done much faster than Damian.)


Now only #142 + update path + update path tests are left to be done, then this is ready. Back to @damiankloip!

Berdir’s picture

(crosspost with #146, I reviewed the previous patch but I don't think the relevant parts here changed)

  1. +++ b/core/modules/file/file.module
    @@ -49,6 +49,17 @@ function file_help($route_name, RouteMatchInterface $route_match) {
    +function file_field_widget_info_alter(array &$info) {
    +  // This allows setting a valid default widget in the FileUriItem annotation.
    +  // It's not strictly needed, but it's a required value on the annotation so
    +  // should be a valid value.
    +  // @see \Drupal\file\Plugin\Field\FieldType\FileUriItem
    +  $info['uri']['field_types'][] = 'file_uri';
    

    the comment is confusing and seems to be backwards in explaining things.

    I'd explain this by starting with what we do, something like: "Allow to use the uri widget for the file_uri field type, which uses it as the default widget".

    The strictly required sentence doesn't seem relevant here. I'm not even sure we even need a comment at all or mention the default widget to be honest. The main purpose is to allow the uri widget on the file type, which is almost self-explaining from that line IMHO, anything else is an after-thought.

  2. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
    @@ -0,0 +1,40 @@
    +      ->setSetting('case_sensitive', $field_definition->getSetting('case_sensitive'))
    

    does it really make sense to support the case sensitive setting on this? That is relevant for storage only IMHO and implies that it can be set on the field where it 100% depends on the uri and whatever processing is going to happen during file_create_url()

  3. +++ b/core/modules/file/tests/src/Kernel/FileUriItemTest.php
    @@ -0,0 +1,40 @@
    +    $expected_url = base_path() . $this->siteDirectory . '/files/druplicon.txt';
    

    see https://www.drupal.org/node/2881999 for discussion on whether we should or not should not use base_path(), also https://www.drupal.org/node/2529170().

    maybe we should just use file_transform_relative(file_create_url()) here? We're not testing that those functions are working correctly, just that the url property is defined and correctly calls those functions?

  4. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -49,8 +62,13 @@ public function __construct(EntityManagerInterface $entity_manager, ClientInterf
    +    if ($this->halSettings->get('bc_file_uri_as_url_normalizer')) {
    +      // Replace the file url with a full url for the file.
    +      $data['uri'][0]['value'] = $this->getEntityUri($entity);
    +    }
    

    interesting, makes sense as BC.

    There's the related problem with the top-level url property, which is currently working through the File::url() and the implementation of \Drupal\hal\Normalizer\ContentEntityNormalizer::getEntityUri() that calls url() even when there is no canonical link template (there is no non-deprecated implemention for this).

    I don't want to delay this, but I am wondering if we should change that behavior also here, with the same BC flag, so we don't need to introduce two separate flags those two things which are clearly related?

    It is possible that this is too much for this, just thinking out lout, because I would love to officially deprecate the behavior of File::url()

Wim Leers’s picture

Thanks for the detailed review — looks like exactly the kind of feedback this sorely needed! 👍❤️

(crosspost with #146, I reviewed the previous patch but I don't think the relevant parts here changed)

Indeed, #144+#145+#146 are trivial changes only.

I'll let @damiankloip address your review, he's the lead on this issue :)

The last submitted patch, 144: 2825487-144.patch, failed testing. View results

The last submitted patch, 145: 2825487-145.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 146: 2825487-146.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
23.26 KB

I added #144 to the wrong class. 😅

(Also: I introduced 1 coding standards violation in #146. Fixing that too.)

Should be green now.

Status: Needs review » Needs work

The last submitted patch, 152: 2825487-152.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review

#152 failed due to DrupalCI infra problems: Warning: apcu_store(): Unable to allocate memory for pool.

Retesting.

tedbow’s picture

Wow, this is looking great! Looked it over and just a couple things.

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
    @@ -101,6 +112,40 @@ protected function getExpectedCacheContexts() {
    +    $expected += [
    +      'field_rest_test_multivalue' => [
    

    Trying to understand this test it is not clear when you need to change the expected value for this field. Maybe we could have comment here. I see this code block is used 3x in \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase but with a comment each time. Could this be a method on EntityResourceTestBase? Maybe the method name would make it clearer why it is needed.

  2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
    @@ -101,6 +112,40 @@ protected function getExpectedCacheContexts() {
    +      ]
    

    Nit: Missing comma

damiankloip’s picture

Berdir thanks for the review - very much appreciated!

#147.1: Agree, it could be much more concise - changed to your suggestion,
#147.2: Good point, removed. It only makes sense for the actual 'value' for the field that is actually stored.
#147.3: This is actually how the patch originally did it I think :) Changed back. I preferred this way too, as you say - we're not testing the creation of the actual url as such.
#147.4: I think I understood what you meant with this... I have overridden the getEntityUri() method for the FileEntityNormalizer in the patch. This means we can return either the full or relative URL in both places (top level and the field 'value'). We can then deal with the url() method replacement in another issue. Is this what you had in mind?

#155.1: Tedbow, that's a good question. I will defer this one to WimLeers!
#155.2: Comma added.

Also, speaking to Wim about this earlier; It doesn't seem like there is much need for an actual upgrade path test. The REST tests are testing the actual behaviour of this BC flag. So all the upgrade path test would need to test is that a config value was set. Doesn't seem overly useful.

Berdir’s picture

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -85,4 +86,15 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
+   * {@inheritdoc}
+   */
+  protected function getEntityUri(EntityInterface $entity) {
+    if ($this->halSettings->get('bc_file_uri_as_url_normalizer')) {
+      return $entity->url('canonical', []);
+    }
+
+    return file_url_transform_relative(file_create_url($entity->getFileUri()));
+  }

That's one approach yes. I was wondering if we'd want to update the url() implementation based on that setting but it seems a bit weird that it would depend on a hal.module specific setting.

And it's already deprecated, so we can possibly just leave it until 9.x. Wondering that that means for #2402533: No link template "canonical" found for the "file" entity type, which currently tries to replicate the url() implementation also in toUrl(). Maybe it actually shouldn't...

I still think that what we are doing here is a bit strange, especially when considering file_entity, which registers an actual canonical template at file/{file}, so this change would kind of break that again (currently it re-implements url() to fix other things still relying on the deprecated function).

In short, I'm not sure what the correct entity URI for file entities is, now that we have the file URL explicitly defined.

I'd expect, especially in a generic implementation, that the self href points to the page with the information about the file, not the actual binary file data.

That said, I also know that this was added so that the file URL is available when e.g. fetching a node with file/image fields. If we'd stop doing this, then it would always require an additional, explicit GET request to get that information. Maybe FileItem should *also* have such a computed property? (we are adding image style urls to ImageItem, so it would make sense to have a url for FileItem, which images would inherit for the original image)). But that either requires a second BC setting or we'd have to do it at once or at least also in 8.5.x.

Just thinking out loud, interested in what others are thinking about this.

kylebrowning’s picture

That said, I also know that this was added so that the file URL is available when e.g. fetching a node with file/image fields. If we'd stop doing this, then it would always require an additional, explicit GET request to get that information.

This alone seems worth the effort to ensure devices/clients do not have to do this. But maybe GraphQL solves that for us down the road?

I'd personally rather another computed property so that Out of the box, were best in class.

Status: Needs review » Needs work

The last submitted patch, 156: 2825487-156.patch, failed testing. View results

Wim Leers’s picture

#155:

Wow, this is looking great!

Glad you agree!

  1. That is an unfortunate complication introduced by the improved test coverage of #2800873: Add XML GET REST test coverage, work around XML encoder quirks. Since then, every fieldable entity type automatically gets a multi-value field added, and it's then tested.
    I created #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations to make that problem go away. Please help review that so we can simplify this patch even before this goes in.
  2. 😭
Wim Leers’s picture

(Partial review of #156, because I need to run for 🏓!)

+++ b/core/modules/file/tests/src/Kernel/FileUriItemTest.php
@@ -33,7 +33,7 @@ public function testCustomFileUriField() {
-    $expected_url = base_path() . $this->siteDirectory . '/files/druplicon.txt';
+    $expected_url = file_transform_relative(file_create_url($uri));

+++ b/core/modules/file/tests/src/Unit/ComputedFileUrlTest.php
@@ -39,7 +39,7 @@ public function testGetValue() {
-    $expected = base_path() . $this->siteDirectory . '/files/druplicon.txt';
+    $expected = file_transform_relative(file_create_url($this->testUrl));

We should revert this change.

I see you did it in response to #147.3.

See my reasoning in #128.10.

Wim Leers’s picture

#156:

So all the upgrade path test would need to test is that a config value was set. Doesn't seem overly useful.

Removed tags, updated IS.

#158: Cool to see you chime in again :) Thanks!


#147:

It is possible that this is too much for this, just thinking out lout, because I would love to officially deprecate the behavior of File::url()

Yes please! 😭 👍

#157

And it's already deprecated, so we can possibly just leave it until 9.x. Wondering that that means for #2402533: No link template "canonical" found for the "file" entity type, which currently tries to replicate the url() implementation also in toUrl(). Maybe it actually shouldn't...

Right, while I'd love to deal with this (see my reaction above), I think perhaps we should do that in #2402533: No link template "canonical" found for the "file" entity type. Because this issue really only deals with normalizers & field properties, not with entity implementations, and it seems #2402533 was created specifically for that?
Basically: I think that merits a discussion on its own?

I'd expect, especially in a generic implementation, that the self href points to the page with the information about the file, not the actual binary file data.

Completely agreed! For that, we have #2402533.

In short, I'm not sure what the correct entity URI for file entities is, now that we have the file URL explicitly defined.

Posted a proposal at #2402533-81: No link template "canonical" found for the "file" entity type.

Maybe FileItem should *also* have such a computed property? (we are adding image style urls to ImageItem, so it would make sense to have a url for FileItem, which images would inherit for the original image)).

Indeed! I descoped file fields (FileItem) in #128.16, to make this issue more focused and actionable, and deferred FileItem to #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization).

It's only if we pull the fixing of File::url() into this issue that we need to deal with file fields too. First doing #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization) will provide "the new correct way" of getting a file's URL. After that is done, we can do #2402533: No link template "canonical" found for the "file" entity type, which for existing sites would end up with 'field_file' => […, 'url' => 'http://example.com/llama.jpg', 'file_url' => 'http://example.com/llama.jpg'] and for new sites would end up with 'field_file' => […, 'url' => 'http://example.com/entity/file?_format=hal_json', 'file_url' => 'http://example.com/llama.jpg'] — i.e. with another BC flag to not break existing sites.

So, my proposal for a plan of attack:

  1. keeping this issue scoped to fixing normalization of File entities, which it does by making the $file->uri field contain not only a value property (containing public://llama.jpgcode>), but also a computed <code>url property (containing /sites/default/files/llama.jpg)
  2. then doing #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization), to improve the normalization of FileItem fields, which it would do by adding a computed file_url property (behaving identical to $file->uri->url
  3. then doing #2402533: No link template "canonical" found for the "file" entity type, to get rid of the weirdness in File::url() (or at least explicitly deprecate it), with a BC flag that allows you to remove that completely, and which would also result in the File entity's HAL normalization to contain a different value for the _self link, as well as resulting in the FileItem field's normalization's url key-value pair to contain a different value
+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -85,4 +86,15 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
+  protected function getEntityUri(EntityInterface $entity) {
+    if ($this->halSettings->get('bc_file_uri_as_url_normalizer')) {
+      return $entity->url('canonical', []);
+    }
+
+    return file_url_transform_relative(file_create_url($entity->getFileUri()));
+  }

+++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
@@ -34,15 +34,17 @@ class FileHalJsonAnonTest extends FileResourceTestBase {
-    if ($this->config('hal.settings')->get('bc_file_uri_as_url_normalizer') === TRUE) {
+    if ($use_url_bc) {

@@ -67,7 +69,7 @@ protected function getExpectedNormalizedEntity() {
-          'href' => $url,
+          'href' => $use_url_bc ? $url : file_url_transform_relative($url),

I think this this should be reverted, it is out of scope here, it's in scope of #2402533: No link template "canonical" found for the "file" entity type.

Berdir’s picture

I didn't know about #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization), makes sense to split that out. And you mentioned #2402533: No link template "canonical" found for the "file" entity type where you basically approach what the latest patch does from the opposite direction, by keeping BC for file entities. As we I think all agree here, that BC should be temporary and be disable-able* :)

In general I prefer separate issues and smaller changes as well, my only argument against that, as already mentioned, is that if we don't manage to resolve all those issues in 8.5.x, then we will introduce changes over multiple minor versions, including multiple settings, so clients will need to be updated several times if they want to remove their dependency on BC behavior.

I would propose a slight change to that plan, however:

1. Do this, keep out the self href change.
2. Do #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization), so we have a neat replacement for that in the context of nodes
3. Deprecate the current behavior of the href property with the approach in #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization), so we do *not* need to actually change File::url()

And then do our best to get those things done by 8.5.0 so we can control it all with a single BC setting.

And separately from that, offer an explicit API replacement for $file->url(), maybe $file->getFileUrl(), like I suggested above.

Because then we could simply close #2402533: No link template "canonical" found for the "file" entity type as won't fix and simply remove the deprecated function implementation in 9.x. Or we could do the getFileUrl() thing in that issue.

* Thinking about the concept of hiding behavior changes behind settings that we automatically set differently during update than new installations works for specific existing sites with specific REST clients, but it would still be a problem for e.g. a generic drupal something client that needs to work with both existing and new installations, but I guess those will need to decide which version of the setting it wants to support.

Berdir’s picture

And in response to #161.

I see you did it in response to #147.3.

See my reasoning in #128.10.

I don't feel strongly about it, but I'm not convinced by your argument in #128.10. As I said in my comment, we are not interested in testing how exactly the file URL is built, we know that file_url_transform_relative(file_create_url($entity->getFileUri())); is the proper API to get the file URL and that it is what we want to get from this property. So IMHO doing that in the test is perfectly valid. And as mentioned, I'd like to avoid adding more usages of base_path() which I'm sure we'd like to deprecate eventually (the same can be said about the file functions, true..).

damiankloip’s picture

Status: Needs work » Needs review
FileSize
23.06 KB
2.16 KB

I like the approach outlined in #162 and revised in #163, I would really prefer to keep this issues focused on the file normalization, also avoiding any changes to File::url(). Once we have this in, I think we will have some good momentum and understanding to get #2925520: Add a computed 'url' property to FileItem (for exposing file URL in file field normalization) done in good time.

I have left the file_url_transform_relative(file_create_url()) changes in for now until that is resolved. I am honestly fine with either way, unless there is a strong wish to move it back (happy to do so, just letting the discussion on that run its course). This is how it worked originally too.

Here is an updated patch with the previous href link changes removed.

Status: Needs review » Needs work

The last submitted patch, 165: 2825487-165.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

#163

makes sense to split that out.

🎉

In general I prefer separate issues and smaller changes as well, my only argument against that, as already mentioned, is that if we don't manage to resolve all those issues in 8.5.x, then we will introduce changes over multiple minor versions, including multiple settings, so clients will need to be updated several times if they want to remove their dependency on BC behavior.

  1. First: I totally agree we should minimize pain. But we also need to take into account issue/commit complexity. This is much less likely to land if we make it one big patch. Especially because these are so tricky issues to untangle.
  2. Second: this issue would provide a flag to change the normalization of File entities, #2402533 would provide a flag to change the behavior of File::url(). It's totally possible to have code that is only affected by one of the two! It also makes the flags easier to explain.
  3. First + Second = I think it's okay to have both of them have their own flags.

I would propose a slight change to that plan, however: [1 = identical, 2 = identical], [3 = so we do *not* need to actually change File::url()]

That sounds fine too: effectively just let the already-deprecated File::url() continue to be deprecated, right?

And then do our best to get those things done by 8.5.0 so we can control it all with a single BC setting.

I don't see how we can achieve this. Unless you mean that if we do all this before 8.5.0-beta, that we are allowed to merge multiple BC flags into a single flag? That is … very interesting + clever 😀 Never been done before, but definitely worth a try! I like it. We'll see if a single BC flag really makes sense in the end, but totally +1 on making that the goal.

AWESOME! Sounds like we have consensus 🎉 😀 🎉

And separately from that, offer an explicit API replacement for $file->url(), maybe $file->getFileUrl(), like I suggested above.
Because then we could simply close #2402533 as won't fix […]

+1. Or introduce a 'download' link relation type. No need to reach consensus about that here though.

And as mentioned, I'd like to avoid adding more usages of base_path() which I'm sure we'd like to deprecate eventually (the same can be said about the file functions, true..).

Exactly. Both base_path() and file_create_url() are problematic: the first relies on global state, the second relies on the container (and therefore also on global state). Neither works in unit tests, both need special massaging. So all other things being equals, I think we should pick the path of least resistance, which is base_path() today.


#165

I have left the file_url_transform_relative(file_create_url()) changes in for now

See last thing I wrote above. The current patch is failing simply because it's using this undefined function. Insisting on calling that function will add the need to mock it. Is that really better than using base_path()? I think not.

Wim Leers’s picture

Issue summary: View changes

Fix typo in IS.

damiankloip’s picture

If we change back to Wim's suggestion, all the fails in #165 will be fixed... :)

damiankloip’s picture

Status: Needs work » Needs review
FileSize
23.1 KB
2.12 KB
Wim Leers’s picture

A week without a comment. I'd love to have Berdir RTBC this patch, because he's A) an expert in all of this, B) he's the maintainer of https://www.drupal.org/project/file_entity.

Until we get that review from him, here's a "final review" from me. I only found two nits:

  1. +++ b/core/modules/hal/config/install/hal.settings.yml
    @@ -1,3 +1,4 @@
    +bc_file_uri_as_url_normalizer: false
    

    Nit: This still needs a comment similar to those in serialization.settings.yml.

  2. +++ b/core/modules/hal/hal.install
    @@ -31,3 +31,15 @@ function hal_update_8301() {
    +  return t('The REST API will no longer return the full URL for file entity URI values for HAL+JSON responses. It will return the URI value itself, with an additional URL property providing a root relative file path. If your site depends on these value being strings, <a href="https://www.drupal.org/node/2925783">read the change record to learn how to enable the BC mode.</a>');
    

    Nit: This message is incorrect. Nothing will change for existing sites! So this shouldn't return any string.

  3. --- a/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
    +++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/File/FileResourceTestBase.php
    @@ -154,6 +154,7 @@ protected function getExpectedNormalizedEntity() {
    +          'url' => base_path() . $this->siteDirectory . '/files/drupal.txt',
    

    👍 We're testing both the new case and the BC case in this updated test coverage.

  4. --- a/core/modules/hal/tests/src/Kernel/FileNormalizeTest.php
    +++ b/core/modules/hal/tests/src/Kernel/FileNormalizeTest.php
    

    👍 We're testing only the new case in the kernel test.

Berdir’s picture

Status: Needs review » Needs work

Looks fine to me beside the comments above.

In regards to 2, I guess what we could do is point out that the site is now using the BC mode and point to docs on how to disable that? All sites will eventually need to do that before they can update to 9.x*

* That gave me an idea: Should we create a follow-up issue to have a requirements hook/reports page that displays a warning for each enabled BC setting, and maybe the ability to change them, or something like that?

Wim Leers’s picture

In regards to 2, I guess what we could do is point out that the site is now using the BC mode and point to docs on how to disable that? All sites will eventually need to do that before they can update to 9.x*

Genius! 👌 Yes! Point to the CR!

Should we create a follow-up issue to have a requirements hook/reports page that displays a warning for each enabled BC setting, and maybe the ability to change them, or something like that?

I like the idea, but … that's eventually going to take over the entire status report :P

Alright, so @damiankloip still needs to address #171.1 and #171.2. And for #171.2, he should take #172 into account. Then this will be RTBC :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
22.98 KB

#2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations is in, rebased for that. Which means this is now gone:

+++ b/core/modules/hal/tests/src/Functional/EntityResource/File/FileHalJsonAnonTest.php
@@ -100,6 +112,40 @@ protected function getExpectedCacheContexts() {
+    $expected += [
+      'field_rest_test_multivalue' => [
+        0 => [
+          'value' => 'One',
+        ],
+        1 => [
+          'value' => 'Two',
+        ],
+      ],
+    ];

(Also had to resolve a conflict for #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes.)

Still leaving assigned to @damiankloip for

Alright, so @damiankloip still needs to address #171.1 and #171.2. And for #171.2, he should take #172 into account. Then this will be RTBC :)

damiankloip’s picture

Thanks Wim and Berdir! Great to have this in a good place now. Here is a patch with those comment changes/additions.

Really good to see #2928702: Make EntityResourceTestBase's field_rest_test_multivalue test field less invasive: omit it from normalizations in too, so we can simpify this patch further!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, lets do this.

Wim Leers’s picture

OMG OMG OMG OMG THIS IS RTBC!!! 😲🎉🍾🥂

This is the very last blocker to #1927648: Allow creation of file entities from binary data via REST requests. It'd be wonderful to ship Drupal 8.5 with the ability to upload files at last!

Wim Leers’s picture

I reviewed the patch in detail (to try to pre-emptively address nits that committers will raise), and also reviewed the change record in detail.

Updated both.

CR
The CR was inaccurate because it was started from the "bools/ints as strings" normalizer CR, which also changed things for existing sites. This issue/patch is not changing things for existing sites.
Patch
I only found docs nitpicks, which I all fixed.
  1. +++ b/core/modules/hal/config/install/hal.settings.yml
    @@ -1,3 +1,8 @@
    +# Before Drupal 8.5, the file URI field value was overridden to only return the
    +# full file URL. The default for new sites is now to return the actual URI value
    +# as well as a root relative file path. Enable this setting to use the previous
    +# behavior.
    +bc_file_uri_as_url_normalizer: false
    

    s/URI field/'uri' field/
    s/full file URL/absolute file URL/
    s/root relative/root-relative/

    Missing information on behavior for existing sites.

    Missing @see to update function and CR.

  2. +++ b/core/modules/hal/config/schema/hal.schema.yml
    @@ -6,3 +6,6 @@ hal.settings:
    +      label: 'Whether to retain pre Drupal 8.5 behavior of normalizing file URI values as a full URL.'
    

    Similar clarifications.

  3. +++ b/core/modules/hal/hal.install
    @@ -31,3 +31,15 @@ function hal_update_8301() {
    +  return t('BC Mode has been enabled for file URI normalization. Only the full file URL will be returned <a href="https://www.drupal.org/node/2925783">read the change record for more information about opting out of this behavior.</a>');
    

    Not all those who see "BC" appear while updating their site will know what it means. We should spell it in full.

    Plus similar remarks.

  4. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
    @@ -26,6 +30,13 @@ class FileEntityNormalizer extends ContentEntityNormalizer {
    +   * The hal settings config.
    

    s/hal/HAL/

  5. +++ b/core/modules/hal/tests/src/Functional/FileDenormalizeTest.php
    @@ -20,6 +20,20 @@ class FileDenormalizeTest extends BrowserTestBase {
    +    // Override the default configuration to the hal BC setting is enabled, to
    +    // return the full URL value as the file URI 'value'.
    

    "override the … to the … is enabled" needs improving :)

    Can just be replaced by @see hal_update_8501().

    Also, this entire test will be removed in https://www.drupal.org/node/1927648, so this is just a temporary: should be clarified too.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 178: 2825487-178.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Silly testbot. Failing a patch that contains only docs changes.

cburschka’s picture

Should we get rid of the unused EntityInterface import in the FileEntityNormalizer class? :)

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/file/src/ComputedFileUrl.php
@@ -0,0 +1,47 @@
+    $this->url = file_url_transform_relative(file_create_url($uri));
...
+++ b/core/modules/file/src/Plugin/Field/FieldType/FileUriItem.php
@@ -0,0 +1,39 @@
+    $properties['url'] = DataDefinition::create('uri')

This doesn't seem like it should be valid. The 'uri' type should be validated to have a scheme (see PrimitiveTypeConstraintValidator::validate()). Is there a reason why tests are passing without this triggering validation errors? Do we not validate computed properties?

Berdir’s picture

Lets just make it a string then?

Not sure if computed properties are explicitly validated or not, but right now in HEAD, nothing is going to validate them except kernel tests of specific things as there are no file entity forms in core.

cburschka’s picture

Apparently not - the validator just iterates through the items to be validated, Map::getIterator() calls Map::getProperties() without an argument, and the computed properties are only included with Map::getProperties(TRUE). So computed properties are not validated.

Then yeah, maybe it should be a string. (I'm not sure why it isn't an absolute URL anyway, but there's probably a good reason.)

(While testing the above, I also noticed that ComputedFileUrlTest is namespaced in Unit even though it's a kernel test. Seems to be a remnant of #121.)

cburschka’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
857 bytes
23.37 KB

#181: nice catch! That was one the sole coding standard fail, 👍

#185: Another nice catch!


#182 + #183 + #184: Indeed, computed fields are never validated because they're not stored. I applied Berdir's suggestion.

effulgentsia’s picture

Adding reviewer credit. Thanks everyone for all the great reviews!

  • effulgentsia committed be64202 on 8.5.x
    Issue #2825487 by damiankloip, Wim Leers, garphy, cburschka, tedbow,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.5.x and published the CR.

Wim Leers’s picture

🎉🎉🎉

Wim Leers’s picture

Wim Leers’s picture

This also helps unblock JSON API: #2926463-7: [PP-1] Deprecate JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it, which has its own work-around that it had to invent because this issue hadn't been solved yet. (That is in turn blocked on another JSON API infrastructure issue: #2921257-22: On Drupal 8.5, JSON API should respect Typed Data's new ::isInternal(), and JSON API Extras should use ::setInternal(), which this also unblocks.)

Progress for the entire API-First ecosystem!

Wim Leers’s picture

Issue tags: +8.5.0 highlights

More than a year of working on building consensus both in this issue and in its blockers to add missing API infrastructure. I think this belongs in the release notes, and even highlights.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.