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

CommentFileSizeAuthor
#103 2402533-103.patch22.84 KBWim Leers
#103 interdiff.txt782 bytesWim Leers
#100 2402533-100-interdiff.txt8.17 KBBerdir
#100 2402533-100.patch23.1 KBBerdir
#93 2402533-93.patch21.35 KBBerdir
#91 2402533-91.patch968 bytesBerdir
#79 2402533-diff-76-79.txt636 bytesvijaycs85
#79 2402533-79.patch3.76 KBvijaycs85
#76 2402533-76.patch3.72 KBvijaycs85
#76 2402533-diff-73-76.txt1.77 KBvijaycs85
#73 2402533-73.patch3.83 KBWim Leers
#73 interdiff.txt2.85 KBWim Leers
#69 2402533-69.patch5.79 KBBerdir
#62 2402533-62.patch5.78 KBYesCT
#62 2402533-62-with-trigger-error.patch5.78 KBYesCT
#62 2402533-62-testsonly.patch3.69 KBYesCT
#62 interdiff.2402533.58ish.62.txt3.56 KBYesCT
#62 interdiff.2402533.52.58ish.txt2.61 KBYesCT
#55 2402533-55-triggerdeprecatederror.patch2.09 KBYesCT
#52 interdiff.2402533.49.52.txt5.1 KBYesCT
#52 2402533-52.patch6.47 KBYesCT
#49 2402533-49.patch3.47 KBalexpott
#49 48-49-interdiff.txt3.14 KBalexpott
#48 interdiff-2402533-46-48.txt2.77 KBtjwelde
#48 no_link_template-2402533-48.patch3.66 KBtjwelde
#46 interdiff-2402533-44-46.txt1.69 KBtjwelde
#46 no_link_template-2402533-46.patch4.3 KBtjwelde
#44 no_link_template-2402533-44.patch4.09 KBtjwelde
#42 no_link_template_2402533_42.patch4.3 KBtjwelde
#39 file_usage_view_breaks-2676552-39.patch3.96 KBchr.fritsch
#32 interdiff.txt619 bytesLeon Kessler
#32 no_link_template-2402533-32.patch2.59 KBLeon Kessler
#31 2402533_30.patch2.59 KBBerdir
#26 interdiff.txt2.16 KBWim Leers
#26 2402533_26.patch2.13 KBWim Leers
#23 interdiff.txt969 bytesslashrsm
#23 2402533_22.patch2.31 KBslashrsm
#19 interdiff-2402533-16-19.txt1.25 KBtamasd
#19 drupal-file-entity-link-2402533-19.patch2.16 KBtamasd
#16 interdiff-2402533-12-16.txt1.51 KBtamasd
#16 drupal-file-entity-link-2402533-16.patch2.14 KBtamasd
#12 drupal-file-entity-link-2402533-12.patch1.5 KBtamasd
#10 drupal-file-entity-link-2402533-10.patch897 bytestamasd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

What would be the expected behaviour, the path to the file?

cs_shadow’s picture

Yes, I would expect a link to the path of the file.

Berdir’s picture

Can 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.

dawehner’s picture

Well you, you would certainly need to do some magic in order to get it running properly.

cs_shadow’s picture

Shouldn't it suffice to implement link() function instead which simply creates the link using url() and label(). Maybe something like:

'<a href="' . $this->url() . '">' . $this->label() . '</a>';

This would circumvent the problem of defining it as a link template

Dave Reid’s picture

Issue tags: +Media Initiative
slashrsm’s picture

Issue tags: +sprint
slashrsm’s picture

Issue tags: +D8Media
tamasd’s picture

I have started working on this issue.

tamasd’s picture

Status: Active » Needs review
FileSize
897 bytes
jhedstrom’s picture

Issue tags: +Needs tests

This should probably have a test if possible.

tamasd’s picture

I added a testcase.

slashrsm’s picture

