Opening this as a major follow-up of #1818560: Convert taxonomy entities to the new Entity Field API at webchick's request.

Right now, path.module has very specific storage and form alter hooks that integrate with and support nodes and taxonomy terms.

Previous attempts tried to make a path alias a field and support all entities through that. That has been blocked by UX issues.

The mentioned issue introduced a PathItem class as all data on an entity now needs to be explicitly defined. It did that add that field only to taxonomy terms as everything more would have lead to more refactoring not related to the conversion, which was a big patch already. We do want to use that to generalize support for all entities, however.

Two steps are relatively obviously towards that path:
- Use the class for node as well
- Use hook_entity_*() hooks instead of hook_$entitytype_*() (Sooner or later, that will also move to methods on the PathItem class

After that, I'm not 100% sure yet:
- How do entity types define that they want a path alias field? Possibly just define it themself, just like entity types now do with language? That pattern explicitly chosen to not make core entity types special and allowing contrib to do it in the same way..
- How do the form parts happen? The language stuff currently relies on some tricks to not fail if language.module is disabled. And we also want to keep pathauto in mind, which wants to extend everywhere the path UI is exposed on an entity. If we can make #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets happen (and we really want to), then the non-configurable path field could have a corresponding widget that would show up if the field is enabled and not if it's not. And pathauto could replace it with a different widget.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

For this kind of field we have a killer feature that needs re-roll #1751210: Convert URL alias form element into a field and field widget

Berdir’s picture

Yes, webchick was strictly against making it a field that's exposed to the user.

This is about making it a non-configurable (in the UI) field and contradicts with the other issue.

andypost’s picture

@Berdir she was against the UI that exposed in last patch #1751210-125: Convert URL alias form element into a field and field widget
At the same time I got similar problems within #1907960-154: Helper issue for "Comment field" for dynamic property

So looking at #1969728: Implement Field API "field types" as TypedData Plugins it seems not so hard to join forces of fields and typeData

Berdir’s picture

Status: Active » Needs review
FileSize
5.17 KB

Not sure if this is going to work. Enforcing NG as nodes are passed in as BC.

Status: Needs review » Needs work

The last submitted patch, term-unification-1980822-1.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
5.31 KB

This should result in a lot less exceptions.

Status: Needs review » Needs work

The last submitted patch, path-unification-1980822-6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
5.31 KB

Fixing/simplifiying some code there.

fago’s picture

Now, as we've the insert/update/delete callback methods in fields, can't we use them here? This would be a good clean-up and make the code re-usable as a side-effect.

Berdir’s picture

Yes, I started working on this but my first attempt wasn't very successful, it didn't want to call my methods ;) Something crazy with entity translation, which might now work better. The current patch is also part of #1939994: Complete conversion of nodes to the new Entity Field API

Berdir’s picture

Issue tags: -Entity Field API

#8: path-unification-1980822-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity Field API

The last submitted patch, path-unification-1980822-8.patch, failed testing.

Berdir’s picture

The current patch that still uses hooks went in as part of the mentioned issue above.

So the main thing left to do here would be to move those hooks to methods on PathItem. And think about who defines the field for whom. And maybe also implement loading the alias, to simplify form altering. And we should make it a widget. And... ;)

andypost’s picture

The only trouble with #1751210: Convert URL alias form element into a field and field widget is ability to limit addition more then one path-field to entity

twistor’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

This moved the hooks to methods. Looks like if the computed flag it set, then the methods won't be called.

I can't figure out how to get a widget to show up for non-field.module fields.

Berdir’s picture

Yeah, that doesn't work yet. See #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title. I don't know how generic the implementation there is already, haven't looked at the patch, but I assume it would be useful to test and give feedback?

Berdir’s picture

