With the #1818556: Convert nodes to the new Entity Field API the initial conversion has been done, but most access to $node properties/fields still goes via the bc-decorator entity.
To complete the conversion we need to change node_load(_multiple())() to return the original (NG-)entity and fix all usages of node properties to use the new entity field syntax: $node->title
becomes $node->title->value
.
Proposed resolution steps
#1 -
most frequently accessed things are those that we already have methods for, methods that are the same for NG and BC: id(), bundle(), getRevisionId(), label() (already used in most places)... suggestion would be to do a first iteration and convert everything we can to those methods.
#3 -
We might have multiple passes in this issue and a commit for each pass. Another step, could be to have a single patch for each module integrating with Node to convert the reltated additions to actual entity fields.
#9 - make the issue as META and each conversion file as separate issue to make easy all big patches in queue to easy merge changes
Part of #1346214: [meta] Unified Entity Field API
Steps
- change node->nid to node->id()
- change node->title to node->label()
- change node->type to node->bundle()
Comment | File | Size | Author |
---|---|---|---|
#96 | 1939994-96.patch | 116.63 KB | Berdir |
#96 | 1939994-96-interdiff.txt | 2.23 KB | Berdir |
#94 | 1939994-94.patch | 116.75 KB | Berdir |
#94 | 1939994-94-interdiff.txt | 591 bytes | Berdir |
#92 | 1939994-92.patch | 116.82 KB | Berdir |
Comments
Comment #1
BerdirI've been thinking quite a bit about the best approach to tackle this.
What I've seen in the taxonomy term issue is that the most frequently accessed things are those that we already have methods for, methods that are the same for NG and BC: id(), bundle(), getRevisionId(), label() (already used in most places).
My suggestion would be to do a first iteration and convert everything we can to those methods. That's going to be a huge but for the most part trivial patch (global search and replace, fix the broken ones like empty($node->id()) stuff). That's a lot of code that we don't have to touch again and doing it in one go should be the least disruptive for other issues I think.
Comment #2
podarokagree with @Berdir about first iteration
Comment #3
plachYes, this soulds like a plan. We might have multiple passes in this issue and a commit for each pass. Another step, could be to have a single patch for each module integrating with Node to convert the reltated additions to actual entity fields.
Comment #4
podarok#3 another step looks like a META follow-up issue
EDIT
Updated summary
Comment #4.0
podarokUpdated issue summary.
Comment #5
plachSorry, I did not explain myself: #3 is not an alternative to #1. I just meant to have a patch for each module integrating with Node. But remaining in this issue, just to keep the patch size manageable.
Comment #5.0
plachUpdated issue summary.
Comment #6
podarok#5 didn`t post it like alternative
following up )
Comment #7
fagoAgreed, sounds like a good plan.
Comment #8
BerdirAlright, started working on a patch but this is going to conflict a lot with some other active issues, e.g. the entityreference/taxonomy reference one, taxonomy term NG conversion and comment as field, which are going to be harder to re-roll than this. So it probably makes sense to wait with this for a while.
Comment #8.0
BerdirUpdated issue summary.
Comment #9
andypostUpdated summary.
Suppose better make the issue as META and each conversion file as separate issue to make easy all big patches in queue to easy merge changes
Comment #9.0
andypostAdded steps and link to meta
Comment #10
podarokbetter title + updated summary due to #9
Comment #11
podarok#8 @Berdir good to see in summary as blockers those issues
Comment #12
BerdirAdditionally to the standard methods, we want to add methods on NodeInterface for things like status, changing status, created/changed and so on. That will make it easier to replace those as well.
Still not sure what to do about the comment as field issue. I will probably start by defining an interface and a NodeBCDecorator similar to what I did for users to come up with a good interface and then start to do search & replace, possibly in separte patches or split up by module or so.
Comment #13
andypost@Berdir, comment-field still lacks of dynamic entity reference to the attached entity
Comment #14
BerdirThat's not really relevant for this :)
What I mean is that this issue and the comment as field one will conflict *a lot*. And given that you already moved a lot code to generic entity code, you probably took care of a lot of ->nid => ->id() and similar changes already, so it would be easier to start after that is done. That said, we need to get rid of the BC decorator asap and need to start moving here and you have to re-roll almost daily anyway...
Comment #15
Berdir@timplunkett started doing exactly that in #2015123: Expand NodeInterface to provide getters and setters
Comment #16
fagoThis does not really block re-newing field API though, does it? It blocks being able to remove the BC decorator.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedRaising to critical per #1818580-7: [Meta] Convert all entity types to the new Entity Field API. Nodes are Drupal's most important entity type, so it's not like we can release without finishing this.
Comment #18
BerdirFirst patch. This removes the BC layer from nodes and starts converting everything we can to methods.
I have a branch that has the removal and each method change in separated commits, I'm just trying to see where we are with the tests. We can look how we want to split it up.
Comment #20
BerdirFixed syntax error, was introduced because @alexpott committed a patch that added that interface too this morning, and it didn't conflict when I rebased :) Let's see if there are more.
Comment #22
BerdirOk, this should be better.
The first patch just does the method conversions. This shouldn't have too many fails left.
The second patch removes the BC layer, this shows how far we're away from removing it.
Comment #24
BerdirThis should fix most of the fails for real ;)
Comment #26
BerdirRebased.
Comment #28
BerdirOk, I had the node form controller already switched to NG, that caused it to ignore a number of stuff that was set there. This should be better. Also fixed a bunch of other tests. Something with access still isn't quite right, don't know what yet.
Comment #30
BerdirRe-roll :)
Comment #32
BerdirJust the non-removal patch, This should fix the remaining test fails.
Lost time tracking down a bug that I didn't introduce and couldn't understand how I could be introducing it: #2031183: Improve test coverage for node authored on widget
Biggest change is the introduction of a setLanguage() method on EntityInterface(), we need this because language() caches the language it has. Alternatively, we could implement onChange( to handle this.
Comment #34
BerdirThis is going to conflict a lot ;)
Comment #36
BerdirMerge with the configurable languages issue didn't go well.
Comment #38
BerdirThis will hopefully fix the last translation tests.
Comment #39
fagoAwesome!
Left over?
In that context changing it does not make sense.
Needs to be removed.
Comment #40
fagooh, and this should the language fix from #1868004-68: Improve the TypedData API usage of EntityNG as well I guess
Comment #41
BerdirThanks for the review, killed setLanguage() and instead added the fix from there, removed the debug stuff, reverted the change in field.attach.inc.
Comment #43
BerdirFixed syntax error.
Comment #45
BerdirFixed some fails, haven't figured out the Node translation UI test yet.
Comment #47
BerdirRe-roll.
Will soon start to branch this off to get it in per method (group of related methods if patch size isn't too bug) but want to see what the tests have to say.
Comment #49
BerdirLost the interdiff from #45.
Also updated the removal patch, merged node_submit() into the form controller, no idea why this wasn't already done and merged in the patch from #1980822: Support any entity with path.module to make path aliases more or less work. They are correctly saved, but something with the language isn't correct yet, in my manual test it was saved as language en, which did not work, when I manually changed it to und, then it worked.
Comment #50
BerdirComment #51
BerdirTagging
Comment #53
BerdirRe-roll, the node translation UI test will still fail, but the bc removal patch should do much better.
Comment #55
BerdirOk, some new patches with more conversions and test fixes.
- Full conversion of methods, still the same failures.
- Only the nid changes (including a few more that I missed so far), this should be green and if so, will create a separate issue to get that in.
- Patch with BC removal.
Comment #57
BerdirPretty heavy re-roll after the uid patch and some controller conversions got it. Fixed the translation fails of the nid patch, let's see if something else broke.
Comment #59
BerdirI thought I fixed those fails already, must have lost that.
Just the patch then, which should now finally pass.
Comment #60
BerdirOh, didn't reference #2041287: Convert $node->nid to $node->id() and $node->isNew() yet, that's where the nid patch is now reviewed and hopefully committed soon, with your help :)
These are re-rolls on top of that, went through the NG test fails and fixed a lot of them. Just uploading to see how the testbot is doing. A few tricky test fails are remaining, but this should take care of a lot of them.
There's some crazy stuff in forum.module (surprise!), with $topic->last_post, which sometimes was a node object and sometimes a stdClass. changed to to entity_create() for now, maybe we should go the other way and always make it a stdClass, for performance reasons (or a different object but that I think would be out of scope)
Comment #62
BerdirUpdated patch without the nid part.
Comment #63
xmacinfonode->label()
still does not make sense.We need to keep
node->title()
, or developers will think Drupal is gone insane. :-)Is there an issue with a patch to put back
node->title()
?That Change record, [#1697182], does not specify why or how…
.
Comment #65
BerdirOk, reverted those method calls in that preprocess for now, I think what we should do there is always use a stdClass object, the documentation on it is lousy anyway and core only uses it to populate the other variables.
Comment #66
chx CreditAttribution: chx commentedCould we remove the getBCentity call from load and see whether it passes?
Comment #67
BerdirIt doesn't. And it's not that easy to remove the BC decorator ;) See the -bc-removal patches (+ interdiffs, which shows the difference), which I'm also uploading from time to time, last in #60.
There are still a lot of fails left, some possibly due to missing method usage but most due to different behavior, configurable fields (for which we have no methods, obviously).
Some of those fails are easy to fix, e.g. "Object of class Drupal\Core\Entity\Field\Field could not be converted to string" stuff is usually straight-forward but other things are... tricky. That's why I'm getting this in piece by piece, to have the simple search & replace stuff out of the way so we can then in the end focus on the non-trivial things.
Comment #68
chx CreditAttribution: chx commentedThen let's get this in.
Comment #69
BerdirThanks, let's keep this open to track the overall progress, opened #2049039: Convert node properties to methods. to get the current green patch in, please review/RTBC there.
Comment #70
BerdirRe-roll of the full patch, can't reproduce some of these test fails, let's see where we stand.
Comment #72
BerdirFixed a number of tests,
Comment #74
BerdirReroll based on the patch in the methods issue and removing the NodeBCDecorator class.
The last patch resulted in a lot of additional fails due to the removal of various getBCEntity() calls, let's see if this is better now that the bc decorator for users got removed.
Comment #76
BerdirAnother re-roll based on the other issue and some fixes for forum and other tests.
Comment #78
BerdirRe-roll, some tests fixed. Book is causing me serious troubles, crazy crazy stuff. This is just some experimenting there.. it's doing all kinds of unholy things with $node->book, sometimes joined menu_link data, sometimes it's an actual menu_link entity, which by now is quite a different thing and only works due to the array bc stuff there (not related to EntityBCDecorator).
Currently thinking about makin mlid an entity reference there or something like that. We'll see.
Comment #80
xjmThis is an approved API change per #1983554-8: Remove BC-mode from EntityNG (tagging on behalf of @webchick).
Comment #81
klausi#2063571: Node denormalization for fields with limited values is broken is blocked by this issue.
Comment #82
rlmumfordThe patch doesn't apply anymore, if someone rerolls this I'm happy to have a crack at fixing the tests.
Comment #83
Berdir@rlmumford: Hi! ;) I'm doing a lot of rebasing here to keep the simple replacements separated, the idea is to get them in separately from this in #2049039: Convert node properties to methods.. That issue unfortunately got a bit stuck on #2056721: Remove TypedDataInterface::getType(). So it's better when I do the re-roll to keep my git history.
I'll try to do that asap, but there's not much point in that until the typed data issue is committed as I will have to do some heavy search & replace afterwards. But we can probably continue with fixing tests once I re-rolled, let's sync in IRC when that happened.
Be warned, you might get to see some dark places while trying to fix these tests :)
Comment #84
BerdirFull re-roll based on the referenced issue to see where we are...
Comment #86
BerdirMessed up the forum_submitted merge conflict, this should fix those noticed. Also found the reason for the node access language fails.
Comment #88
rlmumfordJust going to see if this fixes one of the failed tests.
Comment #90
BerdirYay, the method issue got in, updated patch.
I'm going to try a different approach with the book module.
Comment #92
BerdirRe-roll and fixed those tests.
- Node revision turned out to be trivial, I just compared the field object instead of the value
- For book, I reverted the previous changes and used a custom entity builder callback that assigns the book stuff to the node without having to convert it to a field. I hope we can live with that because making it a real field would be very complicated. We could look into it in a follow-up.
- Same for re-translate checkbox of the translation module. That could also be a normal submit callback as it's not really data, more like an action, so no need to go through the node entity hooks.
Let's see if there was new code that relies on the BC decorator since my last re-roll. If not, we might have a green patch.
Comment #94
BerdirClose. This should really be green then.
Comment #95
tim.plunkettVery little to complain about! Much easier to review with
git diff --color-words
Ahem.
It's a beautiful thing!
Well, does it?
These look a bit odd. When is it not an entity?
Comment #96
Berdir- Re-rolled
- Removed debug()
- I can make it even more beatiful ;)
- I think that @todo is ok, but some manual testing of forum.module certainly wouldn't hurt, our test coverage there is certainly not perfect ;) Thinking about it, especially the shadow copy stuff needs to be checked.
- Yeah, the $translations stuff is ugly, sometimes translation.module creates stdClass objects there, sometimes it's a node object. Not too worried about this as there's a critical issue to remove translation.module.
Comment #97
chx CreditAttribution: chx commentedI would say it's good to go. I am a bit ambivalent on $node->getAuthor()->getUsername() vs $node->uid->entity->name as it's nice to have IDE support, doxygen, whatnot but then again a uniform way wouldn't hurt either -- but getAuthor is already in HEAD so let's get this in so that we can finally get rid of the BC decorator.
Comment #98
tim.plunkettWow, you weren't kidding.
+1 RTBC
Comment #99
Berdir#96: 1939994-96.patch queued for re-testing.
Comment #100
catchNice.
Committed/pushed to 8.x!
Comment #101
BerdirRemoving sprint tag.
Comment #102.0
(not verified) CreditAttribution: commentedUpdated issue summary.