Normalize path fields as part of the entity: allow REST clients to know the path alias

I think it belongs in path, with a service provider, but let's see if we can get consensus first. There's a sample service provider in file entity module (contrib) that does the same thing.

Patch looks great.

+++ b/core/modules/path/src/PathServiceProvider.php
@@ -0,0 +1,33 @@
+    if (isset($modules['hal']) ) {

Why only for hal, and not for other serializations?

#2609824: REST: how to set an entity's path alias when POSTing, and how to get an entity's path alias when GETting asked about this too, both for JSON and HAL+JSON.

Progress FTW.

+++ b/core/modules/path/src/PathServiceProvider.php
@@ -0,0 +1,33 @@
+    // Only register the normalizer if HAL module is installed.

This needs to say why - i.e. that the normalizer extends the HAL FieldNormalizer.

For a minute I was going to say this should live in HAL module, since alias storage lives in \Drupal\Core, but then it depends on PathFieldItemList which is in path module, so we'd still have an optional dependency either way.

Part of me is thinking about a path_hal module with a dependency on both, but then that clutters the module list a bit.

Also another way to get path aliases for entities would be to have path aliases themselves exposed to REST, then you could look up aliases by the system path for the entity and get it that way - which would work regardless of the internals of whether a site has path module itself installed or not.

The path field item makes sense in terms of cleaning up the form integration of path module, but URL aliases really aren't a part of the entity as such in the way they're stored at the moment, so moving back to CNR to discuss a bit more.

Well, is there no way that field items could specify that even they are computed, they should be normalized?

#14 is a great question. Who would know the answer to that?

I accidentally created this duplicate issue #2693077: Path is not returned in REST response.

The real problem is, is that the data is never retrieved from the database (or rather, it is, but only when you load the widget). I think it would be better to get the data from the database when the field/entity is loaded. Doing it that way exposes the data to all of the existing systems (Form API, REST API, Serializes, Caching, etc.) for free.

Perhaps we could listen to entity load, check to see if a field of type "path" is on it, and add it to the loading?

I really wish there was something like getComputedValue on \Drupal\Core\Field\FieldItemListInterface that would be called when the entity is loaded.

But since that doesn't exist (afaik), then I think we should use hook_entity_storage_load() to also load the path (only when an entity has the path field, which may be under a different name).

Attached is a patch that should fix the issue, did this again 8.0.x since I think it's a critical issue, but I can merge 8.1.x or 8.2.x if that is more approriate.

Since the values are already set by the time you load the widget, I cleaned up the widget a bit. I also tested this and it seems to work perfectly in xml, json, and hal_json

Whoops... should be continue not return.

Add the langcode.

Add the language to the query conditions.

Add defaults to hopefully resolve the test failures. :/

Default alias value is an empty string.

Well, I'm not sure why the test is failing, so if someone smarter than me could take a look at it, I would greatly appreciate it. :)

I don't understand, loading the path is what should happen since it's a "field". All fields are loaded when he entity is loaded. Doing this resolves this issue.

If you have a separate normalizer, you are tying the path to the form and to the normalizer, but not the entity object itself. That means anywhere in Drupal you want to modify the path, you have to look it up yourself, rather than just have it on the entity like every other field.

If we didn't want this behavior, we shouldn't have made it a field in the first place.

Also, I don't know how you're going to get out of the caching issue. If you add it to a normalizer, it will be cached with the request, but not added as a cache dependency (as if it would if it was already on the entity). Because of this, if I were to go in and modify the alias directly (and not the entity itself), the cache will not be invalidated and the wrong data will be returned to the user.

I guess if you also want to override the REST Resource for any entity that has this field, you could do that (or add a check in the rest module), but all of that seems like nonsense, just add it to the entity and be done with it.

Yes, it causes an extra database lookup, but there are few instances you don't need the alias (imho).

And... if someone really doesn't want the extra database lookup, they can just remove the base field from the entity. You'll still be able to have aliases, it just wont be directly tied to the entity itself.

Oh I see, well either way, I don't want to take over this issue, I just think this is a better solution is all. If it really belong in another issue that's fine, I can make another one, but I don't see why.

I really don't think we should further entangle path aliases and entities, and said the same in #13.

Then why is it a field in the first place?

Are we really ok saying that a path field value, must always be retrieved by the module/application that needs to modify it?

For instance, if I load a node from the database, the only way to modify the path field is by then loading the path with the path storage. Should other computed fields do this as well?

I just don't think it should be a field at all if it's not going to be loaded with the entity. We should at least remove it from the entity object (some how?) so there isn't an expectation of it having a value. At the same time, if it's not returned with the entity, then it should not be updated with the PHP or REST API.

Like I said, ideally, there would be a way in Drupal core to dynamically load computed fields when they are needed, but I don't think that API exists. :(

So, if we don't load it with the entity, then I think we need to stop making it a field all together. We can have the form just be a standard form alter that is added to nodes and taxonomy terms (and any other entity that wants it) and then we'll make a REST endpoint to retrieve a url alias by source (which is inconsistent with the Entity REST API, but I don't see a way around it).

The choice is yours, I think it's gross either way, but adding it to the entity seems to be the lesser of two evils at this point.

Then why is it a field in the first place?
...
So, if we don't load it with the entity, then I think we need to stop making it a field all together. We can have the form just be a standard form alter that is added to nodes and taxonomy terms (and any other entity that wants it)

The issue that changed this from a standard form alter is #1980822: Support any entity with path.module - that and related issues has most of the history. I fully agree that what we have currently is a half-measure, I just don't think making it a full-measure is necessarily the right direction.

and then we'll make a REST endpoint to retrieve a url alias by source (which is inconsistent with the Entity REST API, but I don't see a way around it).

That's what I suggested earlier in the issue. Given that we support non-entity path aliases, and that entities can have multiple path aliases in both different languages and the same language, we might need that anyway for a complete REST API.

There's also #1553358: Figure out a new implementation for url aliases open which could change all of this again if it ever happens.

The choice is yours, I think it's gross either way, but adding it to the entity seems to be the lesser of two evils at this point.

I agree it's all gross, but that's why I tried to push back a bit before pushing ahead with this implementation. I don't know which I really think is the least worst at the moment.

Also loading the path alias into the entity means extra i/o on entity loads, for something which is never, ever accessed as a property of the entity except for providing default values in forms and REST requests. Not keen on that either. Also can't usefully take advantage of entity caching since path aliases can be changed via the path alias UI independent of entity CRUD.

Also loading the path alias into the entity means extra i/o on entity loads, for something which is never, ever accessed as a property of the entity except for providing default values in forms and REST requests. Not keen on that either. Also can't usefully take advantage of entity caching since path aliases can be changed via the path alias UI independent of entity CRUD.

Is there a way to make something a cache dependency of an entity (like a page cache)? (I'm assuming the path item can be a cache dependency?). Even if we make a custom serializer, we have the same issue (but perhaps it's easier to solve).

I think it actually makes sense to have the alias as a computed field, but it seems like Drupal is missing some key APIs for computed fields (i.e. only retrieving the alias when needed, storing the computed value in cache, as a dependency, if possible, etc.). I thought about overriding getValue() method on the List class, but that didn't seem like a good idea. :/

I mean, in reality, you should be able to have a computed field be anything from 1+1 to an HTTP Request to an external service and Drupal core should take care of everything else as if it were a standard base field, for you (imho).

Also, I think a good rule of thumb is "API First" and the way path module is now, it's not API first at all; so either way we go, I think that's what we need to go for.

Since I don't think we can remove the field (and technically "break" the API, which is already broken imho).

Here's some possible solutions:

• I/O We could alter the entity query and also query for the path when queering for the entity. This would make the path alias similar to a field (another left join?).
• Cache When someone creates or updates an alias, we could lookup the entity by the source and clear out the entity cache for that entity. This is the same thing you'd do if you were to allow someone to modify a field independent of an entity, you'd have to get the parent entity and clear the cache for it.

The only alternative that I can see is that we change path field to store the pid. That would return the pid when you load the entity (it would be on the base table). Also, you'd have it available to look up via REST or other API's and you could easily look it up for the form, etc. Also, don't have to worry about cache (other than a delete), but that's the same thing that happens for entity reference. I don't really like this idea, but I think it's better than not loading the pid at all when the entity is loaded.

Reroll

Drupal 8.1 version of the patch.

Another way we could do this, is just add the pid to the entity. That way the entity's cache is not dependent on any changes to the alias.

Rerolled against 8.3.x

I completely agree with all of the arguments that @davidwbarratt outlines: either it's a field and then it should be normalized (which it is not today), or if it doesn't get normalized, then it shouldn't be a field.

That being said, @catch is also right in two important aspects:

1. Multiple aliases for a single entity: entities can have multiple path aliases in both different languages and the same language, we might need that anyway for a complete REST API.
2. Path aliases don't have cache tags, so impede caching: Also can’t usefully take advantage of entity caching since path aliases can be changed via the path alias UI independent of entity CRUD.

I think the patch can be updated to support point 1. Marking Needs tests for that.

However, updating it to support point 2 is going to be very difficult: it's essentially blocked on #2480077: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags.

Per #17, #2693077: Path is not returned in REST response was marked as a duplicate of this issue. Adding that to the list of related issues.

Berdir commented at #2480077-21: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags:

Sure that this is the correct issue reference?

What you need is first #2539634: PathItem::delete() never runs because the path field type is a computed field in disguise and a follow-up of that to actually compute/load the alias when accessed. which is the better approach than what that other issue is doing, I don't want to always load them, only when necessary.

As usual, Berdir is right. It looks like that could be the better solution.

Ok, this patch takes into account there might be multiple paths for translations. Probably still needs work but wanted to get this out to get feedback.

The main thing I have to think about when I review #59's interdiff is: we really need test coverage for all the edge cases, because this code is not at all trivial. i.e. single alias, single translated alias, multiple aliases, multiple translated aliases, and combinations.

The second thing I have to think of: We really need Berdir to review this!

Ok this patch fixes the test fail PathLanguageTest

Basically we have to check if the when dealing with a new translation that we don't use the existing pathitem from the source.

@Wim Leers agreed on the need for test coverage.

Assiging to myself. Working on fix.

Ok the test failures in #62 I believe because I was not testing for a translatable entity type with an entity that was not translatable. Now I am instead checking if the actual path field is translatable.

Yes, not sure about doing this on storage_load. That means that information will be cached together with the node, so when you edit it in the admin UI, the edit form will still show the old values. And it also does additional queries for every node that is being loaded, although we will most likely *not* use those values.

What we need is to be able to load it on-demand when the values are accessed. Problem is that this is currently a bit complicated and we have no proper API for it (also due to get() vs. __get()). #2413471: FieldItemBase must check property definitions instead of already instantiated properties might help with that, need to review that.

Postoning on #2413471: FieldItemBase must check property definitions instead of already instantiated properties.

I updated the patch there that should fix the test failures. Otherwise seems like simple fix.

Unassigning me, Berdir's comment in #67 covers my objections. I'd also add there's #2336597: Convert path aliases to full featured entities open and likely to be revived as part of the workflow initiative, so we should bear in mind what things would look like after that too.

After applying patch on #65, I received the following error on "node/add/{bundle}" pages:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getTranslationLanguages() on null in Drupal\path\Plugin\Field\FieldWidget\PathWidget->formElement() (line 78 of core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php).

After looking around, node/add/{bundle} page's don't have $entity->id() for $unchanged_entity to be set to, leading to a "null" value and the subsequent error.

So I've added a check for null and patch to solve.

Here is a new (rough) patch and approach. I'm not sure we need to postpone on #2413471: FieldItemBase must check property definitions instead of already instantiated properties. We don't need the field item as such? As path fields are a really special kind of snowflake right now, we can just adda normalizer for the PathFieldItemList instead and lookup the entity alias directly from the parent (as I think catch was getting at above somewhere).

1. Path is a computed field now, so there should always be a PathItem now.

Thanks both for your input.

@berdir, yes that's correct, however \Drupal\hal\Normalizer\FieldNormalizer::normalizeFieldItems() checks first whether the field is empty or not, which returns TRUE for path most of the time...

@tstoekler:

1. Yes, pretty much, plus the comment above based on #73. So we have a field item but the field is considered empty still.
2. Hmm, we don't return all translation values for all fields, do we? Maybe just in hal? if so, we might need to just make sure the hal specific normalizer honours the current langcode from $context. As each translated field item will get normalized anyway.
3. Shouldn't denormalization get handled anyway if 'alias' is posted back and the field item is saved. The logic in PathItem looks to handle this.

But as mentioned, this is still rough, and has no tests either.

Yes, if we could use the item level again, we would get the hal translations for free. So we really just need to fix the isEmpty() issue then we could actually do that...

Here is an updated patch to make it use field item level, and check $context for a langcode (this is a generic pattern, but means it makes it work 100% with hal).

So this now leads to the question, could we have a smaller separate issue JUST for isEmpty() on the PathFieldItemList class, then postpone this issue on that?

@damiankloip I think you forgot to upload the new patch ;)

