Updated: Comment #N

Problem/Motivation

#2005716: Promote EntityType to a domain object converts the entity type from an array into a classed object. While previously the function used to get this data was called entity_get_info(), the object returned is of the interface EntityTypeInterface. This is further compounded by the EntityInterface and EntityStorageControllerInterface interfaces having methods called entityType() that return a string, and entityInfo() that return EntityTypeInterface

Proposed resolution

Rename the method returning the entity type object to be called getEntityType() and renamed the existing entityType() method to getEntityTypeId()

protected $entityInfo and $entityType properties in classes are renamed accordingly.

Remaining tasks

Write the patch

User interface changes

N/A

API changes

The entityInfo() method on both EntityInterface and EntityStorageControllerInterface will be renamed to getEntityType(). The current code>code>entityType() method is renamed to getEntityTypeId().

protected $entityInfo and $entityType properties in classes are renamed accordingly.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

$entity->id()
$language->id
$entity_type->getId()
;-)

tim.plunkett’s picture

Not my fault they don't follow our getter pattern. ;)

fago’s picture

Status: Postponed » Active
tim.plunkett’s picture

Status: Active » Needs review
FileSize
129.03 KB
Berdir’s picture

Looks good so far.

We should IMHO also at least also fix the documentation on EntityStorageControllerInterface and EntityInterface for those methods. At least the storage controller now looks like this:

  /**
   * Returns the entity info.
   *
   * @return string
   *   The entity info.
   */
  public function entityType();

The @return type was obviously already wrong before, but this isn't getting better :)

Similar for the protected properties in Entity, and the Entity controller base classes like EntityStorageControllerBase.

Apart from the actual properties, we also have stale references to them like "* Set by entity info." for $this->cache.

Not sure what belongs to this issue and what to the $entity_type/$entity_info variable one, but I would really like to see the interfaces and base classes cleaned up so that they make sense again :)

Berdir’s picture

Re-roll, what about this for fixing the documentation on those two interfaces?

I'd be ok to get this in like this.

Berdir’s picture

6: entity-2164827-6.patch queued for re-testing.

The last submitted patch, 6: entity-2164827-6.patch, failed testing.

Berdir’s picture

FileSize
127.28 KB

Re-roll. Patch is a bit smaller because a few pieces are no longer necessary in the Metadata generator and legacy field stuff was removed. Might have some new code that will fail, let's see.

Status: Needs review » Needs work

The last submitted patch, 9: entity-2164827-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
127.32 KB
2.05 KB

Fixing tests.

The last submitted patch, 11: entity-2164827-11.patch, failed testing.

Berdir’s picture

Fixing those test fails.

Berdir’s picture

FileSize
127.55 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 14: entity-2164827-14.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
128.51 KB
985 bytes

Fixing that test. Can I haz a review/RTBC? :)

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
@@ -7,10 +7,12 @@
-interface EntityDisplayInterface {
+interface EntityDisplayInterface extends EntityInterface {

Nice catch! :-)

Looked through the patch line by line and couldn't find anything to complain.

catch’s picture

The patch doesn't match the issue summary, what happened to $this->entityType()->getId()? I don't particularly mind $this->entityTypeId() just wondering.

While asking questions - how come the property is kept as $entityType - that looked a bit odd with the method name change.

Berdir’s picture

I think getEntityId() us useful as it's used very often, will update the issue summary.

I was wondering about that too (it's strange), and I'm OK with renaming it if you think it makes sense. We were trying to get the most critical/conflicting change out of the way, renaming internal property shouldn't have that much impact. I think the entity classes are not relevant, but the controllers might require quite some changes but that should be easy to do in PhpStorm.

Berdir’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
197.51 KB
72.89 KB

Ok, let's try to rename it, the attached patch renames all protected $entityInfo's that I could find and the corresponding protected $entityType's.

Then we should hopefully have most of the changes that affect API's (local variables, arguments and so on don't matter and we have an issue for them)

Status: Needs review » Needs work

The last submitted patch, 20: entity-2164827-20.patch, failed testing.

The last submitted patch, 20: entity-2164827-20.patch, failed testing.

Berdir’s picture

20: entity-2164827-20.patch queued for re-testing.

The last submitted patch, 20: entity-2164827-20.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
199.69 KB
2.96 KB

Ah, that conflicted with an existing property in the entity display mode add forms, this should fix that.

tstoeckler’s picture

+++ b/core/modules/entity/lib/Drupal/entity/Form/EntityDisplayModeAddForm.php
@@ -36,19 +38,19 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL)
+    $this->entity->targetEntityType = $this->targetEntityTypeId;
...
-    $this->entity->targetEntityType = $this->entityType;

This points to another variable that should be renamed :-/

Otherwise looks good. Reviewed both interdiffs.

