Problem/Motivation
We have a pattern of having properties on classes, and making getters for them. See #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
In sub issues of that methods meta, and also in general when people look at our classes, they are confused that we have id() (and label()) [ .. a separate issue would be for and not getId() and getName() (or getLabel).]
To postpone or not to postpone
According to #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?:
This is desirable because of DX and reducing confusion because some code will have both ->id() in some lines and getId() in others. The outside-of-drupal world expects getId().
This is a task, because it is not a bug.
This is normal, because .. it is not critical or major. https://www.drupal.org/node/45111 (priority definition, needs more detail there)
This impacts core and contributed modules .. negatively because lot and lots of places have id() and it will be a giant patch for core.
Benefit of this is perhaps equal to the disruption.
And thus, should be 8.1.x
Proposed resolution
Proposed resolution: do a mega patch to programatically change id() to getId() everywhere.
Remaining tasks
- (done, but get feedback to confirm) Figure out at what point in the 8 or 9 release to plan to do this, per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?
- Postponed on #2350807: add getId() and make id() a wrapper for it and deprecate it
- Postponed also on 8.1.x being opened for development. (Please work on 8.0.x issues)
User interface changes
No.
API changes
No.
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedOpened this so we can at least document the reasoning. And refer people here when the subject comes up again.
Comment #2
dawehnerWhile it is quite a lot of work, it is not a hard one perse.
I would rather vote for won't fix tbh.
Comment #3
BerdirYeah, agree with won't fix. We have to stop renaming all the things at some point and arguably, getId() does read strange IMHO.
Comment #4
sun"strange"...? — That's what literally every programmer on this world expects. Just not the special unicorn of Drupal developers.
If we cannot do this for D8 for any reason, then this issue should be moved to D9.
Comment #5
YesCT CreditAttribution: YesCT commentedbecause of the contentious nature of this one, it's not for the light of heart (or novices). :)
Comment #6
filijonka CreditAttribution: filijonka commentedHi the meta issue mentioned in summary is something I'm reviewing (and redoing) right now and I have some inputs and thoughts.
1. label() isn't the same as getName()
2. People seems to already be ignoring the label function, and implementing get/set-label(). Probably because there lack of knowledge of label().
3. I'm assuming that renaming functions are something you don't do by physically changing the names of the functions but more adding a new function and setting the old one to deprecated?
So my suggestion would be to in the classes Entityinterface and ConfigEntityBaseInterface add the get/set function and set the original once to deprecated. We don't need to do more with that right now, it's low priority to update the classes extending these.
Why? Well one thing that we will gain on this is actually that the contrib modules that uses these classes will use the right functions directly, mostly because most contrib haven't begun on 8.x modules yet.
Hence when the deprecated functions will be removed it will have less impact.
Comment #7
chx CreditAttribution: chx commentedA very big hurray for getId(). In the snap to grid article I didn't even dare to mention entity ids because it would have went like this "So now I want an entity id. In D7 you could either use entity_extract_id() or guess whether it's ->nid ->tid ->uid. In D8, surely, it's ->get... opsie, not it's ->id(). " so I skipped that. It's really weird it's not called getId().
Comment #8
YesCT CreditAttribution: YesCT commentedI'm gonna try doing just the getId() part.
the more I work with this, the more annoying id() is
and the more I talk to new people, the more I see that they are confused by it.
Comment #9
BerdirPlease no. Just search for "->id()" and you will know why. There are *thousands* of instances of that in core alone, and without also changing uuid(), label(), bundle() and others, we will not get rid of the inconsistency completely anyway.
Yes, it is a bit confusing, but it's IMHO not worth the change now in 8.x. At some point, we have to stop refactoring things just to make them a bit nicer. I did a quick test and the result was "782 files changed, 3145 insertions(+), 3145 deletions(-)", resulting in a 1.5MB patch. I'm sure there are some wrong matches in there, but still, it's a bigger patch than FormStateInterface was for a single method rename?
Comment #10
sunLet's simply leave the old
id()
, … methods around as BC method aliases?For example:
Comment #11
YesCT CreditAttribution: YesCT commentedok, lets get feedback on pushing this to
8.1.x
and do #2350807: add getId() and make id() a wrapper for it and deprecate it
Comment #12
YesCT CreditAttribution: YesCT commentedComment #13
YesCT CreditAttribution: YesCT commentedoops. (not sure if triage is "work" on an issue in this context according to #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?.) removing amsterdam tag.
updating issue summary while following #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? instructions.
Comment #14
webflo CreditAttribution: webflo commentedMVP: id() marked as deprecated in the EntityInterface and
id()
proxies togetId()
.Comment #15
YesCT CreditAttribution: YesCT commentedupdating to reflect that #2350827: remove deprecated id() would be the actual removal of id().
@webflo that would be #2350807: add getId() and make id() a wrapper for it and deprecate it can you post the patch there?
Comment #16
YesCT CreditAttribution: YesCT commentedComment #17
YesCT CreditAttribution: YesCT commentedComment #18
YesCT CreditAttribution: YesCT commentedComment #19
YesCT CreditAttribution: YesCT commentedComment #20
YesCT CreditAttribution: YesCT commentedComment #21
Mile23+1 on this name change, and it would be great if it could be in 8.0.x, because if its disruptive now, it will be way more disruptive later.
Saying this is postponed is basically saying it will never change. It's pretty minor in the grand scheme of things, however.
I started on this experimentally, just to see if NetBeans was up to the challenge. I only refactored EntityInterface and then tried to get a PHPUnit run to finish for about a half hour. I didn't finish, or even come to a nice stopping place (PHPUnit runs still result in crashes). Some stats:
NetBeans did a pretty good job. control-R is your friend.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedBig +1
Comment #23
BerdirI'm still -1 on this. There are various other methods that are missing a get prefix as well, like label(), uuid(), language() bundle(), url(), urlInfo(), link() (the three url related ones of those are pretty new and nobody was bothered by the missing get...()). I don't see why id() is different from those.
If you'd really want enforce it, we'd have to rename all of those as well. Otherwise it's going to be just as confusing?
Also, in 8.x, we will *never* be able to remove id(), so the function will stay there forever and contrib and custom code will continue to use it. So what you get is then a mix of id() and getId(), which is even more confusing IMHO.
Comment #24
dawehnerAgreed. The only thing we could do, and in case we do, the earlier the better, is to introduce the other methods and mark the original ones as depreated.
On the other hand I think its a bad design to a) pretend with a get...() method that its a simple and normal property, especially for things like label() this is simply not the case.
Comment #25
daffie CreditAttribution: daffie commented+1 for me. I do agree with Berdir that if we should do it we must do it completely and also do label(), uuid(), language() bundle(), url(), urlInfo(), link() and any others out there. Also remove the mentioned function so that we do not get a mix of id() and getId().
Comment #26
filijonka CreditAttribution: filijonka commentedwell contrib will only use id() if it isn't deprecated and not another option is available as I suggested in #6 number 3.
and yes we should defintly try to do this for all misnamned functions
Comment #29
catchIt's not impossible to do this in a minor release.