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.
Comment | File | Size | Author |
---|---|---|---|
#35 | drupal8.path-field-item.34.patch | 4.01 KB | sun |
#32 | path-unification-1980822-32-interdiff.txt | 940 bytes | Berdir |
#32 | path-unification-1980822-32.patch | 4.08 KB | Berdir |
Comments
Comment #1
andypostFor 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
Comment #2
BerdirYes, 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.
Comment #3
andypost@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
Comment #4
BerdirNot sure if this is going to work. Enforcing NG as nodes are passed in as BC.
Comment #6
BerdirThis should result in a lot less exceptions.
Comment #8
BerdirFixing/simplifiying some code there.
Comment #9
fagoNow, 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.
Comment #10
BerdirYes, 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
Comment #11
Berdir#8: path-unification-1980822-8.patch queued for re-testing.
Comment #13
BerdirThe 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... ;)
Comment #14
andypostThe 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
Comment #15
twistor CreditAttribution: twistor commentedThis 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.
Comment #16
BerdirYeah, 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?
Comment #17
BerdirI 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?
Comment #18
Berdir15: path-unification-1980822-15.patch queued for re-testing.
Comment #20
vladan.me CreditAttribution: vladan.me commentedRe-roll #15
Comment #21
vladan.me CreditAttribution: vladan.me commentedComment #22
fagoList 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.
Comment #23
fagosee #2134079: Storage-related methods are not invoked for computed fields
Comment #25
Berdir20: path-unification-1980822-20.patch queued for re-testing.
Comment #26
sunLooks 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?
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?
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?)
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?
Comment #27
Berdir1. 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..
Comment #28
alexpottThis is now unnecessary as #1980822: Support any entity with path.module has landed.
Also needs work as we should use
getEntity()
Comment #29
jibranDid you mean #2134079: Storage-related methods are not invoked for computed fields?
Comment #30
BerdirRe-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.
Comment #31
jibranCan we add comment here about what is $this-> getParent()->getParent()?
Comment #32
BerdirNo we can't because I just forgot the update them.
Comment #33
Berdir32: path-unification-1980822-32.patch queued for re-testing.
Comment #34
sunRe-rolled against HEAD. Updated for $entity->getSystemPath().
Comment #35
sund'oh.
Comment #36
andypostAny reason to change logic?
Looks like updated alias is not saved, and if that's true we have weak tests
Comment #37
sunI stared at the code for a few minutes now, but I fail to see a difference in the executed logic?
Comment #38
jhodgdonRE #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.
Comment #39
andypostThe difference is:
My bad... elseif makes more sense!
Comment #40
BerdirI 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?
Comment #41
sunTo 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:
I raised this concern in #2141539-22: Rename FieldItemListInterface::filterEmptyValues() to filterEmptyItems() but was told that I'm wrong. ;)
Comment #42
yched CreditAttribution: yched commented@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 ?
Comment #43
BerdirMaybe 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.
Comment #44
sunIndeed, 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? :-)
Comment #45
BerdirYes, looks fine. Back to RTBC.
Comment #46
catchCommitted/pushed to 8.x, thanks!
Comment #47
BerdirYay.
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.