Haha, yes indeed!

+++ b/core/modules/path/src/Normalizer/HalPathFieldItemNormalizer.php
@@ -0,0 +1,24 @@
+class HalPathFieldItemNormalizer extends PathFieldItemNormalizer {

Why is not extending \Drupal\hal\Normalizer\FieldItemNormalizer? It seems that all the Hal specific normalizers that I have seen extend \Drupal\hal\Normalizer\FieldItemNormalizer.

Would there ever be a case where \Drupal\hal\Normalizer\FieldItemNormalizer gets update with change that should affect all Normlaizers that extend it for hal and this normalizer won't get the change.

+++ b/core/modules/path/src/Normalizer/PathFieldItemNormalizer.php
@@ -0,0 +1,37 @@
+    if ($entity = $object->getEntity()) {
+      $langcode = isset($context['langcode']) ? $context['langcode']  : $entity->language()->getId();
+      $alias = \Drupal::service('path.alias_storage')->lookupPathAlias('/' . $entity->toUrl()->getInternalPath(), $langcode);
+      // Mimic the field structure that would be returned if this field
+      // contained any items.
+      $normalized['alias'] = $alias;
+    }

I would suppose if the Hal normalizer didn't extend this class then this could be move to a trait.

I'm not sure what you mean entirely, the PathItem normalizer already just extends the FieldItem normalizer. Not sure I see the use of a trait here. Or did I miss something else? We would be using a trait but both classes would extend from the same base class?

@damiankloip sorry updated my comment. mentioned wrong class

Tedbow, thanks - makes more sense now! You're correct the current class hierarchy worked nicely for field level, not field item level. I'll update this.

Here is an issue which tries to solve things properly: #2846554: Make the PathItem field type actually computed and auto-load stored aliases

Let's postpone this on #2846554: Make the PathItem field type actually computed and auto-load stored aliases and see if we need anything after. Likely we will just close this one out.

That issue now provides full REST test coverage, so I think we can officially close this one.

I came here to do exactly that :) Thanks!