Follow-up for #2910211: fix all deprecation warnings

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Media/MediaResourceTestBase.php
@@ -154,7 +154,6 @@ protected function getExpectedNormalizedEntity() {
-          'url' => $file->url(),

@@ -166,7 +165,6 @@ protected function getExpectedNormalizedEntity() {
-          'url' => $thumbnail->url(),

I am confused because this issue seems to be about just removing deprecation warnings. So not using a deprecated code path.

But here we are actually changing what is expected in a the normalized entity. If are just fixing the use of deprecated code shouldn't the REST responses not change?

+++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
@@ -179,17 +179,22 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
-  protected function getEntityUri(EntityInterface $entity) {
+  protected function getEntityUri(EntityInterface $entity, array $context = []) {
...
+    $generated_url = $url->setRouteParameter('_format', 'hal_json')->toString(TRUE);
+    $this->addCacheableDependency($context, $generated_url);
+    return $generated_url->getGeneratedUrl();

How is that working? Right now, $context is not passed by reference. \Drupal\serialization\Normalizer\NormalizerBase::addCacheableDependency manipulates $context, but then nothing happens? I'm a bit confused ...

re #15 my understanding is you don't actually need to pass the $context by reference.

$context[ResourceResponseSubscriber::SERIALIZATION_CONTEXT_CACHEABILITY] contains an instance of \Drupal\Core\Cache\CacheableMetadata. So since it is object you always have reference to it(unless you explicitly clone it).

So in \Drupal\serialization\Normalizer\NormalizerBase::addCacheableDependency()
$context[ResourceResponseSubscriber::SERIALIZATION_CONTEXT_CACHEABILITY]->addCacheableDependency($data);
does affect the $context that \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody will have access to.

If \Drupal\serialization\Normalizer\NormalizerBase::addCacheableDependency() added or removed keys from the $context array itself then that would not affect the $context variable that the caller had because arrays are not passed by reference by default.

Oh nevermind, PHP is, well, not the nicest language :)

+++ b/core/modules/file/src/Entity/File.php
@@ -78,12 +79,42 @@ public function setFileUri($uri) {
+    // Lie because we must provide BC.
...
+    // Lie because we must provide BC.

This comment doesn't really provide a lot of value.

This might be better, but I'm not sure.
To provide BC, we provide the old value for canonical links.

Sorry for not finding this earlier:

+++ b/core/modules/file/src/Entity/File.php
@@ -78,12 +79,42 @@ public function setFileUri($uri) {
+    // REST support for file entities. https://www.drupal.org/node/2825487 makes
+    // this obsolete.
...
+    // @todo Remove in https://www.drupal.org/node/2825487.
...
+    // @todo Remove in https://www.drupal.org/node/2825487.

The issue linked to from here is already closed (also, both that issue and this one end on 487 just to make things extra confusing).

1. +++ b/core/modules/file/src/Entity/File.php
   @@ -78,12 +79,42 @@ public function setFileUri($uri) {
       */
   +  public function toUrl($rel = 'canonical', array $options = []) {
   +    // To not break BC, the old behavior is retained for 'canonical'.
   +    // @see ::url()
   +    // @todo Remove in Drupoal 9.0.0.
   +    if ($rel === 'canonical') {
   +      return Url::fromUri($this->url());
   +    }
   +    return parent::toUrl($rel, $options);
   +  }
   +
   

   Seems like this is heavily overlapping with #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation, as you also commented there.

   I'm not too happy about adding more code that's already deprecated, also without doing any deprecation messages.

   We have the already deprecated \Drupal\hal\Normalizer\FileEntityNormalizer, could we override getEntityUri() there and deal with file like that? Then it's all in one place and in 9.x, we can just remove that file. I think that's what we should have done from the beginning instead of overriding url() like that in the file entity class.

   An argument could be made that another entity type has a similar special override of url() and this would break it, but that's already kind of broken, you already have the url() call above that you call *despite* not having a canonical link template. The whole thing is a special case for file.

   And now we rely that file does lie about implementing the canonical link template so that we skip the condition that we befor e had specifically for file :)

2. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -179,17 +179,25 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
        // out to ->url().
        if ($entity->isNew() || !$entity->hasLinkTemplate('canonical')) {
          return $entity->url('canonical', []);
        }
        $url = $entity->urlInfo('canonical', ['absolute' => TRUE]);
   -    return $url->setRouteParameter('_format', 'hal_json')->toString();
   +    if (!$url->isExternal()) {
   

   This is still calling urlInfo(). which is deprecated, just doesn't trigger a warning yet, does it make sens to fix that here too?

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -179,17 +179,25 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
       * @return string
       *   The entity URI.
   

   we should probably improve the return documentation a bit and mention that it can be a string (deprecated?) or a GeneratedUrl object?

2. +++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
   @@ -69,4 +70,16 @@ public function normalize($entity, $format = NULL, array $context = []) {
   +   * {@inheritdoc}
   +   */
   +  protected function getEntityUri(EntityInterface $entity, array $context = []) {
   +    // https://www.drupal.org/project/drupal/issues/2277705 introduced a hack
   +    // in \Drupal\file\Entity\File::url(), but EntityInterface::url() was
   +    // deprecated in favor of ::toUrl(). The parent implementation now calls
   +    // ::toUrl(), but this normalizer (for File entities) needs to override that
   +    // back to the old behavior because it relies on said hack.
   +    return $entity->url('canonical', []);
   +  }
   

   can't you do the same here as you did in getEntityUri() ?

   Convert that string again to a url object and add the option?

   That means the deprecation message that you want to get rid of here won't go away.

   We can't make the one from url() go away once we actually add that but we'll have to figure that out when we add that and want to get rid of the deprecation messages then.

   Actually... you could just call file_create_url($this->getFileUri()) directly here if we want to avoid the (future) deprecation messages.

   I'm actually not 100% sure what the final behavior in 9.x will be now for file in a REST response.

   We now have the file URL in a separate computed field, so overriding uri is already deprecated. IMHO, I would also expect that the file then would not have a top-level self link anymore, like I suggested in the issue referenced above. which means...

3. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -179,17 +179,25 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   -  protected function getEntityUri(EntityInterface $entity) {
   +  protected function getEntityUri(EntityInterface $entity, array $context = []) {
        // Some entity types don't provide a canonical link template, at least call
        // out to ->url().
        if ($entity->isNew() || !$entity->hasLinkTemplate('canonical')) {
          return $entity->url('canonical', []);
        }
   

   I'm not quite sure where this part should go, we can possibly figure it out as part of removing/really deprecating url(), but a separate issue for it probably makes sense, again based on comment that it is valid that an entity does not have a self link, so instead of returning url(), we should just return NULL and the caller should then allow that.

1. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -179,17 +179,25 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
       * @return string
       *   The entity URI.
   

   #47.1

2. +++ b/core/modules/hal/src/Normalizer/ContentEntityNormalizer.php
   @@ -179,17 +179,25 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
        // Some entity types don't provide a canonical link template, at least call
        // out to ->url().
        if ($entity->isNew() || !$entity->hasLinkTemplate('canonical')) {
          return $entity->url('canonical', []);
        }
   

   and #47.3 ;)

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -419,6 +419,8 @@ protected function getExpectedCacheContexts() {
      public function testGet() {
        $this->initAuthentication();
        $has_canonical_url = $this->entity->hasLinkTemplate('canonical');
   +    // @todo Remove in https://www.drupal.org/node/2825487.
   +    $has_canonical_url = $has_canonical_url && static::$entityTypeId !== 'file';
   

   as far as I see, these changes are not necessary anymore? hasLinkTemplate() is already FALSE, so this isn't doing anymore anymore.

   I reverted all changes in this file and at least FileHalJsonCookieTest passed just fine.

4. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -55,8 +55,14 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +      // @todo Remove in Drupal 9.
   +      // @see \Drupal\hal\Normalizer\FileEntityNormalizer
   +      else if ($entity->getEntityTypeId() === 'file') {
   +        $values['url'] = file_create_url($entity->getFileUri());
          }
   

   Why can't we handle this with a setting, just like bc_file_uri_as_url_normalizer?

   I guess we can't do this until we resolve #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization). what about instead of a @todo on 9, make it a @todo pointing to that issue and there we add file_url and deprecate this?

Huh, where exactly did all those months go since the last update here :)

1. No I did, I missed the getGeneratedUrl() call. All good.

I think this looks great now, pretty small patch in the end, without any change to the outside and fewer deprecated method calls. Would have set this to RTBC, but apparently the patch needs a reroll for 8.7 according to #54?

Re-rolled.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1470,7 +1470,8 @@ protected function makeNormalizationInvalid(array $normalization, $entity_key) {
   -    if ($this->entity->hasLinkTemplate('canonical') && ($this->account && static::$auth !== 'cookie')) {
   +    $has_canonical_url = $this->entity->hasLinkTemplate('canonical');
   +    if ($has_canonical_url && ($this->account && static::$auth !== 'cookie')) {
   

   This change is unnecessary - no?

2. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -55,8 +55,14 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +      // @todo Deprecate in https://www.drupal.org/project/drupal/issues/2925520
   +      // @see \Drupal\hal\Normalizer\FileEntityNormalizer
   +      else if ($entity->getEntityTypeId() === 'file') {
   +        $values['url'] = file_create_url($entity->getFileUri());
          }
   

   Are we deprecating or removing?

Addressed comments in #61.

Committed 68c0fd8 and pushed to 8.7.x. Thanks!

diff --git a/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
index e1cef84817..c5b869cb86 100644
--- a/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
+++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
@@ -61,7 +61,7 @@ public function normalize($field_item, $format = NULL, array $context = []) {
       }
       // @todo Remove in https://www.drupal.org/project/drupal/issues/2925520
       // @see \Drupal\hal\Normalizer\FileEntityNormalizer
-      else if ($entity->getEntityTypeId() === 'file') {
+      elseif ($entity->getEntityTypeId() === 'file') {
         $values['url'] = file_create_url($entity->getFileUri());
       }
     }

Fixed coding standards on commit.