Updated: Comment #N

Problem/Motivation

Once #2005716: Promote EntityType to a domain object goes in, we will have a lot of code like EntityTypeInterface $entity_info, and $entity_type will be a string still. This is confusing.

Proposed resolution

Replace $entity_type with $entity_type_id
Any time that $entity_info is an array of all entity types, use $entity_types
When $entity_info is the entity type itself, use $entity_type

Remaining tasks

Write patch

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Postponed » Active
Berdir’s picture

Status: Active » Needs review
FileSize
273.58 KB

Well, I guess that's what we get for renames like this :)

Only renamed $entity_info/$info and $entity-type just when it was in the way. No idea how to identify $entity_type's that are ID's, also not sure if we need to.

One part I didn't touch, and that's hook_entity_info() and the _alter(), not sure what to do about that' Just rename $entity_info and worry about the hook in a different issue?

tim.plunkett’s picture

We could do the renaming of the alter in #1981858: Rename hook_entity_info/alter() to hook_entity_type_build/alter() as well.

Status: Needs review » Needs work

The last submitted patch, 2: rename-entity-info-2165155-2.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
277.42 KB
7.62 KB

Doing the hook rename over there would be OK with me.

Missed to update route defaults/arguments, this should fix most of those tests, didn't check if all are due to this.

Status: Needs review » Needs work

The last submitted patch, 5: rename-entity-info-2165155-5.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
278.26 KB
2.43 KB

Fixing more routing stuff...

Status: Needs review » Needs work

The last submitted patch, 7: rename-entity-info-2165155-7.patch, failed testing.

tim.plunkett’s picture

Bad regex I guess, these changes to modules/config weren't needed.

Berdir’s picture

I didn't do any regex-ing, that was all PhpStorm rename refactoring, weird. it did something similar to some other arguments.

I tried that form manually and it worked, so I didn't look closer, I guess I just tested the ajax part that still worked?

tim.plunkett’s picture

Yeah, those arguments are only used if you deep-link to a specific export, or use the form with AJAX off.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -716,8 +716,8 @@ public function addTranslation($langcode, array $values = array()) {
    -    $info = $this->getEntityType();
    
    +++ b/core/lib/Drupal/Core/Config/Entity/Query/Query.php
    @@ -85,8 +85,8 @@ public function condition($property, $value = NULL, $operator = NULL, $langcode
    -    $entity_info = $this->entityManager->getDefinition($this->getEntityType());
    

    Let's fill a follow up to change the entity query class to also use something like getEntityTypeId.

  2. +++ b/core/modules/block/lib/Drupal/block/BlockAccessController.php
    @@ -31,22 +31,22 @@ class BlockAccessController extends EntityAccessController implements EntityCont
        *   The entity info for the entity type.
    ...
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    ...
    -   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_info
    

    it is a bit sad that we should probably better update the documentation in another follow up. this patch is big enough already.

  3. +++ b/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationController.php
    @@ -32,12 +32,12 @@ class ContentTranslationController implements ContentTranslationControllerInterf
    +  public function __construct($entity_type) {
    

    Another follow up: typehint all those instances.

  4. +++ b/core/modules/edit/lib/Drupal/edit/Form/EditFieldForm.php
    @@ -80,19 +80,19 @@ public function getFormId() {
    +    if ($entityId instanceof EntityChangedInterface) {
    ...
    -    if ($entity instanceof EntityChangedInterface) {
    ...
    +  public function buildForm(array $form, array &$form_state, EntityInterface $entityId = NULL, $field_name = NULL) {
    ...
    +      $this->init($form_state, $entityId, $field_name);
    ...
    -      $this->init($form_state, $entity, $field_name);
    

    My head thought for a while that entityId is an interface, luckily it won't and my head did not exploded.

Berdir’s picture

Thanks for the review!

1. #2191651: Rename Drupal\Core\Entity\Query\QueryInterface::getEntityType() to getEntityTypeId()
2. Fixed all "entity info" that made sense in this context, left a few related to entity_info_cache_clear() and hook_entity_info(), will clean that up in the issue mentioned above.
3. #2191655: Type hint $entity_type everywhere it is an instance of EntityTypeInterface.
4. Yeah, PhpStorm doesn't like our buildForm() trick with additional optional arguments. If you try to rename a variable there, it attempts to match that *somehow* in all buildForm() implemetnations we have. For example, I tried to rename that back using PhpStorm and it removed _$id from $action_id, $ban_id, $entity_type_id, $test_id and so on, $resulting in $ban_, $action_, o.0

Together with #1981858: Rename hook_entity_info/alter() to hook_entity_type_build/alter(), which I will address next, this should allow us to remove "entity_info" completely :)

Berdir’s picture

Re-roll.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Awesome

Berdir’s picture

Berdir’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6c2ed3f and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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