From 3ef6de205fad73a39925f7dcde97baaee34afe33 Mon Sep 17 00:00:00 2001
From: Yuri Kuznetsov <yurikuzn@users.noreply.github.com>
Date: Mon, 21 Mar 2022 11:34:48 +0200
Subject: [PATCH] get id exception

---
 application/Espo/Core/AclManager.php          | 12 ++-----
 application/Espo/Core/Duplicate/Finder.php    |  4 +--
 application/Espo/Core/Htmlizer/Htmlizer.php   |  2 +-
 .../GroupAccount/Hooks/BeforeFetch.php        |  2 +-
 application/Espo/Core/Portal/AclManager.php   | 12 ++-----
 .../Core/Utils/Acl/UserAclManagerProvider.php |  2 +-
 .../Espo/Modules/Crm/Services/Campaign.php    |  2 +-
 .../Modules/Crm/Tools/MassEmail/Processor.php |  2 +-
 application/Espo/ORM/BaseEntity.php           | 20 ++++++++++--
 application/Espo/ORM/Entity.php               | 12 +++++--
 application/Espo/ORM/EntityManager.php        |  2 +-
 application/Espo/ORM/Mapper/BaseMapper.php    |  8 +++--
 .../Espo/ORM/Repository/RDBRelation.php       | 18 +++++------
 .../Espo/ORM/Repository/RDBRepository.php     | 24 ++++++++------
 application/Espo/Repositories/ArrayValue.php  |  2 +-
 .../Espo/Repositories/EmailAddress.php        |  4 +++
 application/Espo/Repositories/PhoneNumber.php |  4 +++
 application/Espo/Repositories/Preferences.php |  4 +--
 application/Espo/Services/Stream.php          |  2 +-
 .../Espo/Tools/EmailTemplate/Processor.php    |  2 +-
 application/Espo/Tools/Import/Import.php      |  2 +-
 .../Espo/Tools/LeadCapture/LeadCapture.php    |  2 +-
 .../Espo/Tools/Stream/HookProcessor.php       | 11 ++-----
 tests/unit/Espo/ORM/EntityTest.php            | 32 +++++++++++++++++++
 24 files changed, 117 insertions(+), 70 deletions(-)

