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
Comment | File | Size | Author |
---|---|---|---|
#49 | 3009854-49-interdiff.txt | 1.71 KB | Berdir |
#49 | 3009854-49.patch | 4.44 KB | Berdir |
#47 | 3009854-47.patch | 3.02 KB | Berdir |
#39 | 3009854-39.patch | 3.02 KB | Berdir |
#38 | 3009854-38-interdiff.txt | 1.28 KB | Berdir |
Comments
Comment #2
Mile23Patch only removes the error from the ignore list.
Comment #4
BerdirI 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().
Comment #5
Berdir> 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.
Comment #6
Wim LeersThis deprecation happened since then, in #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation.
Correct, we need to keep this for now to not break BC in D8.
We already have that setting!
bc_file_uri_as_url_normalizer
inhal.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 supportingbc_file_uri_as_url_normalizer
, which we can in D9.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?
Comment #7
Wim LeersI'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?)
Comment #8
Berdir> 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 :)
Comment #9
Wim LeersD'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.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?Comment #10
Wim LeersComment #11
BerdirYeah, 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.
Comment #13
BerdirYeah, 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.
Comment #14
BerdirComment #16
BerdirThis 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.
Comment #18
BerdirFixed 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 ;)
Comment #20
BerdirRandom 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.
Comment #21
andypost+1 to get this in,
bc_file_entity_uri_normalizer: false
is great default!from code side just nitpick
`else` useless here
Comment #22
klausiSo we are un-deprecating this? I'm confused, do we want to keep this class in the future or not?
Comment #23
BerdirRight 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.
Comment #24
Wim LeersRe-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:(Note the
uri[0].url
computed property appearing in the normalization!)and with this patch:
So … am I missing something, or is #16's observed problem actually not a problem? 🤔
Comment #25
Wim LeersSo 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.Comment #28
Wim LeersTalked to @Berdir, we agreed to merge #2907402: HAL normalization of file fields don't provide file entity id or file entity REST URL with this. Transferring issue credit.
Comment #29
Wim LeersI think this is what @Berdir is proposing. 🙂 Can you confirm, @Berdir?
Comment #32
BerdirAs 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.
Comment #34
BerdirForgot 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.
Comment #35
Wim LeersPreviously 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.
for this effectively.
Comment #36
BerdirYes, 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.
Comment #38
BerdirRemoved one check of the variable that was still needed. Also made the BC test a bit more explicit.
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.
Comment #39
BerdirSo, 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.
Comment #40
Wim LeersWoah … #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.
Comment #41
Wim LeersOhhhhh I just found #2907402-13: HAL normalization of file fields don't provide file entity id or file entity REST URL! You didn't mention it in #39, but you basically moved most of the old patch to #2907402-13: HAL normalization of file fields don't provide file entity id or file entity REST URL!
Comment #42
Wim LeersThe
_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? 🤔
Comment #43
Berdir@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.
Comment #44
Wim LeersHm. 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 … 🚢
Comment #45
alexpottAs 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
Comment #46
BerdirI've finally updated the issue summary. Also created a follow-up for #3099151: Add a requirements check to BC layers behind configuration settings.
Comment #47
BerdirConflicted with the fixed deprecation format. Trivial reroll of a technically trivial patch, so keeping at RTBC.
Comment #48
alexpottIs there a reason why we can't add an @trigger_error() to this method
Comment #49
BerdirAdded a @trigger_error() and updated the BC test.
Comment #50
BerdirTests 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? :)
Comment #51
alexpottCommitted 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.
Comment #54
alexpott@catch +1'd the backport.