Status: Needs review » Needs work

Thank you for your work! Patch looks almost ready. I have just one comment:

+++ b/core/modules/file/src/Entity/File.php
@@ -282,4 +284,15 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+  public function link($text = NULL, $rel = 'canonical', array $options = []) {
+    if (is_null($text)) {
+      $text = $this->label();
+    }

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.

tamasd’s picture

Which exception should I throw? UndefinedLinkTemplateException?

slashrsm’s picture

Yes, that would make sense.

tamasd’s picture

Status: Needs work » Needs review
FileSize
2.14 KB
1.51 KB
slashrsm’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Entity/File.php
@@ -282,4 +285,20 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    if ($rel !== 'canonical') {
+      throw new UndefinedLinkTemplateException();
+    }
+
+    $url = Url::fromUri($this->url($rel, $options));

We should provide some useful information here. See how \Drupal\Core\Entity\Entity::link() throws this.

Berdir’s picture

Hm.

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...

tamasd’s picture

tamasd’s picture

Status: Needs work » Needs review
marthinal’s picture

I 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!

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Entity/File.php
@@ -282,4 +285,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+  public function urlInfo($rel = 'canonical', array $options = []) {
+    if ($rel !== 'canonical') {
+      throw new UndefinedLinkTemplateException(SafeMarkup::format('File entity only has canonical url.'));
+    }
+
+    return Url::fromUri($this->url($rel, $options));

As 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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
969 bytes
Berdir’s picture

Status: Needs review » Needs work

Nice :)

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 :)

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Ah, 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.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.13 KB
2.16 KB

A lot of small problems here. Fixed them all.

dawehner’s picture

So we are dealing with URLs now but the test is dealing with links, I think we should expand the test coverage for it

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Needs review » Needs work

This 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.

alexpott’s picture

Another problem is that the current url() implementation...

  /**
   * {@inheritdoc}
   *
   * @see file_url_transform_relative()
   */
  public function url($rel = 'canonical', $options = array()) {
    return file_create_url($this->getFileUri());
  }

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.59 KB

Reroll, 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:

1) Drupal\Tests\file\Kernel\LinkTest::testPublicLink
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<a href="http://localhost/vfs://root/sites/simpletest/752232/files/druplicon.txt">druplicon.txt</a>'
+'<a href="/vfs://root/sites/simpletest/752232/files/druplicon.txt">druplicon.txt</a>'
Leon Kessler’s picture

Found a slight typo in your patch @Berdir

Uploaded new patch and interdiff

estoyausente’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: no_link_template-2402533-32.patch, failed testing.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Fails in an update path test, I assume that was unrelated/random fails.

alexpott’s picture

Looking 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.

Berdir’s picture

Not 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().

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -71,7 +73,25 @@ public function setFileUri($uri) {
    +    // Core does not provide any link templates for files so build the canonical
    +    // URL manually, but allow contrib to provide link templates.
    +    if ($rel == 'canonical' && !$this->hasLinkTemplate('canonical')) {
    +      return Url::fromUri(file_create_url($this->getFileUri()), $options);
    +    }
    +
    +    return parent::toUrl($rel, $options);
    

    So 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).

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
3.96 KB

I adjusted the patch with alex hints

Berdir’s picture

chr.fritsch’s picture

Status: Needs review » Needs work

Oh, fu.. damn. That happened because we work together on both issues...

tjwelde’s picture

Status: Needs work » Needs review
FileSize
4.3 KB

Since 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.

Status: Needs review » Needs work

The last submitted patch, 42: no_link_template_2402533_42.patch, failed testing.

tjwelde’s picture

uploaded a correctly formatted patch...

tjwelde’s picture