Berdir’s picture

Yeah, but that would involve quite a bit, as I'd have to change everything related to those config entities, view modes, ...

It's also not that problematic, because it can't conflict. The term "entity type" will continue to reference both the name/ID and the EntityType class, I don't think we'll be able to change that completely.

Status: Needs review » Needs work

The last submitted patch, 25: entity-2164827-25.patch, failed testing.

The last submitted patch, 25: entity-2164827-25.patch, failed testing.

Berdir’s picture

25: entity-2164827-25.patch queued for re-testing.

The last submitted patch, 25: entity-2164827-25.patch, failed testing.

The last submitted patch, 25: entity-2164827-25.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
199.65 KB
937 bytes

Hm, interesting, #2175517: Entity displays are themselves config entities actually added the extends for the EntityDisplayInterface too (for ConfigEntityInterface), but it was explicitly removed in #2031725: Move all entity display interfaces to the core component, I can however see now that it can't really work like that. Will cross-post there as well.

Anyway, no longer our problem here.

Re-rolled which removed that part completely, and fixed the new code in MenuLink.

Status: Needs review » Needs work

The last submitted patch, 33: entity-2164827-33.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review
FileSize
199.61 KB

Re-roll.

tstoeckler’s picture

Btw I'm fine with #27. If this is green it's RTBC.

Berdir’s picture

35: drupal_2164827_35.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 35: drupal_2164827_35.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
199.43 KB

Another re-roll.

#2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit has apparently not actually been reverted yet, so when that happens, it will need another re-roll.

Status: Needs review » Needs work

The last submitted patch, 39: entity-2164827-39.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Checked with @alexpott, we will postpone this issue until after the code sprint, won't be committed today or tomorrow, don't bother with a re-roll :)

Berdir’s picture

Status: Needs review » Needs work

Huh

Berdir’s picture

Status: Needs work » Needs review
FileSize
198.59 KB

Ok, here's the re-roll.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This was a classical berdir one: almost none out of scope changes.
RTBC beside of the point made my sun.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -72,17 +72,17 @@ public function enforceIsNew($value = TRUE);
    -  public function entityType();
    +  public function entityTypeId();
    
    @@ -221,11 +221,12 @@ public static function postLoad(EntityStorageControllerInterface $storage_contro
    +  public function entityType();
    ...
    -  public function entityInfo();
    

    sun came up in irc that we should prefix them with "get" if we already change all those instances.

  2. +++ b/core/modules/user/user.module
    @@ -1792,7 +1792,7 @@ function user_cookie_delete($cookie_name) {
    -  if ($entity->entityType() == 'user') {
    

    Topic for a follow up: should we rather check for the interface here instead?

  3. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
    @@ -842,10 +842,10 @@ public function isNew() {
       /**
    -   * Implements \Drupal\Core\Entity\EntityInterface::entityType().
    +   * Implements \Drupal\Core\Entity\EntityInterface::entityTypeId().
        */
    
    @@ -856,10 +856,10 @@ public function bundle() {
       /**
    -   * Implements \Drupal\Core\Entity\EntityInterface::entityInfo().
    +   * Implements \Drupal\Core\Entity\EntityInterface::entityType().
        */
    

    If we change it here, we can inheritdoc it as well.

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
200.51 KB
120.91 KB

Ok, let's try that then, this renames to get*(), let's see if I got them all.

PhpStorm found most places, it interestingly missed a few that were explicitly type hinted but then showed me the correct type hint.

2. I'd actually rather drop the whole hook :) See #2078473: Use entity access API for checking access to private files Don't care that much about getEntityTypeId() vs interfaceof.

3. Fixed those.

Status: Needs review » Needs work

The last submitted patch, 45: entity-methods-2164827-10.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
200.52 KB
1.71 KB

Fixing unit tests.

Status: Needs review » Needs work

The last submitted patch, 47: entity-methods-2164827-12.patch, failed testing.

Berdir’s picture

Berdir’s picture

Status: Needs work » Needs review

Huh, re-test no longer sets the issue status back to needs review?

Berdir’s picture

Ah, the menu link issue has been committed again, so re-adding the fix there, let's see...

The last submitted patch, 47: entity-methods-2164827-12.patch, failed testing.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great, thank you for renaming the methods again.

sun’s picture

Thanks! The renamed methods are looking great :-)

alexpott’s picture

Title: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface » Change notice: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change

Committed 20315e1 and pushed to 8.x. Thanks!

Berdir’s picture

Title: Change notice: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface » Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface
Priority: Major » Critical
Status: Active » Fixed

Thanks, created https://drupal.org/node/2185427 and cross-referenced with the existing change notice at https://drupal.org/node/2181667 about the EntityType change.

Status: Fixed » Closed (fixed)

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