Problem/Motivation

Child issue of #2959269: [meta] Core should not trigger deprecated code except in tests and during updates

DeprecationListenerTrait currently ignores the deprecation error "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility."

The problem is that another functionality was added later on that has not yet been deprecated, and the deprecation message about the whole service being deprecated is not correct. Additionally, it's not possible to deprecate tagged services, as they are automatically called - And in this case, have to, because even the deprecated part is configurable with a setting.

The functionality that was added later on is technically also a BC layer, to keep the current behavior of normalized file entities as the old behavior of $file->url() has been deprecated in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation. However, it had not been explicitly deprecated at that point and removing that results in DX regressions as normalized file fields would no longer include a link to the physical file.

There are two related issues:
* #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL: Fixes empty _links.self definitions for HAL serialization by falling back to the rest/entity_type/ID route. This is now a bugfix without BC breaks
* #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization): This will attempt to unify file and file item normalization to provide the same behavior as json serialization without having to "fake" _links.self to point to the actual file and not the real "self", the file entity.

Proposed resolution

This does the minimum that is still possible in D8. Remove the deprecation on the whole class, instead marks it as internal.

#2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization) will then eventually deprecate the other half of this service and we can then remove it in D10 (if we don't decide to remove hal.module entirely)

Remaining tasks

Notify sites about the fact that they rely on BC layers: #3099151: Add a requirements check to BC layers behind configuration settings

User interface changes

API changes

No change, a deprecated class becomes internal instead.

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Status: Active » Needs review
FileSize
1.36 KB

Patch only removes the error from the ignore list.

Status: Needs review » Needs work

The last submitted patch, 2: 3009854_2.patch, failed testing. View results

Berdir’s picture

I don't think this possible?

This is a tagged service, so it gets used automatically.

Also, the part in \Drupal\hal\Normalizer\FileEntityNormalizer::getEntityUri() is now actually required until 9.x to keep the current behavior of the uri field of file entities. Also not quite sure how to remove that then as it would be a change between 8.latest and 9.x, so we might need another setting to opt-out of this behavior?

We *can* probably remove the class in 9.x, but the trigger_error should possibly only be done inside the condition in\Drupal\hal\Normalizer\FileEntityNormalizer::normalize().

Berdir’s picture

> Also not quite sure how to remove that then as it would be a change between 8.latest and 9.x, so we might need another setting to opt-out of this behavior?

I think I remember now, we said that it is the job of officially deprecating the behavior of $file->url() to deal with that.

Wim Leers’s picture

I think I remember now, we said that it is the job of officially deprecating the behavior of $file->url() to deal with that.

This deprecation happened since then, in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation.

now actually required until 9.x to keep the current behavior of the uri field of file entities

Correct, we need to keep this for now to not break BC in D8.

Also not quite sure how to remove that then as it would be a change between 8.latest and 9.x, so we might need another setting to opt-out of this behavior?

We already have that setting! bc_file_uri_as_url_normalizer in hal.settings. See https://www.drupal.org/node/2925783.

So the serializer.normalizer.file_entity.hal service is deprecated and will be removable in D9, once we can stop supporting bc_file_uri_as_url_normalizer, which we can in D9.


We *can* probably remove the class in 9.x, but the trigger_error should possibly only be done inside the condition in\Drupal\hal\Normalizer\FileEntityNormalizer::normalize().

It's totally possible that we didn't do the deprecation the most ideal way in #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field! Can you explain why this would be better?

Wim Leers’s picture

Can you explain why this would be better?

I'm guessing it would be better in that then the deprecation error is only fired when the service is used for the BC case; in all other cases, it's already safe to remove the service? (Except what then about contrib services extending it? AFAIK we need the current deprecation style to notify those?)

Berdir’s picture

> This deprecation happened since then, in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation.
> We already have that setting! bc_file_uri_as_url_normalizer in hal.settings. See https://www.drupal.org/node/2925783.

Yeah, it's not quite so easy :) The service does 2 things. One is deprecated using the mentioned setting. But the other is trickier. Yes, we removed the call to $file->url(), but we did replace it with an explicit call to createFileUrl(), keeping BC to REST clients, to ensure that _self still points to the physical file. Removing this service now would result in file entities no longer having a _self.

The question is, is that only BC or do we want to keep that behavior:

a) If we do want to keep that, then the service is *not* deprecated and is still required in 9.x too.
b) If we do remove it, then we also need a setting the logic in getEntityUri().

> Can you explain why this would be better?

It is a tagged service, so it will *always* be loaded and accessed, we can't prevent that. That means we can't trigger a deprecation message for the service like that.

So we need to either:

a) Define the service dynamically in a service provider based on the setting. But we do want to keep it for around for BC I suppose, maybe someone depends on it somehow, who knows..
b) Move the deprecation message *inside* the condition, so it only triggers if someone is actually having that setting enabled.

