Follow up for #1869250-37: Various EntityNG and TypedData API improvements
Config\Entity only overrides selective Entity\Entity methods thus far - these methods appear to be identical and not overridden.
Remove them.
None.
None.
From: #1869250-37: Various EntityNG and TypedData API improvements
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php @@ -126,4 +127,53 @@ public function getExportProperties() { + public function getBCEntity() { ... + public function getOriginalEntity() { ... + public function getName() { ... + public function getRoot() { ... + public function getPropertyPath() { ... + public function getParent() {Config\Entity only overrides selective Entity\Entity methods thus far - these methods appear to be identical and not overridden in any way; was there any reason to duplicate them?
We can remove them in a follow-up patch though - no reason to hold up the commit for that.
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | entity-1877638-30.patch | 9.79 KB | smiletrl |
| #22 | entity-1877638-22.patch | 9.75 KB | tim.plunkett |
| #17 | interdiff-core-rename-getOriginalEntity-1877638-14-17.txt | 4.92 KB | das-peter |
| #17 | core-rename-getOriginalEntity-1877638-17.patch | 9.7 KB | das-peter |
| #14 | drupal.1877638.14.configentitybase_cleanup.patch | 4.75 KB | yesct |
Comments
Comment #1
fagook, I'd suggest renaming getOriginalEntity() to getNGEntity(). While the name isn't nice, it's clear what you get and the method is part of the BC-layer code. Anyway, it's just there temporary, but we should avoid possible confusions with $entity->original.
Comment #2
fagoComment #4
fagooh - this doesn't apply as long #1778178: Convert comments to the new Entity Field API isn't committed. Let's get the other one in first.
Comment #5
fagoComment #6
fagook, re-rolled the patch.
Comment #7
sunThanks!
Comment #8
webchickJust a question.. in the glorious future when the letters "NG" are eradicated from the D8 code base... will we still call this "getEntity"? Or will it be "getOriginalEntity"? If so, it seems like we need to keep "Original" in the name.
Comment #9
fagoNope, as then the BC-mode will be removed, those two helpers for switching between the NG and the BC entity will go away as well. I hope that answers your question.
Comment #10
fago#6: d8_cleanup.patch queued for re-testing.
Comment #12
yesct commentedthis might be a good Novice one for the sprints coming up.
reroll doc: http://drupal.org/patch/reroll
Comment #13
Gaelan commentedRerolled.
Comment #14
yesct commentedadded \ in
Implements \Drupal...
to be consistant with surrounding code and also with http://drupal.org/node/1354#file
aside from that, it looked good to me.
Also updating issue summary.
Comment #14.0
yesct commentedadded html code tag to add clarity
Comment #15
fago#14: drupal.1877638.14.configentitybase_cleanup.patch queued for re-testing.
Comment #16
berdirPatch makes sense but we should wait until after the node NG issue is in, will require a ton more renames after that and it's finally RTBC.
Comment #17
das-peter commentedRe-roll after node ng patch.
Comment #18
berdirI thought there would be more of those but I guess most ore getBCEntity().
Looks good, back to RTBC.
Comment #19
berdirRe-adding lost terms.
Comment #20
yesct commentedI looked at the coding style and it looks good to me too.
(retesting to keep it fresh for commit)
Comment #21
yesct commented#17: core-rename-getOriginalEntity-1877638-17.patch queued for re-testing.
Comment #22
tim.plunkettDidn't apply, because before #1446382: Need a reliable way to determine if a specific bundle for an entity type is translatable EntityInterface::getOriginalEntity() was at the end of the file.
Comment #24
berdir#22: entity-1877638-22.patch queued for re-testing.
Comment #25
berdirWas a random failure about a misisng table, back to RTBC.
Comment #26
xjm#22: entity-1877638-22.patch queued for re-testing.
Comment #27
xjmComment #28
berdir#22: entity-1877638-22.patch queued for re-testing.
Comment #30
smiletrl commentedrerolled
Comment #31
berdirThanks, looks good.
Comment #32
webchickNot moving down from RTBC, but no, #9 didn't really answer my question.
Let's fast-forward to 3 months from now or whatever when BC no longer exists. That means NG also doesn't exist, because NG just is "the entity system."
That means we're going to have to rename this function again at some point down the road. What I was asking is therefore whether or not that function will be renamed at that point to "getEntity" or to "getOriginalEntity." If the latter, we're losing important semantic information. And either way, can we not just rename it to that already right now, and leave "BC" in the old stupid one that will eventually be removed?
Comment #33
berdirThis function will be removed together with getBCEntity(), it's the counterpiece to it to get the NG entity object when you have the BC decorator. When we remove the BC decorator we'll also remove this method as there will be no use case for it anymore :)
But until then, getNGEntity() makes more sense than getOriginalEntity() which has nothing to do with $entity->original that we have from 7.x and might become getOriginalEntity() or something like that.
Comment #34
webchickAh-ha. Okie doke then!
Committed and pushed to 8.x. Thanks.
Comment #35.0
(not verified) commentedupdated to indicate write patch no longer a remaining task.