Status: Needs work » Needs review
tjwelde’s picture

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -71,7 +74,53 @@ public function setFileUri($uri) {
       public function url($rel = 'canonical', $options = array()) {
    -    return file_create_url($this->getFileUri());
    +    /** @var Url $uri */
    +    // copied from super method
    +    $uri = $this->toUrl($rel);
    +    $options += $uri->getOptions();
    +    $uri->setOptions($options);
    +    return $uri->toString();
    +  }
    

    This is a change of behaviour that we shouldn't be making because now it will fail for $rel = 'whatever' whereas head will work.

  2. +++ b/core/modules/file/src/Entity/File.php
    @@ -71,7 +74,53 @@ public function setFileUri($uri) {
    +  public function toUrl($rel = 'canonical', array $options = []) {
    +    try {
    +      return parent::toUrl($rel, $options);
    +    } catch (UndefinedLinkTemplateException $e) {
    +      return $this->handleCanonicalUrlExceptions($rel, $options, $e);
    +    } catch (EntityMalformedException $e) {
    +      return $this->handleCanonicalUrlExceptions($rel, $options, $e);
    +    }
    +  }
    +
    +  /**
    +   * This method handles Exceptions thrown from toUrl.
    +   * If the relationship type is "canonical", but there is no linktemplate or a uri_callback, it builds an Url from the file uri.
    +   * If the relationship type is not canonical, it just rethrows the Exception
    +   *
    +   * @param string $rel
    +   *   The link relationship type, for example: canonical or edit-form.
    +   * @param array $options
    +   *   See \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute() for
    +   *   the available options.
    +   * @param UndefinedLinkTemplateException|EntityMalformedException $e
    +   *   The Exceptions thrown from toUrl
    +   * @return \Drupal\Core\Url
    +   * @throws UndefinedLinkTemplateException|EntityMalformedException
    +   *    If we don't want a canonical Url
    +   */
    +  private function handleCanonicalUrlExceptions($rel, $options, $e) {
    +    if ($rel == 'canonical') {
    +      return Url::fromUri(file_create_url($this->getFileUri()), $options);
    +    }
    +    else {
    +      throw $e;
    +    }
       }
    

    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...

    public function toUrl() {
      try {
        return parent::toUrl($rel, $options);
      }
      catch (\Exception $e) {
        if ($rel === 'canonical' && ($e instanceOf UndefinedLinkTemplateException || $e instanceOf EntityMalformedException) {
          return  Url::fromUri(file_create_url($this->getFileUri()), $options);
        }
        throw $e;
      }
    }
    
tjwelde’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
2.77 KB

I 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, but link could be throwing an error, if the rel is not canonical.

alexpott’s picture

Fixing up some code style issues. We don't support additional comments when using @inheritdoc

YesCT’s picture

I 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. :)

YesCT’s picture

Gonna look at this now.

YesCT’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, 52: 2402533-52.patch, failed testing.

YesCT’s picture

Issue tags: +DevDaysMilan

I'm starting back on this today. (at dev days)

YesCT’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.09 KB

uploading just the fix with the

+    trigger_error('For file entities, it is important to use toUrl().', E_USER_DEPRECATED);

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

Status: Needs review » Needs work

The last submitted patch, 55: 2402533-55-triggerdeprecatederror.patch, failed testing.

YesCT’s picture

+++ b/core/modules/file/tests/src/Kernel/LinkTest.php
@@ -0,0 +1,119 @@
+   * @param string $scheme
+   *   Optional string indicating the stream scheme to use. Drupal core includes
+   *   public, private, and temporary. The public wrapper is the default.

I 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: "

  stream_wrapper.public:
    class: Drupal\Core\StreamWrapper\PublicStream
    tags:
      - { name: stream_wrapper, scheme: public }
  stream_wrapper.temporary:
    class: Drupal\Core\StreamWrapper\TemporaryStream
    tags:
      - { name: stream_wrapper, scheme: temporary }

private is defined dynamically in CoreServiceProvider

    // Only register the private file stream wrapper if a file path has been set.
    if (Settings::get('file_private_path')) {
      $container->register('stream_wrapper.private', 'Drupal\Core\StreamWrapper\PrivateStream')
        ->addTag('stream_wrapper', ['scheme' => 'private']);
    }

there is an API to list them. see getWrappers() on StreamWrapperManagerInterface

YesCT’s picture

  1. +++ b/core/modules/file/tests/src/Kernel/LinkTest.php
    @@ -0,0 +1,119 @@
    +   * @param string $filepath
    +   *   Optional string specifying the file path. If none is provided then a
    

    fixing 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);"

  2. +++ b/core/modules/file/tests/src/Kernel/LinkTest.php
    @@ -0,0 +1,119 @@
    +  public function createUri($filepath = NULL, $contents = NULL, $scheme = NULL) {
    ...
    +  public function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
    

    these are helper methods that should only work/be used for this test, so making them protected.

YesCT’s picture

-    $this->assertTrue(is_file($file_path), t('The test file exists on the disk.'), 'Create test file');
+    $this->assertFileExists($file_path);

because...
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()

YesCT’s picture

+++ b/core/modules/file/tests/src/Kernel/LinkTest.php
@@ -0,0 +1,119 @@
+  public function createFile($filepath = NULL, $contents = NULL, $scheme = NULL) {
+    // Don't count hook invocations caused by creating the file.
+    \Drupal::state()->set('file_test.count_hook_invocations', FALSE);
+    $file = File::create([
+      'uri' => $this->createUri($filepath, $contents, $scheme),
+      'uid' => 1,
+    ]);
+    $file->save();
+    // Write the record directly rather than using the API so we don't invoke
+    // the hooks.
+    $this->assertTrue($file->id() > 0, 'The file was added to the database.', 'Create test file');
+
+    \Drupal::state()->set('file_test.count_hook_invocations', TRUE);
+    return $file;
+  }

this 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.

  protected function createFile($file_path = NULL, $contents = NULL, $scheme = NULL) {
    $file = File::create([
      'uri' => $this->createUri($file_path, $contents, $scheme),
      'uid' => 1,
    ]);
    $file->save();
    return $file;
  }

Hm. I wonder if there is a similar generic helper already...

YesCT’s picture

+++ b/core/modules/file/tests/src/Kernel/LinkTest.php
@@ -0,0 +1,119 @@
+   * Creates a link for a public file.
+   */
+  public function testPublicLink() {
+    $file = $this->createFile('druplicon.txt', NULL, 'public');
+    $this->assertEquals($file->toLink()
+      ->toString(), "<a href=\"{$file->toUrl()->toString()}\">{$file->label()}</a>");
+    // Test the deprecated methods as well.
+    $this->assertEquals($file->link(), "<a href=\"{$file->url()}\">{$file->label()}</a>");
+  }

Creates ... 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)

YesCT’s picture

Here 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. :)

