Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Blocked on #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contextsBlocked on #2910211: Allow computed exposed properties in ComplexData to support cacheability.Blocked on #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handlerFix normalization of\Drupal\file\Entity\File
by adding a computedurl
property to aFile
'suri
field. Then thevalue
property will contain the stored value (public://lama.jpg
), andurl
will contain the computed root-relative file URL (/sites/default/files/llama.jpg
).
Remaining tasks
BC layer.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
- A new
url
computed property is added to theFile
entity'suri
field. - This causes all normalizations/formats (
json
,xml
,hal_json
, JSON API and GraphQL in contrib) to have that newurl
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
). - The HAL normalization of
$file->uri
on existing sites continues to behave in the strange way it does in HEAD: thevalue
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
). - 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. - Existing sites can opt in to the new behavior by setting the
bc_file_uri_as_url_normalizer
key inhal.settings
tofalse
.
Data model changes
A new url
computed property is added to the File
entity's uri
field.
Comment | File | Size | Author |
---|---|---|---|
#186 | 2825487-186.patch | 23.37 KB | Wim Leers |
#186 | interdiff.txt | 857 bytes | Wim Leers |
#185 | 2825487-185-interdiff.txt | 648 bytes | cburschka |
#185 | 2825487-185.patch | 23.26 KB | cburschka |
#181 | 2825487-181-interdiff.txt | 581 bytes | cburschka |
Comments
Comment #2
Wim Leers#2793809: [FEATURE] Provide direct download URLs for files and images was the relevant JSON API issue.
Comment #3
Berdirimage 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.
Comment #4
garphy CreditAttribution: garphy at ICI LA LUNE commentedShould 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.
Comment #5
BerdirIt'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...
Comment #6
tstoeckler@Berdir: For JSON API they expose all possible image style URLs, which admittedly is a bit verbose, but still very useful for frontends.
Comment #7
dawehnerWe 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.Comment #8
Wim LeersYou'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!
Comment #9
tstoeckler;-)
Comment #10
Wim LeersThanks :)
Comment #11
dawehnerI agree
Comment #12
Wim Leersgarphy 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 offImageItem
to a separate issue altogether. BesidesImageItem extends FileItem
, so this will already helpImageItem
— it's just thatImageItem
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
.FieldItemNormalizer
orNormalizerBase
because this is not something that needs to be denormalized: the file URL really is a computed fieldNormalizerInterface
. 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 theprocessed
property of aTextItem
also happens to be computed :)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:Doing it correctly like this ensures that it will actually also work in JSON API and even in GraphQL.
file_url
, and its computed value would be a root-relative file URL: it should use thefile_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.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:
file_url
computed property toFileItem
ImageItemNormalizer extends EntityReferenceFieldItemNormalizer
, with something like this:Comment #13
garphy CreditAttribution: garphy at ICI LA LUNE commentedComment #14
Wim LeersThanks for taking this on, garphy!
Comment #15
garphy CreditAttribution: garphy at ICI LA LUNE commentedI'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 ?
Comment #16
garphy CreditAttribution: garphy at ICI LA LUNE commentedI'm still eager to tackle this actually :)
Comment #17
Wim Leers#15: Isn't what you're getting a
public://foobar.png
URL?Comment #18
BerdirNo, 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: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation is doing.
Comment #19
Wim LeersThat's not what I'm seeing:
i.e.:
"uri":[{"value":"public:\/\/2016-11\/apcu_status_report--apcu_enabled_default.png"}]
Comment #20
BerdirThat'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.
Comment #21
garphy CreditAttribution: garphy at ICI LA LUNE commentedSo 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.
Comment #22
BerdirI 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.
Comment #23
Wim LeersUGH 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:\Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer::normalize()
does this:\Drupal\file\Entity\File::url()
does this:So, rescoping this issue.
#21:
That's out of scope here, and we must fix the issues mentioned in #18 to fix that.
Yes, rescoped this issue to that. We need an entity-level normalizer for that.
Comment #24
Wim LeersComment #25
garphy CreditAttribution: garphy at ICI LA LUNE commentedPotential gotcha :
Comment #26
Wim LeersThanks 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.
Comment #27
BerdirI 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
Comment #28
Wim LeersNote that the JSON API contrib module currently is also adding a work-around for this:
\Drupal\jsonapi\Plugin\FileDownloadUrl
classComment #29
garphy CreditAttribution: garphy at ICI LA LUNE commentedI 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.
Comment #31
tedbowI 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
Comment #33
garphy CreditAttribution: garphy at ICI LA LUNE commentedI 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.
Comment #34
garphy CreditAttribution: garphy at ICI LA LUNE commentedBetter title for the issue.
We could also use a summary update.
Comment #36
Wim Leers+1
Perhaps "public URL" would be better than "download URL"?
I'm NOT saying to reroll the patch and change this. I'm asking :)
What is actually causing this to work? How is this populated?
You forgot
file_url_transform_relative()
.Comment #37
Wim LeersComment #38
BerdirI'm not really sure about this approach. Can't we do this as an operation/link or something like that?
Comment #39
Wim LeersA 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 likedownload
) would not work AFAICT: how would->access('some new operation')
actually be able to add a URL to the normalization?Comment #40
BerdirI'm not sure what I meant with operation exactly but definitely not access :) That said, the download access operation actually exists.
Comment #41
Wim LeersIndeed 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
Comment #42
e0ipsoI 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).
Comment #43
garphy CreditAttribution: garphy at ICI LA LUNE commentedWhat would be the JSON output of a link-based approach ?
Is there any example of this approach I can study to rework the patch ?
Comment #44
tedbow@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.
Comment #45
garphy CreditAttribution: garphy at ICI LA LUNE commentedI'll take a look.
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.
Comment #46
garphy CreditAttribution: garphy at ICI LA LUNE commented@elipso regarding #42,
What are those false expectations ?
Comment #47
tedbowOk 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:
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.
Comment #48
tedbowI 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'.
Comment #49
Wim LeersI asked Berdir to review this. I think he's the person best able to choose between #47 and #48.
Comment #50
garphy CreditAttribution: garphy at ICI LA LUNE commentedI 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.
Comment #51
Wim LeersSo this issue still needs test coverage. I think this would need a unit test, much like
\Drupal\Tests\serialization\Unit\Normalizer\PrimitiveDataNormalizerTest
.Comment #52
dpovshed CreditAttribution: dpovshed as a volunteer commentedI'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.
Comment #53
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedRerolled the #48 patch.
Comment #54
Wim Leers@dpovshed Can you please assign the issue to yourself? :)
Comment #55
dpovshed CreditAttribution: dpovshed as a volunteer and at Drupal Ukraine Community commented@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
Comment #56
Wim Leers@dpovshed++
Comment #57
dpovshed CreditAttribution: dpovshed as a volunteer and at Drupal Ukraine Community commentedThe 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:
@wim, could you please ping someone who can help us here :)
Comment #58
dpovshed CreditAttribution: dpovshed as a volunteer commentedand interdiff is follows, of course :)
Comment #60
Wim LeersI pinged @tstoeckler :) https://twitter.com/wimleers/status/845064065059827712
Comment #61
tstoecklerThis 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.
Comment #62
dpovshed CreditAttribution: dpovshed as a volunteer and at Drupal Ukraine Community commentedTobias, thank you, I like very much what I see! Now we need independent reviewer here?
Comment #63
Wim LeersThanks!
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.
This is a c/p remnant from
serializer.normalizer.entity_reference_field_item
. Needs to be updated.This should be wrapped in a
file_url_transform_relative()
call.Comment #64
damiankloip CreditAttribution: damiankloip at Acquia commentedCouple o' comments:
Adds a ..
Do we need a hal equivalent for this too to keep parity?
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.
Comment #65
Wim LeersThat'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 theUriItem
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 toFile
entities specifically), rather than talking about aFile
entity'suri
field, which indeed usesUriItem
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 formailto: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
to:
And then adding
FileUriItem
? It'd only allowhttp://
,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 (usingfile_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 theiruri
field (the only constraint is that they must be unique).Comment #66
tstoecklerI 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.
Comment #67
Wim LeersThis issue is currently fixing the normalization of
\Drupal\file\Entity\File
, i.e. theFile
entity. But we also need this issue to fix the normalization of\Drupal\file\Plugin\Field\FieldType\FileItem
, i.e. theFile
entity reference field.The IS even says this:
For
ImageItemNormalizer
, we now have #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs. Updated IS.Comment #68
Wim LeersComment #69
Wim Leers#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
This code only makes sense for
UriItem
s onFile
entities! ButUriItem
can be used anywhere!In fact, the following use it:
Feed
'surl
,link
andimage
base fieldsItem
'slink
base fieldComment
'shomepage
base fieldLink
'suri
base fieldAll of those would be negatively impacted by this.
Therefore I think it's less disruptive to update
like this;
instead of like this:
… because the current
uri
base field onFile
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 onFile
entities would be the least disruptive approach: it would allow us to add aFileUriItemNormalizer
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.Comment #70
Wim LeersClosed #2394063: Field properties are not normalized or denormalized on entity reference items. as a duplicate.
Comment #71
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is a start on this, not sure if this is the best way to handle to computed property. Still very rough
Comment #72
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #73
dawehnerI just tried out to manually set the "file URL" and nothing complained about it, it just allowed to change the value:
I'm wondering whether we can / should do anything about it?
Comment #74
e0ipsoOne 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:
I'm wondering if we face a similar situation here.
Comment #75
dawehnerI'd like to test the latest patch ...
Comment #76
Wim LeersThis 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:
… that's exactly what this does! So this can merrily continue :)
Comment #77
BerdirI'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.
The file entity actually has a getFileUri() method that we could use wherever this code will then exactly live.
Comment #78
Wim LeersThis is now also a hard blocker for #2895573: Decide on normalization of Media module entities.
Comment #79
Wim LeersThanks for the feedback, @Berdir, that's greatly appreciated!
I like that!
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
! 👏Comment #81
Wim Leers#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!
Comment #82
damiankloip CreditAttribution: damiankloip at Acquia commentedOK, so are we thinking something like this? Before I sink too much time into it... (that's a rough patch disclaimer btw ;) )
Comment #83
garphy CreditAttribution: garphy at ICI LA LUNE commented@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.
Comment #84
Wim LeersAFAICT that'd result in a normalization like this:
Looks great to me!
No, see this bit in the patch:
Comment #85
garphy CreditAttribution: garphy at ICI LA LUNE commentedYeah, 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.
Comment #86
garphy CreditAttribution: garphy at ICI LA LUNE commentedWell... it seems to not be working atm, I still get only "value" sub property for the "uri" property :
Let's see if I can craft a test case for this one.
Comment #87
garphy CreditAttribution: garphy at ICI LA LUNE commentedOk, I think what's missing is a
setInternal(FALSE)
because ofsetComputed(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()
.Comment #88
garphy CreditAttribution: garphy at ICI LA LUNE commented...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.
Comment #89
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, 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.
Comment #90
garphy CreditAttribution: garphy at ICI LA LUNE commentedYou're right. I should have posted a do-not-test patch.
Comment #91
damiankloip CreditAttribution: damiankloip at Acquia commentedThat's exactly the inderdiff we'll want to apply when #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in though :)
Comment #93
Wim LeersActually, 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.
Comment #94
Wim LeersOh and this blocks #1927648: Allow creation of file entities from binary data via REST requests, which is major, meaning this should be major too.
Comment #95
damiankloip CreditAttribution: damiankloip at Acquia commentedYes - 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.
Comment #96
Wim Leers👍
Comment #97
damiankloip CreditAttribution: damiankloip at Acquia commentedHere is a unit test for
\Drupal\file\FileUrl
and a kernel test for using the actual overriddenFileUriItem
with the overridden 'url' property definition that's added.Testing the tests.
Comment #98
dawehnerurl()
is deprecated, it says you should usetoUrl
instead. Any reason not to?Comment #99
damiankloip CreditAttribution: damiankloip at Acquia commentedNot really - lets make things even more verbose :D
Comment #101
damiankloip CreditAttribution: damiankloip at Acquia commentedHmm, @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.
Comment #102
damiankloip CreditAttribution: damiankloip at Acquia commentedMaybe like.
Comment #104
Wim Leers#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in, #2910211: Allow computed exposed properties in ComplexData to support cacheability. is the last remaining blocker, and that too is now RTBC!
Comment #105
Wim Leers#97:
Yes: because
File::url()
is a special duck, and fixing it is a tricky issue of its own, see #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation. (That's also what #101 is alluding to.)So we should go back to #97.
Review of #97:
Why not do this instead:
?
This class would need a
@FieldType
annotation, if we do what I said in point 1. (UriItem
also has a@FieldType
annotation.)No, I think using the
uri
@DataType
plugin (i.e.\Drupal\Core\TypedData\Plugin\DataType\Uri)
) makes sense.Either:
CacheableDependencyInterface
and then have this return the'url.site'
cache context (becausefile_create_url()
returns an absolute URL for the current site in a multisite setup)file_create_url()
, but that offile_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.!I find this extremely confusing, but I see the same at
\Drupal\datetime\DateTimeComputed
and\Drupal\text\TextProcessed
.s/Url/URI/
Comment #106
damiankloip CreditAttribution: damiankloip at Acquia commentedAgreed, File is a special case, with a very much shoe-horned implementation of url() anyway. Related to this, berdir had a comment in #77 :
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.
Comment #108
damiankloip CreditAttribution: damiankloip at Acquia commenteddoh - forgot to change the actual URL code back. I based it on the wrong patch. rerolling.
Comment #109
Wim LeersI 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.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.)
+1
Right.
It basically only removes the
denormalize()
method.Right. I think the answer is pretty simple there: also remove the
normalize()
method.Comment #110
damiankloip CreditAttribution: damiankloip at Acquia commentedThis 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.
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.
Comment #112
damiankloip CreditAttribution: damiankloip at Acquia commentedNote: 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).
Comment #113
Wim LeersNit: s/File specific/File-specific/
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:Therefore it's fine to change the expectations in the Media tests.
Did that in the attached patch.
Comment #114
Wim Leers#112: Can't that be easily remedied with:
?
Comment #116
Wim LeersThis 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 , so issue metadata is already up-to-date.
Comment #117
damiankloip CreditAttribution: damiankloip at Acquia commented@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.
Comment #119
damiankloip CreditAttribution: damiankloip at Acquia commentedRelated 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.
Comment #121
damiankloip CreditAttribution: damiankloip at Acquia commentedFixes.
Getting back to one of @berdir's comments; do we want to add a new method to FileInterface for this? something like getRelativeUrl() ?
Comment #122
Wim LeersI'd agree, but … it doesn't show up in the UI, because:
That's in both in the existing
UriItem
and in the newFileUriItem
, so this is a non-issue :)👍
@Berdir said this in #77:
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 :)
Nit: s/File specific/File-specific/, s/Item/item/
Nit:
file_uri
Nit: Can be removed now. :)
Comment #123
damiankloip CreditAttribution: damiankloip at Acquia commentedFixed 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..
Comment #124
tstoecklerIn that case let's use
hook_field_(formatter|widget)_info_alter()
to add the file-specific field fromfile.module
.telephone_field_formatter_info_alter()
is the precedent for that.Comment #125
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, that's the approach I was thinking if we continue down this route too. tstoeckler++
Comment #126
Wim LeersRight. (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++
Comment #127
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, exactly WimLeers! It't not really 'needed' but.... :)
Comment #128
Wim LeersThis 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… 😳 😁
👍 We do this via an alter hook because the
uri
widget lives incore/lib/…
.👍 This is the crucial change. We change this to use the
file_uri
field type, which extends theuri
field type, and just adds a computed property.❓ Should this be called
ComputedFileUrl
?Nit: s/Cached URL/Computed root-relative file URL./
❓ Should we add an assertion to ensure we're dealing with
File
entities, because if we aren't, thengetFileUri()
won't be available?That'd mean adding this line first:
assert($this->getParent()->getEntity() instanceof FileInterface);
👍 It's okay to add
file_uri
directly here, instead of via an alter hook, because this formatter is owned by thefile
module.Suggested alternative: An entity field containing a file URI, and a computed root-relative file URL.
Suggested alternative: Root-relative file URL.
Nit: s/Custom URI/File URI/
Also: let's add
I think that in this case, it'd be clearer to not use the API functions to determine the expected value. But instead:
Then at least it's impossible for the expectation to change over time, and it's just a string assembled from parts.
Nit:
'test'
->$this->randomMachineName()
.Same remark here as two points earlier.
👍 Thorough!
Nit: "should explicitly should" needs fixing :P
❓ Shouldn't this still assert that the value was set, by calling
->getValue()
, just like we do intestSetValue()
?❓ Couldn't this be merged with
testSetValue()
and be updated to use a data provider?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 alwayspoint 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 'file_url' property to FileItem (for exposing file URL in file field normalization) for that.
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
andxml
(which uses the default normalization), as well ashal_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:
bc_file_uri_as_url_normalizer
key tohal.settings
, set tofalse
in default config, but set totrue
byhal_update_8501()
(similar tobc_timestamp_normalizer_unix
inserialization.settings
+serialization_update_8401()
)false
(the default),FileEntityNormalizer::normalize()
should do NOTHING. That means the HAL normalization (as well as the normalization for thejson
andxml
formats) will look like this:aka
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.true
,FileEntityNormalizer::normalize()
should do the same as todayaka
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).
👍 The changes here are okay, per #2895573: Decide on normalization of Media module entities and as described in #113.2.
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.
Comment #129
Wim LeersFixed inaccuracy in title.
Comment #130
damiankloip CreditAttribution: damiankloip at Acquia commentedNice 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?
Comment #131
damiankloip CreditAttribution: damiankloip at Acquia commentedOk, 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.
Comment #133
damiankloip CreditAttribution: damiankloip at Acquia commentedAh, 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.
Comment #134
Wim Leers#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?
That's not true, the new behavior was not always used by default.
Thoughts:
bc_primitives_as_strings
andbc_timestamp_normalizer_unix
, we added normalizer behavior. In this case, we're removing normalizer behavior.bc_primitives_as_strings: false
is the default, andserialization_update_8302()
also sets it tofalse
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, butserialization_update_8401()
sets it totrue
for existing sites. Because the normalization changes significantly, so consuming code would certainly break. IOW: sites must opt in.bc_file_uri_as_url_normalizer
), consuming code would also certainly break. IOW: sites must opt in.#131:
This is copy/pasted from
serialization_update_8302()
. But it's not the entire REST API that is affected, onlyHAL+JSON
responses.Let's add a
deprecated: "…"
key-value pair.See http://symfony.com/doc/current/service_container/alias_private.html#depr....
Wouldn't
$hal_settings
be better?s/facotry/factory/
This is missing
(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: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!Comment #135
damiankloip CreditAttribution: damiankloip at Acquia commentedNice! 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
Comment #137
damiankloip CreditAttribution: damiankloip at Acquia commentedWhoops, forgot to pass context.
Comment #139
Wim LeersWell, that's understandable, because that behavior is brand new!
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.
:) Now the only remaining thing is to add a
MediaResourceTestBase::getExpectedCacheTags()
, which can call the parent, but must add theconfig:hal.settings
cache tag. For an example, see\Drupal\Tests\hal\Functional\EntityResource\EntityTest\EntityTestHalJsonInternalPropertyNormalizerTest::getExpectedCacheTags()
.Comment #140
Wim LeersAll my remarks in #128 have been addressed, except for:
The two latter ones can already be done.
Comment #141
Wim LeersRelated: #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?
Comment #142
Wim LeersJust another small review while looking at this for #2922487: Follow-up for #2910211: fix all deprecation warnings, which is closely related.
Missing comment. Also, should we comply with #2919845: Create a 'bc' top level item in serialization.settings config object?
8.5.0
9.0.0
\Drupal\Core\Config\ImmutableConfig
Missing inheritdoc
Comment #143
Wim Leers#2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler landed! This is now fully unblocked: BC layer tests are now also possible.
Comment #144
Wim LeersHere's a rebase (with one conflict fixed) plus the trivial fix for the 3 failing
Media
entity type REST tests in #137.Comment #145
Wim LeersThis will make the tests for the "default" normalization (the one provided by
serialization.module
, for thejson
andxml
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.)
Comment #146
Wim LeersAnd 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!
Comment #147
Berdir(crosspost with #146, I reviewed the previous patch but I don't think the relevant parts here changed)
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.
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()
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?
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()
Comment #148
Wim LeersThanks for the detailed review — looks like exactly the kind of feedback this sorely needed! 👍❤️
Indeed, #144+#145+#146 are trivial changes only.
I'll let @damiankloip address your review, he's the lead on this issue :)
Comment #152
Wim LeersI added #144 to the wrong class. 😅
(Also: I introduced 1 coding standards violation in #146. Fixing that too.)
Should be green now.
Comment #154
Wim Leers#152 failed due to DrupalCI infra problems:
Warning: apcu_store(): Unable to allocate memory for pool.
Retesting.
Comment #155
tedbowWow, this is looking great! Looked it over and just a couple things.
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 onEntityResourceTestBase
? Maybe the method name would make it clearer why it is needed.Nit: Missing comma
Comment #156
damiankloip CreditAttribution: damiankloip at Acquia commentedBerdir 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.
Comment #157
BerdirThat'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: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation, 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.
Comment #158
kylebrowning CreditAttribution: kylebrowning commentedThis 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.
Comment #160
Wim Leers#155:
Glad you agree!
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.
Comment #161
Wim Leers(Partial review of #156, because I need to run for 🏓!)
We should revert this change.
I see you did it in response to #147.3.
See my reasoning in #128.10.
Comment #162
Wim Leers#156:
Removed tags, updated IS.
#158: Cool to see you chime in again :) Thanks!
#147:
Yes please! 😭 👍
#157
Right, while I'd love to deal with this (see my reaction above), I think perhaps we should do that in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation. 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?
Completely agreed! For that, we have #2402533.
Posted a proposal at #2402533-81: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation.
Indeed! I descoped file fields (
FileItem
) in #128.16, to make this issue more focused and actionable, and deferredFileItem
to #2925520: Add a computed 'file_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 'file_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: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation, 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:
File
entities, which it does by making the$file->uri
field contain not only avalue
property (containingpublic://llama.jpgcode>), but also a computed <code>url
property (containing/sites/default/files/llama.jpg
)FileItem
fields, which it would do by adding a computedfile_url
property (behaving identical to$file->uri->url
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 theFile
entity's HAL normalization to contain a different value for the_self
link, as well as resulting in theFileItem
field's normalization'surl
key-value pair to contain a different valueI think this this should be reverted, it is out of scope here, it's in scope of #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation.
Comment #163
BerdirI didn't know about #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization), makes sense to split that out. And you mentioned #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation 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 'file_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 'file_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: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation 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.
Comment #164
BerdirAnd in response to #161.
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..).
Comment #165
damiankloip CreditAttribution: damiankloip at Acquia commentedI 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 'file_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.
Comment #167
Wim Leers#163
🎉
File
entities, #2402533 would provide a flag to change the behavior ofFile::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.That sounds fine too: effectively just let the already-deprecated
File::url()
continue to be deprecated, right?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 🎉 😀 🎉
+1. Or introduce a
'download'
link relation type. No need to reach consensus about that here though.Exactly. Both
base_path()
andfile_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 isbase_path()
today.#165
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.Comment #168
Wim LeersFix typo in IS.
Comment #169
damiankloip CreditAttribution: damiankloip at Acquia commentedIf we change back to Wim's suggestion, all the fails in #165 will be fixed... :)
Comment #170
damiankloip CreditAttribution: damiankloip at Acquia commentedComment #171
Wim LeersA 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:
Nit: This still needs a comment similar to those in
serialization.settings.yml
.Nit: This message is incorrect. Nothing will change for existing sites! So this shouldn't return any string.
👍 We're testing both the new case and the BC case in this updated test coverage.
👍 We're testing only the new case in the kernel test.
Comment #172
BerdirLooks 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?
Comment #173
Wim LeersGenius! 👌 Yes! Point to the CR!
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 :)
Comment #174
Wim Leers#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:
(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
Comment #175
damiankloip CreditAttribution: damiankloip at Acquia commentedThanks 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!
Comment #176
BerdirOk, lets do this.
Comment #177
Wim LeersOMG 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!
Comment #178
Wim LeersI 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.
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.Similar clarifications.
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.
s/hal/HAL/
"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.
Comment #180
Wim LeersSilly testbot. Failing a patch that contains only docs changes.
Comment #181
cburschkaShould we get rid of the unused EntityInterface import in the FileEntityNormalizer class? :)
Comment #182
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis 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?Comment #183
BerdirLets 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.
Comment #184
cburschkaApparently 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.)
Comment #185
cburschkaComment #186
Wim Leers#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.
Comment #187
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAdding reviewer credit. Thanks everyone for all the great reviews!
Comment #189
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.5.x and published the CR.
Comment #190
Wim Leers🎉🎉🎉
Comment #191
Wim LeersUnblocked: #1927648-474: Allow creation of file entities from binary data via REST requests, hurray! (YES, that is comment 474!)
Comment #192
Wim LeersThis also helps unblock JSON API: #2926463-7: [>=8.5] Remove 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!
Comment #193
Wim LeersMore 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.
Comment #195
dawehnerI opened up a follow up for this issue on https://www.drupal.org/project/drupal/issues/2956359#comment-12545816