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

User interface changes

No.

API changes

No.

Comments

YesCT’s picture

Opened this so we can at least document the reasoning. And refer people here when the subject comes up again.

dawehner’s picture

Issue tags: +Novice

While it is quite a lot of work, it is not a hard one perse.

I would rather vote for won't fix tbh.

Berdir’s picture

Yeah, agree with won't fix. We have to stop renaming all the things at some point and arguably, getId() does read strange IMHO.

sun’s picture

arguably, getId() does read strange

"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.

YesCT’s picture

Issue tags: -Novice

because of the contentious nature of this one, it's not for the light of heart (or novices). :)

filijonka’s picture

Hi 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.

chx’s picture

A 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().

YesCT’s picture

I'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.

Berdir’s picture

Please 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?

sun’s picture

Let's simply leave the old id(), … methods around as BC method aliases?

For example:

  public function id() {
    return $this->getId();
  }
YesCT’s picture

ok, 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

YesCT’s picture

Issue tags: +Amsterdam2014
YesCT’s picture

Title: rename id() to getId() (and label() to getName() ) » remove (to be deprecated) id()
Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Critical
Issue summary: View changes
Issue tags: -Amsterdam2014
Related issues: +#2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?

oops. (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.

webflo’s picture

MVP: id() marked as deprecated in the EntityInterface and id()proxies to getId().

YesCT’s picture

Title: remove (to be deprecated) id() » replace all core usages of id() with getid()
Issue summary: View changes
Status: Active » Postponed

updating 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?

YesCT’s picture

Priority: Critical » Normal
Issue summary: View changes
YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes
Mile23’s picture

+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:

$ git commit -m '21'
[2246695 289c999] 21
 769 files changed, 3132 insertions(+), 3132 deletions(-)

NetBeans did a pretty good job. control-R is your friend.

Anonymous’s picture

Big +1

Berdir’s picture

I'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.

dawehner’s picture

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.

Agreed. 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.

daffie’s picture

+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().

filijonka’s picture

well 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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Status: Postponed » Active

It's not impossible to do this in a minor release.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.