So really only option :)

Wim Leers’s picture

D'oh, I indeed focused on \Drupal\hal\Normalizer\FileEntityNormalizer::normalize() and hence forgot about \Drupal\hal\Normalizer\FileEntityNormalizer::getEntityUri().

I should've noticed in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation that it was updating the logic of \Drupal\hal\Normalizer\FileEntityNormalizer::getEntityUri() but not the comment. That comment is now misleading.

The question is, is that only BC or do we want to keep that behavior:

I think we should remove it, because otherwise we need to maintain this once-hacky-workaround forever. Then we'd indeed need setting-dependent logic in getEntityUri(). What do you think?

Wim Leers’s picture

Berdir’s picture

Yeah, I tried to put push for putting both things behind the same BC setting when that was added, but I guess it was a bit early and the exact path to where we are now wasn't clear yet. +1 on adding another setting and then adding a dedicated @trigger_error() for each setting inside the respective condition.

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

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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

Yeah, a second setting is a bit awkward in regards to naming and stuff ;)

I also didn't add a @trigger_error() yet, since this isn't an API call that someone needs to update, it isn't going to show up in any tests or with drupal-check. Instead, I think a hook_requirements() check would be a better match for this, that reports it as a warning if that setting is still enabled.

Also needs a CR.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.75 KB

Status: Needs review » Needs work

The last submitted patch, 14: update-hal-file-entity-normalizer-2959282-15.patch, failed testing. View results

Berdir’s picture

This should fix the tests, had to remove the @deprecated as this was causing various test fails, somehow only in kernel tests.. The rest test changes show quite well how this affects the output.

That IMHO makes sense, because self points to the physical file and not the actual rest route. IMHO, the serialization API should automatically fall back to the rest route if an entity has no canonical route.

Also, it looks like that was the *only* path to the physical file. Based on the title, #2124677: Expose URI in file fields when serializing an object should have added that but it didn't because file entities have no canonical link template. #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization) is the new issue that got stuck and it got stuck on basically what this issue is doing, so we have a bit of a hen/egg situtation. But I think this needs to be postponed on that, I'll see if I can refresh that one.

Status: Needs review » Needs work

The last submitted patch, 16: update-hal-file-entity-normalizer-2959282-16.patch, failed testing. View results

Berdir’s picture

Fixed that too. I think I'm too eager in removing that @todo though, because that is #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL, which actually wants to do exactly what I proposed aboved, point self to entity/file/ID. Seems like it would make sense to combine this together, so we only need a single BC setting this time ;)

Status: Needs review » Needs work

The last submitted patch, 18: update-hal-file-entity-normalizer-2959282-18.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Needs review

Random JS test fail. Back to needs review, but as mentioned, I think this isn't committable without #2925520: Add a computed 'file_url' property to FileItem (for exposing file URL in file field normalization), and I also think we should have a fix for those empty self URL's,

Another missing part: a hook_requirements() that tells users which BC settings they have enabled and probably allows them to disable it.

andypost’s picture

+1 to get this in, bc_file_entity_uri_normalizer: false is great default!
from code side just nitpick

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -87,7 +85,12 @@ protected function getEntityUri(EntityInterface $entity, array $context = []) {
+    if ($this->halSettings->get('bc_file_entity_uri_normalizer')) {
+      return $entity->createFileUrl(FALSE);
+    }
+    else {
+      return parent::getEntityUri($entity, $context);
+    }
   }

`else` useless here

klausi’s picture

