Now as the new entity field API got committed, we need to convert existing entity types to make use of it. See #1346214: [meta] Unified Entity Field API and the "Entity Field API" tag.
#1847596: Remove Taxonomy term reference field in favor of Entity reference is postponed on this
Remaining tasks
Follow-up Issues
- #1962806: Remove Drupal\taxonomy\Tests\HooksTest and taxonomy_test.module
- #1962808: Move static $propertyDefinitions into FieldItemBase class
- #1965510: Fix punctuation in Taxonomy module tests' assertion sentences
- #1981270: Clean up depth/parent/parents of taxonomy term
Related Issues
- fix part #33 #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities
- this issue depends on #1862758: [Followup] Implement entity access API for terms and vocabularies (? See #34)
- #1847596: Remove Taxonomy term reference field in favor of Entity reference
- #1975066: Forum terms do not respect/expose term description format
- #1751210: Convert URL alias form element into a field and field widget
- ?
Comment | File | Size | Author |
---|---|---|---|
#89 | 1818560-termng-89.patch | 8.46 KB | andypost |
#61 | d8_term_ng.patch | 146.49 KB | fago |
#61 | d8_termng_interdiff.txt | 2.11 KB | fago |
#57 | drupal-1818560-57.patch | 146.17 KB | dawehner |
#57 | interdiff.txt | 1.09 KB | dawehner |
Comments
Comment #1
dsnopekI think this is also D8MI related because it'll be necessary in order to translate Taxonomy terms, right? Tagging for now...
Comment #2
chx CreditAttribution: chx commentedComment #3
BerdirSo, @chx and me managed to start on this on parallel. Here's my patch, doing a full conversion, not using the BC decorator.
Most taxonomy tests are passing, even some fun ones like taxonomy term antonyms. Not completely through yet with everything.
- Changed the class and controllers to NG, defined properties.
- Changed all ->tid to ->id(), there are probably some wrong ones in there remaining, especially in forum.module.
- Some more ->name to ->label() changes that we missed originally
- After that, there's not much else other than description and format (which we want to combine in a single property later I think but that can probably be done in a follow-up as it will also require changes in the storage)
- Included the fix from #1862758: [Followup] Implement entity access API for terms and vocabularies that is now required as we need the bundle.
- taxonomy_get_tree() becomes an even bigger mess. We really need some sort of partial/lazy loading there.
Work in progress patch for @chx :)
Comment #5
BerdirAh, forgot a debug in there.
Comment #7
BerdirAnd fixed that id(). Let's see if there are any more of those.
Comment #9
BerdirComment #10
BerdirHad a lot of fun with forum.module, taxonomy_get_tree() and taxonomy terms re-ordering today. Still some broken things there, mostly related to parent handling. There's a mix with ->parent and ->parents which is used by forum.module and I don't see through that completely yet.
Comment #12
BerdirCan remove the get_tree() call here.
While this actually worked surprisingly easily I'm not sure if we should be doing that. We have separate tests now to make sure that the alter hooks work as part of the entity tests (for various entity types) and this should be done using a field, not custom glue code.
I'd suggest to keep it here an then open a follow-up to remove that (probably the whole module I don't think there's anything else there) and the corresponding test.
We seem to be doing this in various, inconsistent ways, should probably add a helper function for generating this array.
This is a direct query, shouldn't use id()
I think I need to keep this, instead of the default value, this currently forces it to 0 and therefore deletes parents when it's not updated.
Or actually make it a properly computed field and load the parents when saving. Not so cool for performance, though, especially when saving a lot of terms (weight reordering)
Wondering if we should move this function into the storage controller then it could be switched with a different implementation easily.
I broke this, overwriting the term now.
Comment #13
fagoAmazing progress. berdir++
Yes. I think that any access to the storage should go via the storage controller or EFQ. IF you need to make custom queries, add a method to the storage controller. Properly done, you'd have to add a new interface also.
I'm not sure I got this, let's discuss on IRC.
Comment #14
Berdir- Fixed some of the things pointed out above
- Added a PathItem for path.module.
- Fixed parent handling, back to only setting it to 0 for new terms explicitly and only changing it if it's not NULL. What's a bit strange there is that I had a case where the first offset was 1, not 0, the same as the term id. I think that's because the select values are directly passed to setValue(), wondering if we should default to re-order the items? I think field API does that too.
All failing tests above were green for me...
Comment #16
BerdirRe-rolled, can't see anything different in the patch other then the removed debug that I forgot to remove before.
Comment #18
fagodouble-post
Comment #19
fagoand the wrong issue... *ignore pls*
Comment #20
BerdirThis should fix the last test fails, mad a rather dumb mistake with that breadcrumb thing ;)
Comment #21
dawehnerJust wondering whether the storage controller should be accessed directly?
Static, but probably protected? This is also not done in other places of FieldItemBase, so we can't change it here? (Just curious).
This could use Drupal::service.
A potential follow up to use $term->uri(). Not sure about the other two cases below.
Drupal::service()
Do we have a standard how to document overridden properties?
Would be cool to explain why these values are unset. This might be not trivial for people not knowing EntityNG etc.
Shouldn't this be $term->name->value now?
This wouldn't allow people to use hook_entity_create and set a default parent value, I guess that's okay?
Doesn't isset() already return FALSE, if it's NULL? It has been different on array_key_exists
Nitpick alarm: Missing space after "(int)".
Why not simply call $term->delete() ?
Comment #22
BerdirTagging.
Comment #23
Gábor HojtsyTagging for D8MI topic language-content.
Comment #24
BerdirThanks for the review! Originally thought to wait with this on the ER taxonomy reference issue but it looks like that one is a bit blocked. So let's continue here.
- Used Drupal::entityQuery() to load the terms based on the vid.
- Did not change PathItem. I'm actually not sure what that thing isn't defined in the base class and I'd like to keep it consistent until we do.
- Changed to Drupal::service()
- Documented $values
- The init() thing is described in the parent implementation a bit. Not sure about duplicating that everywhere. And it might go away because those properties might go away if we switch to interfaces (it will be useless then, we only have the public properties for IDE integration).
- "Shouldn't this be $term->name->value now?" taxonomy_get_tree() in some cases still returns stdClasses. Will probably have to get rid of that and instead hope for lazy loading entity classes to happen.
- Removed unecessary NULL check, you're right.
- Changed to ->delete(), those were global search & replaces.
Comment #25
BerdirAlso created the following related/follow-up issues:
- #1962806: Remove Drupal\taxonomy\Tests\HooksTest and taxonomy_test.module
- #1962808: Move static $propertyDefinitions into FieldItemBase class
Both can be done before or after this issue and are nice Novice issues I think.
Comment #27
BerdirThis should fix that test.
Comment #28
YesCT CreditAttribution: YesCT commented#1847596: Remove Taxonomy term reference field in favor of Entity reference is postponed on this (?)
updating issue summary
Comment #29
jibran#1847596: Remove Taxonomy term reference field in favor of Entity reference is major.
Comment #30
klausiComment does not match function name.
Comment #31
YesCT CreditAttribution: YesCT commentedObservations (nothing wrong)
there are many ->name to ->label
and ->name to ->name->value
there are many $forum->tid to $forum->id()
another common substitution pattern. $term['parent'] to $term->parent->value and weight too.
$term->name sometimes is $term->label() and sometimes $term->label?
-------------------
patch coming for:
http://drupal.org/node/1354#drupal
Needs a period at the end of the sentence.
"All documentation and comments should form proper sentences, use proper grammar and punctuation,"
this can be wrapped more.
"Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over..."
Also, Defaults to...
was clarified to be extra. I'm leaving it in though for consistency within the patch. #1916652: defaults to in 1354 doc standards for optional args
Question 1:
does this need to specify public vs protected?
adding newline before class closing }
just @todo (not @todo:)
http://drupal.org/node/1354#todo
Question 2: Does this need an issue? Is it blocking? does this depend on anything else?
maybe a novice follow-up to look for other asserts where the punctuation is missing from sentences.
check and make sure these are slotted already with an issue to change the case to Edit and Delete...
I quickly looked at #421118: [Meta] Standardize capitalization on actions I think we need a new sub-issue on that meta for taxonomy/term operations in the taxonomy module.
Not blocking this issue. It's unrelated to this patch.
check access callback standards.
http://drupal.org/coding-standards/docs#menu-callback
"In the first line, declare what type of callback it is (Page callback, Access callback, etc.), and give a short description of what it does (like you would for any function). Total line length: 80 characters, ending in "."
At the end, include an @see line that points to the hook_menu() implementation where this callback is registered. If multiple hook implementations use the callback, include one @see line for each."
Question 3:
I can't figure out what the @see might be. I dont see a hook_create_access(). Maybe taxonomy_menu() ?
--
About #30:
core/modules/node/lib/Drupal/node/NodeStorageController.php
Hmm. It's spelled Database.. not DataBase..
is it worth a follow-up to fix that everywhere? (not this issue's fault)
For now, based on line 727 of core/lib/Drupal/Core/Entity/DatabaseStorageController.php:
I changed it to
Comment #32
BerdirThis is already part of a critical meta issue, there's not much point in making this major too :)
Comment #33
Wim Leers#31:
AFAICT this is implicitly answered by #1962808: Move static $propertyDefinitions into FieldItemBase class: we shouldn't change it here, because many other
FieldItemBase
subclasses do it the exact same way. We should stick to "the standard" (insofar as it is one) and change all of them at once, if any.This has been pointed out & answered before already.
RE: "novice follow-up to fix punctuation": done, #1965510: Fix punctuation in Taxonomy module tests' assertion sentences.
Question 3: fixed.
#32: This issue already is major :)
(I'm coming at this from the angle "let's make in-place editing also work on non-Node entities, e.g. Taxonomy Terms. Since Edit already depends on
EntityNG
, and Taxonomy Terms are not yetEntityNG
, I can't work on that without this issue being solved.)First: I cannot find any nitpicks… that must be a first :D
Second, code review:
We're changing this function's signature, but in the function description it says this:
Shouldn't that be updated as well? I'm not sure how to update it correctly, because it is currently pretty confusing: comments and code somewhat contradict each other.
The handling of
description
is changed significantly here. Is that really correct? I do see further in the patch that for taxonomy.module,check_markup()
must be used, but if forum.module usedfilter_xss_admin()
before, then we should probably continue to do so.Do we really need to introduce this in this patch? It seems like a change that is only tangentially related to what's the core goal here?We do need this, because we need the Taxonomy Term "extra fields" to be registered as proper Entity Properties, and then this is necessary.
This is ok.
A conditionally existing entity field? That seems very bizarre?However bizarre it may be, it seems that this is correct. Path module alters Taxonomy module to allow a custom alias for each term to be set right in the term's form. Hence it must also update the list of Entity Properties, and that's what this does.
It feels very dirty to be modifying Taxonomy Term's Entity Properties from another module, but that's how this has worked for quite some time, this is not the appropriate issue to change that.
This is ok.
One is "default', other is 'not specified'. They've currently got identical values, but I wonder if this is conceptually correct.
1) Previously, the parent method was called first. Now it's called last. Why?
2) The comment was removed. Restored.
I was going to question whether it is necessary to create an alternative to
entity_page_create_access()
, but sadly, it is. This is fine.Finally: manual testing found no problems, with one big exception. I have added an image field to my Taxonomy Terms, and I can still create and edit terms with images, but, upon rendering, the image is not rendered correctly.
Before:
(on the term page)
(in a view)
After:
(on term page)
(in a view)
When looking at
$item
intheme_image_formatter()
(on the term page, because in a view it doesn't even get to that function:Before:
After:
(on the term page
Nodes are also EntityNG, and there this problem does not occur (an image field there works fine).
Comment #34
BerdirWill need to look at the image problem. Wondering if it has to do with #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities.
The access related part are from #1862758: [Followup] Implement entity access API for terms and vocabularies, they are not really related to this issue but required for it.
Comment #35
Wim Leers#34: I tested the first patch in the first issue you linked to, unfortunately it causes further breakage: #1957748-20: hook_field_prepare_view() is passed empty field values for EntityNG Entities.
Comment #36
Gábor HojtsyOne more D8Mi tag.
Comment #37
jessebeach CreditAttribution: jessebeach commentedThe patch in #33 doesn't apply since (8320fdc2d55b2468b09d1246ee8f03224322ee2d).
The file
EntityReferenceAutocompleteTest.php
was updated to reflect the use of entities for taxonomy terms.It appears that the changes to this file are no longer necessary. I've updated the patch and attached the git-apply rej file to show what I left out.
Comment #39
BerdirThanks for the re-roll, yes looks like that is no longer required. Also fixed a conflict in EfqTest that you overlooked and a new one in forum.admin.inc.
Comment #40
jessebeach CreditAttribution: jessebeach commentedThis is a tough one to keep up to date!
Reroll.
Comment #41
Wim LeersI believe the problem I encountered and explained at length at the bottom of #33 is solved by #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities?
Comment #41.0
Wim Leersadded follow-up issue section
Comment #42
Wim LeersTesting this again; I wanted to check if my understanding in #41 is correct, and it is :)
This patch functions absolutely fine, but there is a bug in Field API for EntityNG handling, which is fixed by #1957748: hook_field_prepare_view() is passed empty field values for EntityNG Entities.
BUT there are a few small things to be fixed or clarified in this patch, that I noted in #33. Here are the things that still need to be fixed:
Almost there! :)
Comment #43
Berdir- The entity_page_create_access() related stuff should be discussed/added to #1862758: [Followup] Implement entity access API for terms and vocabularies, if we need to change it, it needs to happen there.
- the description thing in forum.module is IMHO a bug. it's the term description, and the term description is supposed to be rendered with the associated term description format. Not sure if this is even security related, in fact...
- The language default value is copied from Comment.php, where it is exactly the same. I think this is correct like this. I don't fully understand this, actually :)
- The difference is that it is now easier to change $values before passing it to parent::create() instead of altering the generated entity
Comment #44
Wim Leers3 out of the 4 answers are satisfactory.
Yet when you look at
admin/structure/forum/edit/forum/4
, it is impossible to select a text format! Not before, not after the patch. IMHO this change should be reverted and if we want to change this, it should be in a separate issue.Comment #45
fagoI had another look and I must say that this looks already pretty good. Here are some comments:
Off-topic for this conversion, but I think that shows that we should allow computed fields to have insert/preSave/update methods. See related discussion over at #1969728: Implement Field API "field types" as TypedData Plugins.
Minor, but should be $term->langcode->value imho.
Could that be replaced with if (isset($entity->parent[0]->value) ?
Comment #46
Gábor HojtsyWim: looking at the pre-existing taxonomy schema, taxonomy terms have a format property as the format of their description. While forums don't expose the description as a field with format, they store as taxonomy terms (with a NULL value for format). When that is passed on to check_plain() the fallback format is used, which is somewhat right by default. I agree unless the UI is not modified on forums to allow for format specification (which would likely require some upgrade path, heh), we should avoid doing this.
Comment #47
BerdirOpened #1975066: Forum terms do not respect/expose term description format for the forum problem.
Re-roll, changing to $term->langcode->value.
Using parent[0] is not possible, unfortunately, see the comment above. When set through the form, the keys of that list are set based on the term id, so it might be $term->parent[1], or $term->parent[35764]. Not sure if Entity\Field.php should enforce 0,1,2,3... for the field item id's (delta in field api speech), possibly. But that seems like a separate issue.
The access callback got commited, anything left to prevent an RTBC here? :)
Comment #49
Wim LeersOnce this passes tests again, I'll RTBC :)
It's a bizarre failure. Re-testing.
Comment #50
Wim Leers#47: taxonomy-term-ng-1818560-47.patch queued for re-testing.
Comment #52
BerdirAh, that's a new test, not so bizarre :)
Comment #53
Wim LeersWoot! :) Thanks, Berdir! :)
Comment #54
fagoAwesome work!
I see. I'm not sure we can enforce that, but at least we should clean-up a mis-use like that. But yes - separate issue.
Comment #55
dawehnerSome oddities I came up with while rerolling the patch:
is there a reason for that? (I couldn't find an answer in this issue)
That's one place where parents vs. parent comes up.
Comment #57
dawehnerFixed two small points.
One problem which currently exists is that $term->parent->value is empty, as the comment suggests. What is the proper alternative way to load it?
Comment #58
fagoHm, realized there is a another problem with $term->parent being keyed wrong. Instead of working around it at multiple places, shouldn't we better fix the UI to properly write the list? (also see #47)
Indeed. That's quite a mess, but it's not introduced by that patch. So I think it's better to clean it up in another issue. $term->depth actually is something that differs by parent, so it would belong more into $term->parents[X]->depth.
Comment #59
fago#57: drupal-1818560-57.patch queued for re-testing.
Comment #60
xjmComment #61
fagoUpdated the patch to fix $term->parent being keyed wrong. I figured the form controller overrides submit() instead of buildEntity() also, so fixed that as well.
Interdiff + updated patch attached.
Comment #62
dawehnerI'm wondering whether we can use somehow ->parent here as well?
Comment #63
fagoYes. But while it doesn't look so in the patch, when you dig through the code there are lots of ->parent vs ->parents usages left. As this has not been introduced here, I agree with berdir that it's better to postpone cleaning this up to another issue.
Comment #64
dawehnerSure I have seen that. Cool.
Comment #65
andypostOnly path module related changes seems out-scope. The same functionality needed for nodes so this issue require a follow-up to unify usage of path aliasing on nodes and terms
Maybe better split this 'path-related' hunks to separate issue seems nodes needs this too
Comment #66
andypost#61: d8_term_ng.patch queued for re-testing.
Comment #67
BerdirThe path part is required. I would not have done it here if it wouldn't be. Nodes are still using the BC mode, that's why that (and a million other changes) was not *yet* necessary.
Comment #68
Wim Leers#65: what #67 says, and what I said in #33, where I initially thought the same:
Comment #69
andypostI just tell about follow-up to refactor this type-data
EDIT related issue for path as field #1751210: Convert URL alias form element into a field and field widget
Comment #70
xjmComment #71
xjmComment #72
jhodgdonRegarding the path change, can someone comment on whether Pathauto would still be able to function with this patch? Pathauto would need to be able to:
- Override the path editing area (add a "use automatic" checkbox)
- Override the node save behavior (create an automatic alias if the box is checked)
I'm just not clear on what this change means for the Path module and whether Pathauto would be able to do the same kinds of changes it has done with past versions of Drupal.
Comment #72.0
jhodgdonadded related issues which solve part of this
Comment #72.1
YesCT CreditAttribution: YesCT commentedadded related issues that have been found while working on this. maybe could use some clarification if any are blocking this, or are truely follow-ups (postponsed on this)
Comment #73
YesCT CreditAttribution: YesCT commentedupdated issue summary.
Also adding tag to manually test #72
Comment #74
BerdirYes, there will be various ways how that will still be possible. The only change here is that we make the properties on the entity explicit, the same form/storage alter hooks can still be used to define what actually happens with it.
If we, as suggested by @fago in #45, introduce methods for computed properties to act on save, then pathauto might even be able to simply alter the typed data definition and use a different class.
Comment #75
BerdirRemoving the tag was a crosspost. But, it's not possible to manually test #72 without porting Pathauto to 8.x first ;)
Comment #76
fagoYep, this doesn't patch does not limit pathauto in any way.
Comment #77
jhodgdonThanks! Back to lurk mode. :)
Comment #78
webchickIck! :)
This was not introduced in this patch, but let's get this moved over to a Drupal::path() method in a follow-up.
This made me do a double-take, but "vid" here refers to "vocabulary ID" not "revision ID." Still, it's pretty hard to parse out that this is what bundle() is going to return. Berdir pointed me at #1233394: [Policy, no patch] Agree on a property naming pattern where we're discussing better DX around this. +100.
It's always been beat into my face that we shouldn't type-hint with objects but rather with interfaces. Unfortunately, term interfaces don't exist yet, so this will have to be adjusted in #1391694: Use type-hinting for entity-parameters.
I really dislike that this hard-codes path module's hook_entity_field_info() implementation to taxonomy terms. I would expect that to be something like "foreach ($entity that exposes a user-facing path property...)"
Berdir pushed back and noted that currently this isn't technically introducing a regression, since path module hard-codes a hook_user and hook_taxonomy implementation, so this is just porting over already nasty code. I guess. :\ Still, let's make sure a "major" follow-up exists to get this cleaned up properly. And I don't think #1751210: Convert URL alias form element into a field and field widget is it (maybe with re-scoping), because that's turning URL aliases into a user-facing field, not an entity property.
Yee-uck! Berdir says this is for IDE nice-ity and something we do in all other EntityNG entities. Something we should be able to clean up further with #1391694: Use type-hinting for entity-parameters as well.
Other than that, this all looks like good and fine stuff. I'm willing to commit this as-is, but we still need profiling, so needs work for that.
Comment #79
BerdirThanks, I was initially a bit worried when I saw the needs work but this doesn't look too bad ;)
What do you want to have profiled? I created a node with 24 terms (A-Z ;)) and I'm seeing a regression of 4-5%, both with xhprof and ab. taxonomy_get_tree() in some cases will be quite a bit worse than it already is but if we want to make terms properly translatable, we don't really have a choice (We need a loaded term that we can call label() on so that we get the right language) We need to find other ways to catch up, lazy-loaded terms might help or making that function a service that can be replaced with a cached/more performant solution when necessary.
There's not that much point in profiling every single NG conversion issue except to check if there are obvious problems with the actual conversion (which I can't see here). What we need to do is complete the conversion, throw out the BC mode, complete the few issues that we are working on that will improve EntityNG performance and then do some profiling and see what we can optimize. And make persistent entity load and render caches work :)
Comment #80
Wim Leers#79: I think you addressed everything, besides a major follow-up for
Comment #80.0
Wim Leersadded remaining tasks
Comment #81
fago+1
Comment #82
klonosWe can do profiling but instead of have it block commit perhaps we should simply tag fixed issues with something like "performance regression" and "revisit" if there is serious regression. This way we can do a final profiling and try to optimize things that actually caused the regression (if this regression is still an issue once everything is in place).
Comment #83
webchickOk checked with catch, he agrees with the assessment in #79. One moment.
Comment #84
webchickCommitted and pushed to 8.x. Thanks! :D
Needs a change notice most likely, and let's get that file-up followed for path.module.
Comment #85
Wim LeersHuzzah! Awesome work, Berdir!
Comment #86
BerdirReferenced this issue on http://drupal.org/node/1806650, added it there.
Created #1980822: Support any entity with path.module as requested. That will need a change notice once it's done, but not yet.
There are AFAIK no other API changes here, so I think we're good.
Comment #87
webchickRock, thanks Berdir! :)
Comment #88
andypostAt least here needed follow-up to clean-up introduced inconsistency and continue in #1233394: [Policy, no patch] Agree on a property naming pattern
there's no depth definition at all? so why this converted?
tid => id()
vid => vid->value || bundle() ?????
name => name->value || label() ??????
inconsistency
but here bundle changed back to vid without
->value
Comment #89
andypostThere's primary trouble in
taxonomy_get_tree()
which could return objects and entities depending on$load_entities
argument so it's not clear how to use$term->id()
once object is returned fromdb_select()
Also it shows that taxonomy badly covered with tests
Patch tries to fix depth, but also 'depth' needs definition in storage controller
Comment #90
andypostFiled follow-up #1980966: Refactor TermStorage::loadTree() to properly work with Entity
Comment #92
BerdirCan we do that in a follow-up issue that cleans up depth/parent/parents? depth could actually be different per parent, so it might make sense to make that a property of $term->parent->depth (and $term->parent[1]->depth), which in turn would be a computed field.
It's not as inconsistent as you make it.
vid()/id()/label() should be used when possible, in case of terms, there's the mess with taxonomy_get_tree() which this issue did *not* introduce, just used it in more places.
In e.g. an edit form for example, you don't want to edit the (possibly altered) label of a term, you want to edit the *name*, so, $term->name->value, not $term->label().
The last example there is because that class does a manual query to load terms, it was broken in HEAD, has no test coverage and this issue fixed it. Yes, it should use entity load/EFQ but that's outside the context of this issue.
Comment #93
andypostLet's fix it in follow-up #1981270: Clean up depth/parent/parents of taxonomy term
Comment #93.0
andypostupdate remaining tasks
Comment #94.0
(not verified) CreditAttribution: commentedUpdated issue summary.