Problem/Motivation
The file entity overrides the url() method to to provide a fake entity URL. This was done to to being able to expose the file URL in hal+json API requests. That implementation no longer relies on that, thanks to #2922487: Follow-up for #2910211: fix all deprecation warnings.
Original issue description:
If a file entity is loaded, a call to link()
function ($entity->link()
), results in following exception:
Drupal\Core\Entity\Exception\UndefinedLinkTemplateException: No link template "canonical" found for the "file" entity type in Drupal\Core\Entity\Entity->urlInfo() (line 188 of /var/www/html/drupal8/core/lib/Drupal/Core/Entity/Entity.php).
Proposed resolution
Originally, this issue was about the fact that toUrl() did not implement that behavior and initial implementations and earlier patches attempted to replace that, in a dynamic way depending on having a canonical link template (which e.g. the file_entity project adds).
The new suggested proposed solution is to replace that with an explicit API method that also allows easy access to the root-relative file URL and even makes that the default.
An entity can not have a canonical link template, and every caller must support that.
Remaining tasks
User interface changes
API changes
See change record: https://www.drupal.org/node/3019830
$file->createFileUrl() == file_url_transform_relative(file_create_url($file->getFileUri()));
file_create_url($file->getFileUri()) == $file->createFileUrl(FALSE);
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#103 | 2402533-103.patch | 22.84 KB | Wim Leers |
#103 | interdiff.txt | 782 bytes | Wim Leers |
#100 | 2402533-100-interdiff.txt | 8.17 KB | Berdir |
#100 | 2402533-100.patch | 23.1 KB | Berdir |
#93 | 2402533-93.patch | 21.35 KB | Berdir |
Comments
Comment #1
dawehnerWhat would be the expected behaviour, the path to the file?
Comment #2
cs_shadow CreditAttribution: cs_shadow commentedYes, I would expect a link to the path of the file.
Comment #3
BerdirCan we even specify that as a route/link template? would have to be a dynamic route processor like ? Or a file/1/download path, similar to how file_entity has.
Note that file_entity adds this, to file/1, see https://github.com/md-systems/file_entity/blob/8.x-2.x/file_entity.modul....
Also, url() is currently overriden and directly calls file_create_url() directly.. I have to undo this in FileEntity as well.
Comment #4
dawehnerWell you, you would certainly need to do some magic in order to get it running properly.
Comment #5
cs_shadow CreditAttribution: cs_shadow commentedShouldn't it suffice to implement
link()
function instead which simply creates the link usingurl()
andlabel()
. Maybe something like:This would circumvent the problem of defining it as a link template
Comment #6
Dave ReidComment #7
slashrsm CreditAttribution: slashrsm commentedComment #8
slashrsm CreditAttribution: slashrsm commentedComment #9
tamasd CreditAttribution: tamasd commentedI have started working on this issue.
Comment #10
tamasd CreditAttribution: tamasd at Pronovix commentedComment #11
jhedstromThis should probably have a test if possible.
Comment #12
tamasd CreditAttribution: tamasd at Pronovix commentedI added a testcase.
Comment #13
slashrsm CreditAttribution: slashrsm commentedThank you for your work! Patch looks almost ready. I have just one comment:
Since we're essentially creating canonical link here we should probably throw an exception if any other type of link is requested (+ test for that of course).
Also, our best practice is to submit interdiffs with patches. You can read more about them at https://www.drupal.org/documentation/git/interdiff.
Comment #14
tamasd CreditAttribution: tamasd at Pronovix commentedWhich exception should I throw? UndefinedLinkTemplateException?
Comment #15
slashrsm CreditAttribution: slashrsm commentedYes, that would make sense.
Comment #16
tamasd CreditAttribution: tamasd at Pronovix commentedComment #17
slashrsm CreditAttribution: slashrsm commentedWe should provide some useful information here. See how \Drupal\Core\Entity\Entity::link() throws this.
Comment #18
BerdirHm.
Why don't we implement urlInfo() so that it returns a Url object for the file URI? Then we don't need to special case either url or link anymore?
Then there's also no reason to throw an exception, urlInfo() will do that for you if a link template is not defined.
That urlInfo() method could also be nice and only do that thing with file_create_url() if there is no defined canonical link template.
Then I don't have to override that again in file_entity which *does* have a real canonical link template: https://github.com/md-systems/file_entity/blob/8.x-2.x/src/Entity/FileEn...
Comment #19
tamasd CreditAttribution: tamasd at Pronovix commentedComment #20
tamasd CreditAttribution: tamasd at Pronovix commentedComment #21
marthinal CreditAttribution: marthinal commentedI have the same problem described here but when creating an entity file from rest. I can confirm that the patch fixes the problem in this case.
Thanks!
Comment #22
BerdirAs mentioned above, turn this around. if ($rel == 'canonical' && !$this->hasLinkTemplate('canonical)) { do your thing }
otherwise fall back to parent::fromUri(). That will throw an exception for you.
Comment #23
slashrsm CreditAttribution: slashrsm commentedComment #24
BerdirNice :)
I think this should allow us to drop the custom url() method as well, since that will now fall back on urlInfo() and work just fine. And I can remove my url() method copy as well in file_entity :)
Comment #25
BerdirAh, actually, we can't, because there is no link template and then url() doesn't work. argh...
This is as good as it can be then, for now.
Comment #26
Wim LeersA lot of small problems here. Fixed them all.
Comment #27
dawehnerSo we are dealing with URLs now but the test is dealing with links, I think we should expand the test coverage for it
Comment #29
BerdirThis needs to be updated to change toUrl() now.
Also, while we can't remove url(), I think we should consider to add the same has link template check there, then file_entity doesn't have to do weird things to override it. But we can also do that in a different issue.
Comment #30
alexpottAnother problem is that the current url() implementation...
Totally ignore $options making it totally non compliant with the interface and common sense. Also it always returns an absolute url which is unlike any other entity.
Comment #31
BerdirReroll, moving the code to toUrl().
I tried doing a file_url_transform_relative() but I just can't get the tests to work with that. vfs:// and file_create_url() really don't like each other, weird stuff like this happens:
Comment #32
Leon Kessler CreditAttribution: Leon Kessler commentedFound a slight typo in your patch @Berdir
Uploaded new patch and interdiff
Comment #33
estoyausenteI had the same problem with exactly the same error. I applied the last patch and now it's working correctly. I'm using Drupal 8.1.2 version.
I have tested link, toLink and toUrl metods.
Comment #35
BerdirFails in an update path test, I assume that was unrelated/random fails.
Comment #36
alexpottLooking at #2676552: File usage view breaks if an entity uses a file that has no canonical link template makes me ponder about the wisdom of returning a value for a canonical link when a canonical link template does not exist. If we commit this issue file links will suddenly work and then it commit that issue because hasLinkTemplate() returns false they suddenly won't be links.
I'm not sure what the right answer is here.
Comment #37
BerdirNot sure if we have a choice.
File::url() returns that now, and code relies on it. rest.module does (this is why this was added in the first place) and I'm sure there are endless twig templates now that are doing something like node.some_field.entity.url, seen a bunch of examples, also on stack exchange and so on.
The only alternative idea I have is that we could add a new specific method for this, getFileUrl(). The problem is that it will be confusing for people who try to stop using deprecated methods and the documentation says to use toUrl().
Comment #38
alexpottSo I think here we need to call parent and catch the UndefinedLinkTemplateException exception because it is possible that an implementation will alter the File entity type to have a uri_callback (unfortunately still supported).
Comment #39
chr.fritschI adjusted the patch with alex hints
Comment #40
BerdirIsn't that the patch for the other issue? (#2676552: File usage view breaks if an entity uses a file that has no canonical link template)
Comment #41
chr.fritschOh, fu.. damn. That happened because we work together on both issues...
Comment #42
tjwelde CreditAttribution: tjwelde at Thunder commentedSince it should be possible to provide an uri_callback, we tried another approach. We catch the error and if a canoncial url is requested, we provide it like before, else the error is rethrown.
Comment #44
tjwelde CreditAttribution: tjwelde at Thunder commenteduploaded a correctly formatted patch...
Comment #45
tjwelde CreditAttribution: tjwelde at Thunder commentedComment #46
tjwelde CreditAttribution: tjwelde at Thunder commentedBetter comments.
Comment #47
alexpottThis is a change of behaviour that we shouldn't be making because now it will fail for $rel = 'whatever' whereas head will work.
This doesn't need to be abstracted to a private method. I think it might be better to catch all exceptions and check the $rel in there... ie...
Comment #48
tjwelde CreditAttribution: tjwelde at Thunder commentedI incorporated your suggestions and as we discussed, I left the url method untouched, since it is deprecated.
The only issue could be, that
url
will now always return a string, butlink
could be throwing an error, if the rel is not canonical.Comment #49
alexpottFixing up some code style issues. We don't support additional comments when using
@inheritdoc
Comment #50
YesCT CreditAttribution: YesCT commentedI would be happy to review this and make some tweaks to some comments. But I wont get to it for like another 16 hours or so. :)
Comment #51
YesCT CreditAttribution: YesCT commentedGonna look at this now.
Comment #52
YesCT CreditAttribution: YesCT commentedI'm still working on this, but wanted to post some intermediate work.
Improved the wording of the warning to not use the deprecated method (and will eventually use the trigger error in there that is commented out to find out where core uses it).
Fixed nits in some other comments.
Tried to fix the test so that phpstorm knew all the classes and methods and it was using all the classes... and while doing that, found out that this phpunit test is extending FileManagedUnitTestBase which extends an old deprecated kernel test base that is not a phpunit test... so I think the new test was not actually being run (which is a little iffy cause the testbot verbose test results said that that test had 3 passes. https://dispatcher.drupalci.org/job/default/158456/consoleFull) [edit: actually this passed on the testbot cause the testbot will run any test as whatever it needs to be (in this case the old kernel base test, without worrying about the namespace telling it it should be a phpunit test). but the new kernel phpunit tests are preferred over the old deprecated one, so I'll continue in this direction.]
To make it a phpunit test, changed the class it extends, and then... copy and pasted (with just a few small tweaks to coding standards) a couple methods into the new class.
Now the phpunit test runs on the command line. :) but it fails.
Next I'll take a closer look at the new test file and try and improve it.
Comment #54
YesCT CreditAttribution: YesCT commentedI'm starting back on this today. (at dev days)
Comment #55
YesCT CreditAttribution: YesCT commenteduploading just the fix with the
and setting to needs review, so the testbot runs, so we can see if we get any fails in core to detect where we are calling the deprecated method
Comment #57
YesCT CreditAttribution: YesCT commentedI wanted to improve this doc, so that it referenced the place someone could look to find out what stream schemes were available.
But... that's not easy. So I think I concluded to leave it as just an english list.
here were some thoughts I had (for background and to save them for later)
" * To know what the stream schemes are in Drupal Core, search for name: stream_wrapper, scheme: service tags."
related #2264047: [meta] Document service definition tags but that would make tagged services findable, but not list them by category..
public and temporary are in core.services.yml
ag "name: stream_wrapper, scheme: "
private is defined dynamically in CoreServiceProvider
there is an API to list them. see getWrappers() on StreamWrapperManagerInterface
Comment #58
YesCT CreditAttribution: YesCT commentedfixing this api docs to follow standards for how to specify a param is optional. https://www.drupal.org/node/1354#param
"Optional parameters are indicated by (optional);"
these are helper methods that should only work/be used for this test, so making them protected.
Comment #59
YesCT CreditAttribution: YesCT commentedbecause...
t should not be in assert messages,
in general the default messages are more useful than custom messages
- except for assertTrue()
- - usually there is a more semantic assert to use instead of assertTrue()
Comment #60
YesCT CreditAttribution: YesCT commentedthis was copied from another method. so I was looking at it and not understanding why the hook stuff, why the assert and the comment about the write directly just seems out of place. So I'm trying this simplified version.
Hm. I wonder if there is a similar generic helper already...
Comment #61
YesCT CreditAttribution: YesCT commentedCreates ... and *tests* something.
Also, testing the deprecated methods is a good thing (until we remove them) but this test is not the place to do that (or this method is not the place to do that).
So I'm taking out the testing of the deprecated methods. (maybe another issue to investigate if we have those already and maybe to make them if we do not)
Comment #62
YesCT CreditAttribution: YesCT commentedHere are all those changes.
General note, I'm not totally clear on if the tests are testing the correct things, but let's get the whole thing up and see.
I'll not be back to this today. So anyone else wants to work on it or review, please, you are welcome to. :)
Comment #68
sic CreditAttribution: sic commentedany updates on this?
Comment #69
BerdirSad ignored issue is sad. Rerolled for now.
Comment #72
sic CreditAttribution: sic commented...? :)
Comment #73
Wim LeersThe reroll in #52 broke this test. This is all wrong:
Reverting those changes makes the test pass. It also prevents duplication. And it actually does work.
Comment #74
Wim LeersThis is now a 8.5.x-only patch.
Also, we should check how this relates to #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field.
Comment #75
vijaycs85use not use?
we could catch them in their own catch()
Comment #76
vijaycs85Comment #78
pacproduct CreditAttribution: pacproduct at Makina Corpus commentedI tried the latest patch against D8.4, and
File->toUrl()
returns the absolute path to the file (e.g.http://default/sites/default/files/my-file.jpeg
), as mentionned by @alexpott in #30.I would have expected
/sites/default/files/my-file.jpeg
, which is the behavior of Node entities.Not sure what the best approach is, but here is how I get the expected URL from a file (is there a better way?) :
Comment #79
vijaycs85Thanks for testing it @pacproduct (and great to see you in issue queue). I believe I missed the exception part from previous patch.
Comment #80
borisson_Why is this deprecation commented? We should enable the deprecation. I also don't see
it is important to use...
anywhere else in core.Instead we should do:
Comment #81
Wim LeersThis issue was brought up again in #2825487-157: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field (and #2825487-147: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field).
I think the solution can lie in providing a
canonical
route for theFile
entity which:?_format
or with the equivalent explicit?_format=html
would redirect to the file?_format
, e.g.?_format=hal_json
or?_format=api_json
would be handled by the REST or JSON API moduleThoughts?
Comment #82
Wim LeersComment #83
BerdirThat logic would however still be inconsistent as soon as you install file_entity because then there is a real canonical link template.
IMHO REST should be capable of to handling entities without a canonical URL. Maybe it's not vey REST-y if a thing doesn't have a canonical URL but it just doesn't make sense for plenty of entities to have one.. embedded things like paragraphs and order items for example.
Comment #84
Wim LeersI'd be totally fine with that. In fact, that's what I am trying to do in #2922487, see #2922487-19: Follow-up for #2910211: fix all deprecation warnings and later. I'd love your help & input on that one!
The problem with starting to do that is that you end up breaking BC for … you guessed, it,
File
entities'File::url()
…Comment #87
sic CreditAttribution: sic commentedin hook_node_links_alter i am trying to set the url object of a file instead of a node, which results in the error message. is there any other way?
Comment #88
BerdirWhat you need is something like this:
file_url_transform_relative(file_create_url($file->getFileUri()))
.Yes, it's a quite long, I'd be more than happy to review a patch that adds a dedicated method to the file entity class :)
Comment #89
sic CreditAttribution: sic commentedthanks Berdir, but $links['node']['#links']['node-readmore']['url] expects an url object, file_url_transform_relative returns a string :/ probably wrong place to discuss here, i was just wondering what to do
Comment #90
BerdirIf you need an Url object then you can create one from that string with Url::fromUri('base:' . $file_path);
Comment #91
BerdirI think we discussed (agreed?) in issues like the referenced one that this behavior should be deprecated entirely, so restarting the patch with an active trigger_error() to see if we have any cases that are still calling it.
Comment #93
BerdirAll the remaining usages were actually in tests in core.
I don't know why nobody every wanted to implement my suggestion to have a createFileUrl() method on file entities, this is beautiful ;)
Also completely rewrote the issue title and issue summary.
Comment #94
BerdirComment #95
Wim LeersThanks for picking this up again, @Berdir!
YES 🎉🎉🎉🎉
Übernit: I'd prefer
assert($file instanceof FileInterface)
over this. Stronger assurances, same IDE completion niceties.Comment #96
alexpottCan't see any explicit test coverage of this new method.
At some point we need to work out what to do with file_create_url() and file_url_transform_relative() - not here though.
Should this not say $this->createFileUrl()
Cann't see any explicit test coverage of this deprecation.
I'm not sure this should be the legacy test.
Is this change necessary?
Comment #97
Berdir> Can't see any explicit test coverage of this new method.
How explicit? There are 20 or so calls to it now and lots of test coverage for that, for both cases, isn't that explicit enough? Unit test isn't an option and not sure what a kernel test would bring, afaik file_create_url() does strange things there anyway due to the virtual file system.
> At some point we need to work out what to do with file_create_url() and file_url_transform_relative() - not here though.
#2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it
> Can't see any explicit test coverage of this deprecation.
> I'm not sure this should be the legacy test.
Well, there is a deprecated test ;)
According to \Drupal\Tests\Traits\ExpectDeprecationTrait::expectedDeprecations:
throw new $assertion_failed_error('Only tests with the `@group legacy` annotation can call `setExpectedDeprecation()`.');
And yes, the test itself isn't deprecated, it just calls one deprecated method to test the deprecation. We would need a @group tests-legacy-but-isn't-legacy itself?
But I guess you're saying this needs a new dedicated test that only tests $file->url() and the other call should be converted.
Comment #98
alexpottYep exactly.
Comment #99
goodboyfile_create_url()
can returns FALSE, so we need to check that before passing$url
to thefile_url_transform_relative()
.Comment #100
Berdir#95.3 Used assert() or just if ($file instanceof FileInterface)
#96.1 Did not add more test coverage. IMHO, the existing coverage is enough and in a kernel test the output is weird anyway, this is what you get: http://localhost/vfs://root/sites/simpletest/19315468/files/example.txt, so I don't see why we would want to hardcode something like that, would need to be in a browser test...
#96.3 Removed the whole comment, I think the @trigger_error() is enough, that was still there from earlier patches.
#96.4 + 5: Split the test, removed the url() call completely, we're just testing that a bunch of methods return the same, which they obviously as this is the same entity. The behavior of entity reference fields is tested plenty.
#96.6: Well, it's not strictly necessary to do here but the file_create_url() call is and always was completely bogus since buildUrl() already does call that, so what are doing there is file_create_url(file_create_url()), Found that while looking for case to simplify.
#99: Added, but core is full of unconditional file_url_transform_relative(file_create_url()) calls, I don't think a healthy D8 site should be returning false, at least on on managed file entities. These days we would possibly throw an exception for that case..
Comment #101
goodboyException is a right way but I just what I talk is what I see.
file_create_url()
can returns false because getWrapper() can returns false.Comment #102
kim.pepperThanks @Berdir. Looks like all feedback has been addressed.
Comment #103
Wim LeersAs the maintainer of this class: I like this!
Nit: inheritdoc.
Unused; should be removed.
Fixed all those nits. Keeping at RTBC.
Comment #104
alexpottCrediting @borisson_ for code review.
Committed b2160ca and pushed to 8.7.x. Thanks!
Comment #106
Wim LeersThanks to this, rolled #2646744-34: \Drupal\Core\Url does not accept root-relative (file) URLs, making it impossible to let LinkGenerator create root-relative file URL links to use this new API :)
Comment #107
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedAm I understanding this correctly, have we decided against a canoncial URL for file entities?
Comment #108
BerdirYes, files don't have a canonical URL in core, that was a lie :)
Comment #110
MrPaulDriver CreditAttribution: MrPaulDriver commentedI am seeing this problem with 8.7.7 but finding that patch 103 will not apply.
Must I go with 8.7.x-dev ?
I'd rather not
Comment #111
BerdirI don't know what "this problem" is, the patch here has been committed long ago and is part of every 8.7 release. If you have a problem you should open a new issue and describe what that is.
Comment #112
MrPaulDriver CreditAttribution: MrPaulDriver commentedThanks for the clarification @Berdir
My problem is very similar to this one and is related to the Manage Display module and I opened this issue.
I think that aspects of Manage Display may end up in core, so it could be worth a closer look.