Problem/Motivation

EntityTypeInterface::getKeys() documents that entity IDs must be numeric. This is not true, however, we explicitly support string IDs, see in particular the EntityTestStringId test entity.

Steps to reproduce

-

Proposed resolution

Update the documentation.

Remaining tasks

User interface changes

-

Introduced terminology

-

API changes

-

Data model changes

-

Release notes snippet

-

CommentFileSizeAuthor
#7 issues_3467540_7.png191.51 KBkleiton_rodrigues

Issue fork drupal-3467540

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tstoeckler created an issue. See original summary.

tstoeckler opened merge request !9177

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Active » Needs review

Created a quick MR.

Since this was directly in scope of the lines being touched, I opted to touch up two other things:
- Removed the note about the ID key being required. It is not, it is only required if you actually want to save the entity. That of course means in 99% of cases you do want to specify an ID key, but with contact messages in core we actually have an example of not having an ID, so it doesn't make much sense to claim it's required.
- Removed the usage of the term "Field API". It is a remnant of D7 times. In current times it is actually in correct because getKeys() is used for config entities as well and they have nothing to do with Field API in its current form. The more correct/current term would be "Entity API", but instead of replacing that I just removed the explicit mentioning of that as that would have felt somewhat out of place there.

I also noted that we use the term "property" there, to describe the values of the keys array. That's also somewhat incorrect, as for content entities, those are actually field names and properties are something different, but for config entities we do tend to use the term properties (for example ConfigEntityTypeInterface::getPropertiesToExport(), so I think it might be more clear to consistently use "field or property" instead of just "property". That would need to be changed a bunch of times, though, in that documentation paragraphs, so I anticipate that being flagged as too broad in scope for this issue.

prem suthar’s picture

prem suthar commented

@tstoeckler you used twice from at the @return array description .

 /**
   * Gets an array of entity keys.
   *
   * @return array
   *   An array describing how the certain information can be extracted from
   *   from entities of this entity type:
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Nice catch, thanks! 👍

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Seems pretty straightforward

kleiton_rodrigues’s picture

kleiton_rodrigues commented
StatusFileSize
new issues_3467540_7.png191.51 KB

@prem-suthar RTBC +1

quietone’s picture

quietone commented

@kleiton_rodrigues, There are contributor tasks on Drupal.org that provide the steps for making useful additions to an issue. You may find How is credit granted for Drupal core issues useful as well.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One comment on the MR.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Fixed

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Also the MR is 250 commits back :) but you're right the comments would not cause those failures. Feedback from @catch appears to be addressed.

  • quietone committed 7858c7be on 11.1.x 
    Issue #3467540 by tstoeckler, prem suthar, smustgrave, catch: Update...

  • quietone committed 99a78afb on 11.x 
    Issue #3467540 by tstoeckler, prem suthar, smustgrave, catch: Update...

quietone closed merge request !9177

quietone’s picture

quietone commented
Version: 11.x-dev » 11.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and cherry-picked to 11.1.x

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.