The last submitted patch, 62: 2402533-62-testsonly.patch, failed testing.

The last submitted patch, 62: 2402533-62-with-trigger-error.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 62: 2402533-62.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sic’s picture

any updates on this?

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.79 KB

Sad ignored issue is sad. Rerolled for now.

Status: Needs review » Needs work

The last submitted patch, 69: 2402533-69.patch, failed testing. View results

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sic’s picture

...? :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.85 KB
3.83 KB

The reroll in #52 broke this test. This is all wrong:

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

Reverting those changes makes the test pass. It also prevents duplication. And it actually does work.

Wim Leers’s picture

vijaycs85’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -71,12 +74,36 @@ public function setFileUri($uri) {
    +    // especially important to use not use url() because the behavior is
    

    use not use?

  2. +++ b/core/modules/file/src/Entity/File.php
    @@ -71,12 +74,36 @@ public function setFileUri($uri) {
    +      if ($rel === 'canonical' && ($e instanceof UndefinedLinkTemplateException || $e instanceof EntityMalformedException)) {
    

    we could catch them in their own catch()

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
3.72 KB

Status: Needs review » Needs work

The last submitted patch, 76: 2402533-76.patch, failed testing. View results

pacproduct’s picture

I 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?) :

  ...
  protected static function getFileStdUri($file_object) {
    return file_url_transform_relative(file_create_url($file_object->getFileUri()));
  }
  ...
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
636 bytes

Thanks for testing it @pacproduct (and great to see you in issue queue). I believe I missed the exception part from previous patch.

borisson_’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Entity/File.php
@@ -71,12 +74,42 @@ public function setFileUri($uri) {
+    // trigger_error('For file entities, it is important to use toUrl().', E_USER_DEPRECATED);

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:

@trigger_error('Url is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. ::toUrl() instead.', E_USER_DEPRECATED);
Wim Leers’s picture

This 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 the File entity which:

  1. when requested without a ?_format or with the equivalent explicit ?_format=html would redirect to the file
  2. when requested with a non-HTML ?_format, e.g. ?_format=hal_json or ?_format=api_json would be handled by the REST or JSON API module

Thoughts?

Wim Leers’s picture

Berdir’s picture

That 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.

Wim Leers’s picture

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.

I'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()

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

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

sic’s picture

in 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?

if ($entity->hasField('field_file') &&! $entity->get('field_file')->isEmpty()) {
  $fileUrl = $entity->field_file->entity->toUrl();

  $readmore['url'] = $fileUrl;
}
Berdir’s picture

What 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 :)

sic’s picture

thanks 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

Berdir’s picture

If you need an Url object then you can create one from that string with Url::fromUri('base:' . $file_path);

Berdir’s picture

Status: Needs work » Needs review
FileSize
968 bytes

I 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.

Status: Needs review » Needs work

The last submitted patch, 91: 2402533-91.patch, failed testing. View results

Berdir’s picture

Title: No link template "canonical" found for the "file" entity type » Provide File::createFileUrl() as a replacement for the deprecated File:url() implementationh
Issue summary: View changes
Status: Needs work » Needs review
FileSize
21.35 KB

All 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.

Berdir’s picture

Title: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementationh » Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for picking this up again, @Berdir!

  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -74,13 +74,30 @@ public function setFileUri($uri) {
    +    @trigger_error('File entities returning the URL to the physical file in File::url() is deprecated, use $file->createFileUrl() instead. See https://www.drupal.org/node/3019830', E_USER_DEPRECATED);
    

    YES 🎉🎉🎉🎉

  2. You updated all callers! 🎉
  3. +++ b/core/modules/editor/src/Plugin/Filter/EditorFileReference.php
    @@ -75,9 +75,10 @@ public function process($text, $langcode) {
    +          /** @var \Drupal\file\FileInterface $file */
    
    +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/RSSEnclosureFormatter.php
    @@ -25,13 +25,14 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +      /** @var \Drupal\file\FileInterface $file */
    
    +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
    @@ -23,9 +23,10 @@ class UrlPlainFormatter extends FileFormatterBase {
    +    /** @var \Drupal\file\FileInterface $file */
    
    +++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
    @@ -226,10 +226,11 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +    /** @var \Drupal\file\FileInterface $file */
    

    Übernit: I'd prefer assert($file instanceof FileInterface) over this. Stronger assurances, same IDE completion niceties.

  4. Nit: in the CR, the first sentence is incomplete (File::url() did the following […]). I think it could even be removed?
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/src/Entity/File.php
    @@ -74,13 +74,30 @@ public function setFileUri($uri) {
    +  public function createFileUrl($relative = TRUE) {
    

    Can't see any explicit test coverage of this new method.

  2. +++ b/core/modules/file/src/Entity/File.php
    @@ -74,13 +74,30 @@ public function setFileUri($uri) {
    +    $url = file_create_url($this->getFileUri());
    +    if ($relative) {
    +      $url = file_url_transform_relative($url);
    +    }
    

    At some point we need to work out what to do with file_create_url() and file_url_transform_relative() - not here though.

  3. +++ b/core/modules/file/src/Entity/File.php
    @@ -74,13 +74,30 @@ public function setFileUri($uri) {
    +    // file_create_url($this->getFileUri()) directly.
    

    Should this not say $this->createFileUrl()

  4. +++ b/core/modules/file/src/Entity/File.php
    @@ -74,13 +74,30 @@ public function setFileUri($uri) {
    +    @trigger_error('File entities returning the URL to the physical file in File::url() is deprecated, use $file->createFileUrl() instead. See https://www.drupal.org/node/3019830', E_USER_DEPRECATED);
    

    Cann't see any explicit test coverage of this deprecation.

  5. +++ b/core/modules/file/tests/src/Kernel/FileItemTest.php
    @@ -16,6 +16,7 @@
    + * @group legacy
    
    @@ -76,13 +77,15 @@ protected function setUp() {
       /**
        * Tests using entity fields of the file field type.
    +   *
    +   * @expectedDeprecation File entities returning the URL to the physical file in File::url() is deprecated, use $file->createFileUrl() instead. See https://www.drupal.org/node/3019830
        */
       public function testFileItem() {
    

    I'm not sure this should be the legacy test.

  6. +++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
    @@ -277,7 +277,7 @@ public function testImageFieldSettings() {
    -    $url = file_url_transform_relative(file_create_url(ImageStyle::load('medium')->buildUrl($file->getFileUri())));
    +    $url = file_url_transform_relative(ImageStyle::load('medium')->buildUrl($file->getFileUri()));
    

    Is this change necessary?

Berdir’s picture

> 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.

alexpott’s picture

But I guess you're saying this needs a new dedicated test that only tests $file->url() and the other call should be converted.

Yep exactly.

goodboy’s picture

public function createFileUrl($relative = TRUE) {
    $url = file_create_url($this->getFileUri());
-    if ($relative) {
+    if ($relative && $url) {
      $url = file_url_transform_relative($url);
    }
    return $url;
  }

file_create_url() can returns FALSE, so we need to check that before passing $url to the file_url_transform_relative().

Berdir’s picture

Status: Needs work » Needs review
FileSize
23.1 KB
8.17 KB

#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..

goodboy’s picture

Exception 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.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Berdir. Looks like all feedback has been addressed.

Wim Leers’s picture

  1. +++ b/core/modules/editor/src/Plugin/Filter/EditorFileReference.php
    @@ -75,9 +76,8 @@ public function process($text, $langcode) {
    -          if ($file) {
    +          if ($file instanceof FileInterface) {
    

    As the maintainer of this class: I like this!

  2. +++ b/core/modules/file/tests/src/Kernel/FileLegacyTest.php
    @@ -0,0 +1,55 @@
    +   * Modules to enable.
    +   *
    +   * @var array
    ...
    +
    +  protected function setUp() {
    

    Nit: inheritdoc.

  3. +++ b/core/modules/file/tests/src/Kernel/FileLegacyTest.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Directory where the sample files are stored.
    +   *
    +   * @var string
    +   */
    +  protected $directory;
    

    Unused; should be removed.

Fixed all those nits. Keeping at RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Crediting @borisson_ for code review.

Committed b2160ca and pushed to 8.7.x. Thanks!

  • alexpott committed b2160ca on 8.7.x
    Issue #2402533 by YesCT, Berdir, tamasd, tjwelde, Wim Leers, vijaycs85,...
Wim Leers’s picture

gbyte’s picture

Am I understanding this correctly, have we decided against a canoncial URL for file entities?

Berdir’s picture

Yes, files don't have a canonical URL in core, that was a lie :)

Status: Fixed » Closed (fixed)

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

MrPaulDriver’s picture

I 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

Berdir’s picture

I 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.

MrPaulDriver’s picture

Thanks 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.