+++ b/core/modules/path/path.module
@@ -225,64 +225,12 @@ function path_entity_field_info($entity_type) {
-      'computed' => TRUE,
-      'list' => TRUE,

I get why you removed computed (although I'm not sure that computed not executing those methods is correct, seems like it's especially useful for those?) but why remove list?

Berdir’s picture

Status: Needs review » Needs work

The last submitted patch, 15: path-unification-1980822-15.patch, failed testing.

vladan.me’s picture

Issue summary: View changes
FileSize
4.37 KB

Re-roll #15

vladan.me’s picture

Status: Needs work » Needs review
fago’s picture

I get why you removed computed (although I'm not sure that computed not executing those methods is correct, seems like it's especially useful for those?) but why remove list?

List is added by default, so can be removed. The hooks should be invoked for computed properties - I agree. I suppose it's just missing because of the defaullt iterator.

fago’s picture

Status: Needs review » Needs work

The last submitted patch, 20: path-unification-1980822-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
sun’s picture

Looks very good - can we move forward here?

The part I do not understand is how this conflicts or contradicts with #1751210: Convert URL alias form element into a field and field widget — wouldn't a field-field implementation be able to use or re-use the PathItem class?

  1. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
    @@ -47,4 +47,57 @@ public function getPropertyDefinitions() {
    +      $entity = $this->getParent()->getParent();
    

    OT: What is the first parent? And why is it safe to assume that the second is the entity?

    Shouldn't there be a getEntity() method in field item classes?

  2. +++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
    @@ -47,4 +47,57 @@ public function getPropertyDefinitions() {
    +  public function delete() {
    
    +++ b/core/modules/path/path.module
    @@ -222,56 +221,6 @@ function path_entity_field_info($entity_type) {
    -function path_entity_predelete(EntityInterface $entity) {
    

    predelete is being replaced with [post]delete -- are we sure that we did not intentionally use predelete before?

    (or was that merely by means of consistency?)

  3. +++ b/core/modules/path/path.module
    @@ -214,7 +214,6 @@ function path_entity_field_info($entity_type) {
         $info['definitions']['path'] = array(
    

    Do we know what is going to break if the $entity_type = node|taxonomy_term conditions are removed in the path_entity_field_info() hook?

Berdir’s picture

1. There is a getEntity() method now, this was written before it existed. The first parent is the list.

2. Not sure if there was a specific reason, but there is no pre delete method for field item classes. I think delete is fine, it's not an actual reference, so it's ok to delete it afterwards.

3. Right now nothing, because the form code is still hardcoded to those two forms, there would just be an empty an unused path field on all entities. We're now however almost there to convert that into a widget but I still think we should still add it explicitly having an alias for custom blocks for example is really pointless :) What we might want to do is move the definition to node/term baseFieldDefinitions(), so that provide an actual example that contrib entities can copy from. Would need to be conditional there..

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/path/path.module
@@ -214,7 +214,6 @@ function path_entity_field_info($entity_type) {
-      'computed' => TRUE,

This is now unnecessary as #1980822: Support any entity with path.module has landed.

Also needs work as we should use getEntity()

jibran’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
617 bytes

Re-roll, removed the customized check and used getEntity()

Still leaves us with the question who should define the field, conditional in Node::baseFieldDefinitions() or in the hook.

And we should also be close to be able to use a widget to edit it.

jibran’s picture

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
@@ -45,4 +45,57 @@ public function getPropertyDefinitions() {
+      $entity = $this->getParent()->getParent();
...
+    $uri = $this->getParent()->getParent()->uri();

Can we add comment here about what is $this-> getParent()->getParent()?

Berdir’s picture

No we can't because I just forgot the update them.

Berdir’s picture

sun’s picture

Re-rolled against HEAD. Updated for $entity->getSystemPath().

sun’s picture

d'oh.

andypost’s picture

+++ b/core/modules/path/lib/Drupal/path/Plugin/Field/FieldType/PathItem.php
@@ -53,4 +53,55 @@ public static function schema(FieldDefinitionInterface $field_definition) {
+  public function update() {
+    // Delete old alias if user erased it.
+    if ($this->pid && !$this->alias) {
+      \Drupal::service('path.crud')->delete(array('pid' => $this->pid));
+    }
+    // Only save a non-empty alias.
+    elseif ($this->alias) {
+      $entity = $this->getEntity();
+
+      // Ensure fields for programmatic executions.
+      $langcode = $entity->language()->id;
+
+      \Drupal::service('path.crud')->save($entity->getSystemPath(), $this->alias, $langcode, $this->pid);
+    }

+++ b/core/modules/path/path.module
@@ -232,53 +232,6 @@ function path_entity_field_info($entity_type) {
-    // Delete old alias if user erased it.
-    if ($entity->path->pid && !$entity->path->alias) {
-      \Drupal::service('path.crud')->delete(array('pid' => $entity->path->pid));
-    }
-    // Only save a non-empty alias.
-    if ($entity->path->alias) {
-      $pid = $entity->path->pid;
-      // Ensure fields for programmatic executions.
-      $langcode = $entity->language()->id;
-      \Drupal::service('path.crud')->save($entity->getSystemPath(), $entity->path->alias, $langcode, $pid);
-    }

Any reason to change logic?
Looks like updated alias is not saved, and if that's true we have weak tests

sun’s picture

I stared at the code for a few minutes now, but I fail to see a difference in the executed logic?

jhodgdon’s picture

RE #37 - hah, I thought I was just tired last night when I was looking at comment #36. I don't see it either. Both of them are calling service('..')->save(...) as far as I can see, and it even looks like they are probably saving the same data. Presumably/hopefully the existing tests would fail if that were not the case.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

The difference is:

-    // Only save a non-empty alias.
-    if ($entity->path->alias) {
+    // Only save a non-empty alias.
+    elseif ($this->alias) {

My bad... elseif makes more sense!

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

I think there is one problem here, that was identified by sun in his signature issue. And that is that the field method will *not* be called when the field is removed. I'm not sure if we have test coverage for deleting aliases?

sun’s picture

To provide context for others, that issue is #1548204: Remove user signature and move it to contrib and the relevant lines of that patch are these:

class UserSignatureItem extends TextItem {
  public function isEmpty() {
  // @todo FieldItemList::filterEmptyValues() blatantly removes entire field
  //   property definitions (instead of just values), which causes update() to
  //   no longer be invoked if you empty out the user signature value.
  return $this->value === NULL;
}

I raised this concern in #2141539-22: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() but was told that I'm wrong. ;)

yched’s picture

@sun: I didn't say you were wrong, I just said that [#8281031] was not the issue to discuss this :-). That patch was merely renaming a method to match current nomenclature (Value / Item), without changing what it actually does.

Now, we do need a filterEmptyItems() method that does exactly what it currently does.

The issue here, as I understand it (but I might be wrong, I'm somewhat lacking D8 bandwidth these days), is rather that FieldItemList::update() / presave() / delete() base implementations simply loop over items present in the list at the time the call is made and defer to their own delete() / update() / presave() implementations. That was what we came up with with fago and effulgentsia back in Portland when figuring out how to merge the D7 "field type API" with the D8 Entity Field API.

Meaning, unlike the hook_field_[update|presave|delete]() field-type callbacks in D7 that received the *list*, this doesn't let you do a before/after diff at the level of the list, but only at the level of each item, which is not a good fit if you need to react to observe things like "values were a, b, c, now it's only a, b"

Field types that need such logic currently have to provide a dedicated ItemList class for their field type, that overrides the update() / presave() / delete() methods and implement logic there, at the "list of items" level, instead of item by item. That's exactly what 'file' field type does to track "files that the itemList no longer references" - see FileFieldItemList.

Not saying that it's ideal, and it might very well be that the API around FieldItemList / FieldItem ::update() / presave() / delete() is flawed, but at least there's a known workaround ? Is the use case here really different from file fields that need to track the list of referenced files ?

Berdir’s picture

Maybe there is no problem, because we only delete if there is a pid but no alias, so it wouldn't be empty. I'm not sure about the test coverage we have, but we should make that deleting an alias is covered, then we should be able to move on. Just wanted to make sure we check that first.

sun’s picture

Indeed, there is no problem here, because the PathItem happens to have an additional 'pid' property, which causes the field item to be not empty.

The functionality is actually covered by tests in Drupal\path\Tests\PathAliasTest::testNodeAlias().

RTBC? :-)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, looks fine. Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Berdir’s picture

Yay.

To actually make this re-usable for other entity types now, we need to do #2201051: Convert path.module form alters to a field widget.

Status: Fixed » Closed (fixed)

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