diff --git a/application/Espo/Core/AclManager.php b/application/Espo/Core/AclManager.php
index 9f3068d193..9df73811ab 100644
--- a/application/Espo/Core/AclManager.php
+++ b/application/Espo/Core/AclManager.php
@@ -198,11 +198,7 @@ class AclManager
 
     protected function getTable(User $user): Table
     {
-        $key = $user->getId();
-
-        if (!$key) {
-            $key = spl_object_hash($user);
-        }
+        $key = $user->hasId() ? $user->getId() : spl_object_hash($user);
 
         if (!array_key_exists($key, $this->tableHashMap)) {
             $this->tableHashMap[$key] = $this->tableFactory->create($user);
@@ -213,11 +209,7 @@ class AclManager
 
     protected function getMap(User $user): Map
     {
-        $key = $user->getId();
-
-        if (!$key) {
-            $key = spl_object_hash($user);
-        }
+        $key = $user->hasId() ? $user->getId() : spl_object_hash($user);
 
         if (!array_key_exists($key, $this->mapHashMap)) {
             $this->mapHashMap[$key] = $this->mapFactory->create($user, $this->getTable($user));
diff --git a/application/Espo/Core/Duplicate/Finder.php b/application/Espo/Core/Duplicate/Finder.php
index 3b942b49fd..8e5eef4e67 100644
--- a/application/Espo/Core/Duplicate/Finder.php
+++ b/application/Espo/Core/Duplicate/Finder.php
@@ -108,7 +108,7 @@ class Finder
     {
         $entityType = $entity->getEntityType();
 
-        if ($entity->getId()) {
+        if ($entity->hasId()) {
             $where = Cond::and(
                 $where,
                 Cond::notEqual(
@@ -136,7 +136,7 @@ class Finder
     {
         $entityType = $entity->getEntityType();
 
-        if ($entity->getId()) {
+        if ($entity->hasId()) {
             $where = Cond::and(
                 $where,
                 Cond::notEqual(
diff --git a/application/Espo/Core/Htmlizer/Htmlizer.php b/application/Espo/Core/Htmlizer/Htmlizer.php
index 6f1f7e047b..1fe6a7da7f 100644
--- a/application/Espo/Core/Htmlizer/Htmlizer.php
+++ b/application/Espo/Core/Htmlizer/Htmlizer.php
@@ -274,7 +274,7 @@ class Htmlizer
 
         $relationList = $entity->getRelationList();
 
-        if (!$skipLinks && $level === 0 && $this->entityManager && $entity->getId()) {
+        if (!$skipLinks && $level === 0 && $this->entityManager && $entity->hasId()) {
             foreach ($relationList as $relation) {
                 $collection = null;
 
diff --git a/application/Espo/Core/Mail/Account/GroupAccount/Hooks/BeforeFetch.php b/application/Espo/Core/Mail/Account/GroupAccount/Hooks/BeforeFetch.php
index 33c58dbb47..a21a00a6e7 100644
--- a/application/Espo/Core/Mail/Account/GroupAccount/Hooks/BeforeFetch.php
+++ b/application/Espo/Core/Mail/Account/GroupAccount/Hooks/BeforeFetch.php
@@ -162,7 +162,7 @@ class BeforeFetch implements BeforeFetchInterface
         if (
             $campaignId &&
             $target &&
-            $target->getId()
+            $target->hasId()
         ) {
             $this->getCampaignService()
                 ->logBounced(
diff --git a/application/Espo/Core/Portal/AclManager.php b/application/Espo/Core/Portal/AclManager.php
index e4232e6100..319a832409 100644
--- a/application/Espo/Core/Portal/AclManager.php
+++ b/application/Espo/Core/Portal/AclManager.php
@@ -109,11 +109,7 @@ class AclManager extends InternalAclManager
 
     protected function getTable(User $user): TableBase
     {
-        $key = $user->getId();
-
-        if (!$key) {
-            $key = spl_object_hash($user);
-        }
+        $key = $user->hasId() ? $user->getId() : spl_object_hash($user);
 
         if (!array_key_exists($key, $this->tableHashMap)) {
             $this->tableHashMap[$key] = $this->portalTableFactory->create($user, $this->getPortal());
@@ -124,11 +120,7 @@ class AclManager extends InternalAclManager
 
     protected function getMap(User $user): Map
     {
-        $key = $user->getId();
-
-        if (!$key) {
-            $key = spl_object_hash($user);
-        }
+        $key = $user->hasId() ? $user->getId() : spl_object_hash($user);
 
         if (!array_key_exists($key, $this->mapHashMap)) {
             /** @var Table */
diff --git a/application/Espo/Core/Utils/Acl/UserAclManagerProvider.php b/application/Espo/Core/Utils/Acl/UserAclManagerProvider.php
index 5243a80ec7..db94567372 100644
--- a/application/Espo/Core/Utils/Acl/UserAclManagerProvider.php
+++ b/application/Espo/Core/Utils/Acl/UserAclManagerProvider.php
@@ -69,7 +69,7 @@ class UserAclManagerProvider
      */
     public function get(User $user): AclManager
     {
-        $key = $user->getId() ?? spl_object_hash($user);
+        $key = $user->hasId() ? $user->getId() : spl_object_hash($user);
 
         if (!isset($this->map[$key])) {
             $this->map[$key] = $this->load($user);
diff --git a/application/Espo/Modules/Crm/Services/Campaign.php b/application/Espo/Modules/Crm/Services/Campaign.php
index 3bc9ba9ee8..2b32a84e60 100644
--- a/application/Espo/Modules/Crm/Services/Campaign.php
+++ b/application/Espo/Modules/Crm/Services/Campaign.php
@@ -304,7 +304,7 @@ class Campaign extends Record implements
         if ($queueItem) {
             $massEmail = $this->entityManager->getEntity('MassEmail', $queueItem->get('massEmailId'));
 
-            if ($massEmail && $massEmail->getId()) {
+            if ($massEmail && $massEmail->hasId()) {
                 $logRecord = $this->entityManager->getEntity('CampaignLogRecord');
 
                 $logRecord->set([
diff --git a/application/Espo/Modules/Crm/Tools/MassEmail/Processor.php b/application/Espo/Modules/Crm/Tools/MassEmail/Processor.php
index 06b5250ba2..0b2ae1e589 100644
--- a/application/Espo/Modules/Crm/Tools/MassEmail/Processor.php
+++ b/application/Espo/Modules/Crm/Tools/MassEmail/Processor.php
@@ -409,7 +409,7 @@ class Processor
 
         if (
             !$target ||
-            !$target->getId() ||
+            !$target->hasId() ||
             !$emailAddress
         ) {
             $queueItem->set('status', EmailQueueItem::STATUS_FAILED);
diff --git a/application/Espo/ORM/BaseEntity.php b/application/Espo/ORM/BaseEntity.php
index 4389362644..4a03d1591a 100644
--- a/application/Espo/ORM/BaseEntity.php
+++ b/application/Espo/ORM/BaseEntity.php
@@ -130,9 +130,25 @@ class BaseEntity implements Entity
     /**
      * Get an entity ID.
      */
-    public function getId(): ?string
+    public function getId(): string
     {
-        return $this->get('id');
+        /** @var ?string */
+        $id = $this->get('id');
+
+        if ($id === null) {
+            throw new RuntimeException("Entity ID is not set.");
+        }
+
+        if ($id === '') {
+            throw new RuntimeException("Entity ID is empty.");
+        }
+
+        return $id;
+    }
+
+    public function hasId(): bool
+    {
+        return $this->id !== null;
     }
 
     /**
diff --git a/application/Espo/ORM/Entity.php b/application/Espo/ORM/Entity.php
index dd0a2d633a..5e3fecbd11 100644
--- a/application/Espo/ORM/Entity.php
+++ b/application/Espo/ORM/Entity.php
@@ -60,8 +60,16 @@ interface Entity
 
     /**
      * Get an entity ID.
+     *
+     * @return non-empty-string
+     * @throws \RuntimeException If an ID is not set.
      */
-    public function getId(): ?string;
+    public function getId(): string;
+
+    /**
+     * Whether an ID is set.
+     */
+    public function hasId(): bool;
 
     /**
      * Reset all attributes (empty an entity).
@@ -73,7 +81,7 @@ interface Entity
      *
      * Two usage options:
      * * `set(string $attribute, mixed $value)`
-     * * `set(array|object $valueMap)`
+     * * `set(array|stdClass $valueMap)`
      *
      * @param string|stdClass|array<string,mixed> $attribute
      * @param mixed $value
diff --git a/application/Espo/ORM/EntityManager.php b/application/Espo/ORM/EntityManager.php
index f881b3573e..093117be2f 100644
--- a/application/Espo/ORM/EntityManager.php
+++ b/application/Espo/ORM/EntityManager.php
@@ -346,7 +346,7 @@ class EntityManager
             throw new RuntimeException("Can't refresh a new entity.");
         }
 
-        if ($entity->getId() === null) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't refresh an entity w/o ID.");
         }
 
diff --git a/application/Espo/ORM/Mapper/BaseMapper.php b/application/Espo/ORM/Mapper/BaseMapper.php
index b2512a935a..94c7a920b3 100644
--- a/application/Espo/ORM/Mapper/BaseMapper.php
+++ b/application/Espo/ORM/Mapper/BaseMapper.php
@@ -700,10 +700,12 @@ class BaseMapper implements RDBMapper
     {
         $params = $select->getRaw();
 
-        $id = $entity->getId();
+        if (!$entity->hasId()) {
+            throw new RuntimeException("Entity w/o ID.");
+        }
 
-        if (empty($id) || empty($relationName)) {
-            throw new RuntimeException("Can't mass relate on empty ID or relation name.");
+        if (empty($relationName)) {
+            throw new RuntimeException("Empty relation name.");
         }
 
         $relType = $entity->getRelationType($relationName);
diff --git a/application/Espo/ORM/Repository/RDBRelation.php b/application/Espo/ORM/Repository/RDBRelation.php
index 1f4ae588d1..b9e12a44b1 100644
--- a/application/Espo/ORM/Repository/RDBRelation.php
+++ b/application/Espo/ORM/Repository/RDBRelation.php
@@ -75,7 +75,7 @@ class RDBRelation
         $this->entity = $entity;
         $this->hookMediator = $hookMediator;
 
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't use an entity w/o ID.");
         }
 
@@ -363,7 +363,7 @@ class RDBRelation
             throw new RuntimeException("Entity type doesn't match an entity type of the relation.");
         }
 
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't use an entity w/o ID.");
         }
     }
@@ -375,7 +375,7 @@ class RDBRelation
      */
     public function isRelated(Entity $entity): bool
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't use an entity w/o ID.");
         }
 
@@ -580,12 +580,12 @@ class RDBRelation
             throw new RuntimeException("Can't update not many-to-many relation.");
         }
 
-        $id = $entity->getId();
-
-        if ($id === null) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Entity w/o ID.");
         }
 
+        $id = $entity->getId();
+
         $this->getMapper()->updateRelationColumns($this->entity, $this->relationName, $id, $columnData);
     }
 
@@ -602,12 +602,12 @@ class RDBRelation
             throw new RuntimeException("Can't get a column of not many-to-many relation.");
         }
 
-        $id = $entity->getId();
-
-        if ($id === null) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Entity w/o ID.");
         }
 
+        $id = $entity->getId();
+
         return $this->getMapper()->getRelationColumn($this->entity, $this->relationName, $id, $column);
     }
 
diff --git a/application/Espo/ORM/Repository/RDBRepository.php b/application/Espo/ORM/Repository/RDBRepository.php
index 0b7d49f120..9900d026cb 100644
--- a/application/Espo/ORM/Repository/RDBRepository.php
+++ b/application/Espo/ORM/Repository/RDBRepository.php
@@ -306,7 +306,7 @@ class RDBRepository implements Repository
             throw new RuntimeException("Not supported entity type.");
         }
 
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             return null;
         }
 
@@ -373,7 +373,7 @@ class RDBRepository implements Repository
             throw new RuntimeException("Not supported entity type.");
         }
 
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             return 0;
         }
 
@@ -484,7 +484,7 @@ class RDBRepository implements Repository
      */
     public function isRelated(Entity $entity, string $relationName, $foreign): bool
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             return false;
         }
 
@@ -495,6 +495,10 @@ class RDBRepository implements Repository
         /** @var mixed $foreign */
 
         if ($foreign instanceof Entity) {
+            if (!$foreign->hasId()) {
+                return false;
+            }
+
             $id = $foreign->getId();
         }
         else if (is_string($foreign)) {
@@ -535,11 +539,11 @@ class RDBRepository implements Repository
      */
     public function relate(Entity $entity, string $relationName, $foreign, $columnData = null, array $options = [])
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't relate an entity w/o ID.");
         }
 
-        if (! $foreign instanceof Entity && !is_string($foreign)) {
+        if (!$foreign instanceof Entity && !is_string($foreign)) {
             throw new RuntimeException("Bad 'foreign' value.");
         }
 
@@ -598,11 +602,11 @@ class RDBRepository implements Repository
      */
     public function unrelate(Entity $entity, string $relationName, $foreign, array $options = [])
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't unrelate an entity w/o ID.");
         }
 
-        if (! $foreign instanceof Entity && !is_string($foreign)) {
+        if (!$foreign instanceof Entity && !is_string($foreign)) {
             throw new RuntimeException("Bad foreign value.");
         }
 
@@ -710,11 +714,11 @@ class RDBRepository implements Repository
      */
     public function updateRelation(Entity $entity, string $relationName, $foreign, $columnData)
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't update a relation for an entity w/o ID.");
         }
 
-        if (! $foreign instanceof Entity && !is_string($foreign)) {
+        if (!$foreign instanceof Entity && !is_string($foreign)) {
             throw new RuntimeException("Bad foreign value.");
         }
 
@@ -743,7 +747,7 @@ class RDBRepository implements Repository
      */
     public function massRelate(Entity $entity, string $relationName, array $params = [], array $options = [])
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new RuntimeException("Can't related an entity w/o ID.");
         }
 
diff --git a/application/Espo/Repositories/ArrayValue.php b/application/Espo/Repositories/ArrayValue.php
index ff892a973b..527f3c35ef 100644
--- a/application/Espo/Repositories/ArrayValue.php
+++ b/application/Espo/Repositories/ArrayValue.php
@@ -132,7 +132,7 @@ class ArrayValue extends Database
 
     public function deleteEntityAttribute(CoreEntity $entity, string $attribute): void
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             throw new Error("ArrayValue: Can't delete {$attribute} w/o id given.");
         }
 
diff --git a/application/Espo/Repositories/EmailAddress.php b/application/Espo/Repositories/EmailAddress.php
index 4a7a21bd33..a28d933672 100644
--- a/application/Espo/Repositories/EmailAddress.php
+++ b/application/Espo/Repositories/EmailAddress.php
@@ -115,6 +115,10 @@ class EmailAddress extends \Espo\Core\Repositories\Database implements
      */
     public function getEmailAddressData(Entity $entity): array
     {
+        if (!$entity->hasId()) {
+            return [];
+        }
+
         $dataList = [];
 
         $emailAddressList = $this
diff --git a/application/Espo/Repositories/PhoneNumber.php b/application/Espo/Repositories/PhoneNumber.php
index b9c482a27f..8df9ba99e6 100644
--- a/application/Espo/Repositories/PhoneNumber.php
+++ b/application/Espo/Repositories/PhoneNumber.php
@@ -105,6 +105,10 @@ class PhoneNumber extends Database implements
      */
     public function getPhoneNumberData(Entity $entity): array
     {
+        if (!$entity->hasId()) {
+            return [];
+        }
+
         $dataList = [];
 
         $numberList = $this
diff --git a/application/Espo/Repositories/Preferences.php b/application/Espo/Repositories/Preferences.php
index 9e78087e5a..9fc9041019 100644
--- a/application/Espo/Repositories/Preferences.php
+++ b/application/Espo/Repositories/Preferences.php
@@ -237,7 +237,7 @@ class Preferences implements Repository,
 
     public function save(Entity $entity, array $options = []): void
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             return;
         }
 
@@ -293,7 +293,7 @@ class Preferences implements Repository,
 
     public function remove(Entity $entity, array $options = []): void
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             return;
         }
 
diff --git a/application/Espo/Services/Stream.php b/application/Espo/Services/Stream.php
index a42edd616e..144ba2c3db 100644
--- a/application/Espo/Services/Stream.php
+++ b/application/Espo/Services/Stream.php
@@ -392,7 +392,7 @@ class Stream
 
     public function unfollowAllUsersFromEntity(Entity $entity): void
     {
-        if (!$entity->getId()) {
+        if (!$entity->hasId()) {
             return;
         }
 
diff --git a/application/Espo/Tools/EmailTemplate/Processor.php b/application/Espo/Tools/EmailTemplate/Processor.php
index c841edcf28..a1b52ca32c 100644
--- a/application/Espo/Tools/EmailTemplate/Processor.php
+++ b/application/Espo/Tools/EmailTemplate/Processor.php
@@ -277,7 +277,7 @@ class Processor
             $text = str_replace('{' . $type . '.' . $variableName . '}', $value, $text);
         }
 
-        if (!$skipLinks && $entity->getId()) {
+        if (!$skipLinks && $entity->hasId()) {
             $text = $this->processLinks(
                 $type,
                 $entity,
diff --git a/application/Espo/Tools/Import/Import.php b/application/Espo/Tools/Import/Import.php
index 59240d4c49..302bc0d860 100644
--- a/application/Espo/Tools/Import/Import.php
+++ b/application/Espo/Tools/Import/Import.php
@@ -532,7 +532,7 @@ class Import
             }
         }
 
-        if ($entity->getId()) {
+        if ($entity->hasId()) {
             $this->entityManager
                 ->getRDBRepository($entity->getEntityType())
                 ->deleteFromDb($entity->getId(), true);
diff --git a/application/Espo/Tools/LeadCapture/LeadCapture.php b/application/Espo/Tools/LeadCapture/LeadCapture.php
index 9dbd38ee85..6ae401a0f9 100644
--- a/application/Espo/Tools/LeadCapture/LeadCapture.php
+++ b/application/Espo/Tools/LeadCapture/LeadCapture.php
@@ -352,7 +352,7 @@ class LeadCapture
             }
         }
 
-        if ($toRelateLead && $targetLead->getId()) {
+        if ($toRelateLead && $targetLead->hasId()) {
             $this->entityManager
                 ->getRDBRepository('Lead')
                 ->relate($targetLead, 'targetLists', $leadCapture->get('targetListId'), [
diff --git a/application/Espo/Tools/Stream/HookProcessor.php b/application/Espo/Tools/Stream/HookProcessor.php
index 31625bc5e5..e7d10ff6c2 100644
--- a/application/Espo/Tools/Stream/HookProcessor.php
+++ b/application/Espo/Tools/Stream/HookProcessor.php
@@ -596,19 +596,12 @@ class HookProcessor
         $audited = $this->metadata->get(['entityDefs', $entityType, 'links', $link, 'audited']);
         $auditedForeign = $this->metadata->get(['entityDefs', $foreignEntityType, 'links', $foreignLink, 'audited']);
 
-        $id = $entity->getId();
-        $foreignId = $foreignEntity->getId();
-
-        if (!$id || !$foreignId) {
-            return;
-        }
-
         if ($audited) {
-            $this->service->noteRelate($foreignEntity, $entityType, $id);
+            $this->service->noteRelate($foreignEntity, $entityType, $entity->getId());
         }
 
         if ($auditedForeign) {
-            $this->service->noteRelate($entity, $foreignEntity->getEntityType(), $foreignId);
+            $this->service->noteRelate($entity, $foreignEntity->getEntityType(), $foreignEntity->getId());
         }
     }
 
diff --git a/tests/unit/Espo/ORM/EntityTest.php b/tests/unit/Espo/ORM/EntityTest.php
index b62f9e5776..ece6141209 100644
--- a/tests/unit/Espo/ORM/EntityTest.php
+++ b/tests/unit/Espo/ORM/EntityTest.php
@@ -74,6 +74,9 @@ class EntityTest extends \PHPUnit\Framework\TestCase
     {
     }
 
+    /**
+     * @return \Espo\ORM\Entity
+     */
     protected function createEntity(string $entityType, ?string $className = null)
     {
         $defs = $this->metadata->get($entityType);
@@ -348,4 +351,33 @@ class EntityTest extends \PHPUnit\Framework\TestCase
 
         $this->assertEquals(1, $entity->get('int'));
     }
+
+    public function testHasId(): void
+    {
+        $entity = $this->createEntity('Test');
+
+        $this->assertFalse($entity->hasId());
+
+        $entity->set('id', '1');
+
+        $this->assertTrue($entity->hasId());
+    }
+
+    public function testGetId1(): void
+    {
+        $entity = $this->createEntity('Test');
+
+        $entity->set('id', '1');
+
+        $this->assertEquals('1', $entity->getId());
+    }
+
+    public function testGetIdEmpty(): void
+    {
+        $entity = $this->createEntity('Test');
+
+        $this->expectException(\RuntimeException::class);
+
+        $entity->getId();
+    }
 }