Issue tags: +drupaldevdays
+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -13,8 +13,6 @@
 /**
  * Converts the Drupal entity object structure to a HAL array structure.
- *
- * @deprecated in Drupal 8.5.0, to be removed before Drupal 9.0.0.
  */
 class FileEntityNormalizer extends ContentEntityNormalizer {

So we are un-deprecating this? I'm confused, do we want to keep this class in the future or not?

Berdir’s picture

Right now, the @deprecated is definitely a lie, there is non deprecated, necessary code in there.

I'm deprecating it again but based on a setting and having the tag there results in deprecation messages in tests.

We either have to remove it or dynamically register the service when either setting is enabled.

Wim Leers’s picture

Re-read the issue.

@Berdir and I reached consensus in #11, after @Berdir pointed out in #8 I was looking at things too simplistically in 😊

Then @Berdir went on to implement that consensus, in #14 + #16 + #18. But at the end of #16 he encountered a new roadblock 🙃

In #16, @Berdir points out that #2124677: Expose URI in file fields when serializing an object should have added a URL to the physical file (a "file URL" as in computed by file_create_url()). But that did not happen in #2124677: Expose URI in file fields when serializing an object (which predates Drupal 8.0.0). Over at #2907402-6: HAL normalization of file fields don't provide file entity id or file entity REST URL, @damiankloip points out that #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field should do that. And according to my tests, it did do just that: /entity/file/1?_format=hal_json returns this in HEAD:

{
  "_links": {
    "self": {
      "href": "http://d8.test/sites/default/files/2019-10/hipster%20llama_0.jpg"
    },
    "type": {
      "href": "http://d8.test/rest/type/file/file"
    },
    "http://d8.test/rest/relation/file/file/uid": [
      {
        "href": "http://d8.test/user/1?_format=hal_json"
      }
    ]
  },
…
  "uri": [
    {
      "value": "public://2019-10/hipster llama_0.jpg",
      "url": "/sites/default/files/2019-10/hipster%20llama_0.jpg"
    }
  ],
…
}

(Note the uri[0].url computed property appearing in the normalization!)

and with this patch:

{
  "_links": {
    "self": {
      "href": ""
    },
    "type": {
      "href": "http://d8.test/rest/type/file/file"
    },
    "http://d8.test/rest/relation/file/file/uid": [
      {
        "href": "http://d8.test/user/1?_format=hal_json"
      }
    ]
  },
…
  "uri": [
    {
      "value": "public://2019-10/hipster llama_0.jpg",
      "url": "/sites/default/files/2019-10/hipster%20llama_0.jpg"
    }
  ],
…
}

So … am I missing something, or is #16's observed problem actually not a problem? 🤔

Wim Leers’s picture

So while #16 was inaccurate AFAICT, #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL still remains a problem: given a File entity HAL normalization, there is no link to its URL. That's what #18 points out. So … I agree that merging #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL with this issue and solve both at the same time is the sensible thing to do: otherwise we change the same code twice.

Wim Leers credited tedbow.

Wim Leers’s picture

Wim Leers’s picture

I think this is what @Berdir is proposing. 🙂 Can you confirm, @Berdir?

Status: Needs review » Needs work

The last submitted patch, 29: 3009854-29.patch, failed testing. View results

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Berdir’s picture

Status: Needs work » Needs review
FileSize
18.49 KB
6.83 KB

As discussed, yes. I didn't rename the setting yet, but I'd then propose to use the same setting as well for this new behavior, although maybe that's not even required and we say that returning a URL where we previously returned an empty string is fine to just do? Either way, it will get a bit more complicated anyway with the reference normalization per that other issue. I suggest you read up on that and then we can discuss.

Most of the test fails were easy to fix, buuuuuut... FileNormalizerTest nicely points out that we have a dependency problem here. We're changing the normalization in hal_json, which does *not* rely on rest.module to require a route generated by rest.module *and* having that resource enabled. As a first step, I just made it conditional, which fixes the kernel test without any further changes as it is not enabled there.

That feels like cheating a bit, but to be honest, we already have the same dependency there, it's just better hidden. The assumption that we're making is that the entity route *is* the REST route, including the format.

*If* we decide that we need to have BC for the old behavior, we'll also need a test for that. If not, then we can remove that settings check from both code and tests.

Status: Needs review » Needs work

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

Berdir’s picture

Forgot to add the settings to the parent class, that's why it fails. Waiting on feedback on whether that even needs a settings check there before fixing that.

Wim Leers’s picture

although maybe that's not even required and we say that returning a URL where we previously returned an empty string is fine to just do?

Previously there was no information under _links.self. So I can’t think of a reason any API client would even read that —
the change being the addition of information then IMO does not need a BC layer.

That’s also what JSON:API and GraphQL preach, and API versioning in general: avoid removing or changing, only add. Then you don’t break API clients.

So: no BC layer necessary.

+++ b/core/modules/aggregator/tests/src/Functional/Hal/ItemHalJsonTestBase.php
@@ -60,7 +60,7 @@ protected function getExpectedNormalizedEntity() {
-          'href' => '',
+          'href' => $this->config('hal.settings')->get('bc_file_entity_uri_normalizer') ? '' : $this->baseUrl . '/entity/aggregator_item/1?_format=hal_json',

for this effectively.

Berdir’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Needs work » Needs review
FileSize
18.02 KB
5.07 KB

Yes, removed that check and made the self changes in the tests unconditional.

The other tricky part is the rest asumption/dependency, but I think that might be acceptable, it was kind of there before too as I wrote.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
1.28 KB

Removed one check of the variable that was still needed. Also made the BC test a bit more explicit.

+++ b/core/modules/media/tests/src/Functional/Hal/MediaHalJsonAnonTest.php
@@ -64,7 +64,7 @@ protected function getExpectedNormalizedEntity() {
         ],
         $this->baseUrl . '/rest/relation/media/camelids/thumbnail' => [
           [
-            'href' => $thumbnail->createFileUrl(FALSE),
+            'href' => $this->baseUrl . '/entity/file/2?_format=hal_json',
             'lang' => 'en',
           ],

As a reminder, this isn't ready yet. This change is the problem now. This is a file/image item embedded thingy, and we lose the file URL here as well, because it's all connected.

Berdir’s picture

So, I talked about this with @alexpott a bit, and while explaining the problem, I basically convinced myself and he also suggested to instead just to undeprecate this class.

We can still remove the one method that is already behind a setting. But the service-level deprecation is dropped and I moved the @deprecated on the class to that specific method. Not sure about doing a @trigger_error() inside the condition, as notifying tests about it wouldn't be very useful I think. As mentioned before, a requirements check that reports on any enabled BC settings like this might be better to reach the target audience here.

I also added an @internal on the class as this is definitely not something anyone should be using as an API.

Wim Leers’s picture

Woah … #39 is a complete departure from all previous patches?!

Removing the class-level and service-level deprecation is one thing, but the _links.self URI was a desirable change?

I am confused now.

Wim Leers’s picture

Wim Leers’s picture

The _links.self addition is now happening in #2907402-14: HAL normalization of file fields don't provide file entity id or file entity REST URL. I RTBC'd that.

That means this patch is solely about removing this one particular deprecation from the hardcoded list of skipped deprecations, which is absolutely essential to do before 9.0. I don't fully understand the reasoning behind #39. It looks like it's just working around the problem by moving the deprecation to a different level and intentionally omitting the triggering of a deprecation error which AFAIK is not okay? 🤔

Berdir’s picture

@trigger_error() has one purpose IMHO. To fail tests when someone has code that relies on a deprecated API/behavior. Its target audience are developers. I think it is very unlikely that tests are going to have that setting enabled.

This behavior is controlled by configuration, there is no alternative thingy that a developer should be using instead, not *inside* Drupal. They need to update their app or decoupled website, but a @trigger_error() wouldn't help them discover this problem.

We could easily add a @trigger_error() inside the if condition, but unless we start to actually log these to watchdog in 8.9, I don't that makes much sense.

IMHO, if we need something, then that's a hook_requirements() check that shows a warning or so that they might be relying on the deprecated thing that will go away in 9.x and should disable it.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Hm. That's fair. And this does not change the fact that we'll still remove the entire \Drupal\hal\Normalizer\FileEntityNormalizer class in Drupal 9.0.0 anyway.

The only way that #39 is a regression compared to HEAD is that people using that service directly won't be getting a deprecation error. But … you shouldn't do that for any normalizer anyway: the API is not that service, the API is to call $this->serializer->normalize() with a particular object, and it's the Serializer service that'll select the appropriate class/service.

So … 🚢

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

As this issue is a bit confusing it'd be great if the issue summary contained a summary of the issue comments that got us to this point and listed next steps for follow-ups to take on.

Once the issue summary is updated this can be put back to rtbc

Berdir’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

I've finally updated the issue summary. Also created a follow-up for #3099151: Add a requirements check to BC layers behind configuration settings.

Berdir’s picture

Conflicted with the fixed deprecation format. Trivial reroll of a technically trivial patch, so keeping at RTBC.

alexpott’s picture

+++ b/core/modules/hal/src/Normalizer/FileEntityNormalizer.php
@@ -61,6 +61,8 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Lin
   /**
    * {@inheritdoc}
+   *
+   * @deprecated in drupal:8.5.0 and is removed from drupal:9.0.0.
    */
   public function normalize($entity, $format = NULL, array $context = []) {

Is there a reason why we can't add an @trigger_error() to this method

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.44 KB
1.71 KB

Added a @trigger_error() and updated the BC test.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Tests passed and I just added the @trigger_error() and updated the test, so might be OK to move it back to move it back to RTBC? :)

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed a6d5e5b and pushed to 9.0.x. Thanks!
Committed 677f8fe and pushed to 8.9.x. Thanks!

I had to manually apply the patch to 9.0.x because their were conflicts in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() but they were simple to resolve.

Given that this is undeprecating something I think we should consider backporting this to 8.8.x once the freeze is over.

  • alexpott committed a6d5e5b on 9.0.x
    Issue #3009854 by Berdir, Wim Leers, Mile23, alexpott, andypost, tedbow...

  • alexpott committed 677f8fe on 8.9.x
    Issue #3009854 by Berdir, Wim Leers, Mile23, alexpott, andypost, tedbow...
alexpott’s picture

Status: Patch (to be ported) » Fixed

@catch +1'd the backport.

  • alexpott committed 151ce2d on 8.8.x
    Issue #3009854 by Berdir, Wim Leers, Mile23, alexpott, andypost, tedbow...

Status: Fixed » Closed (fixed)

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