[#270] Fixed 'entityCreate()' base field expansion and dynamic id key resolution.#346
Merged
AlexSkrypnyk merged 3 commits intomasterfrom Apr 19, 2026
Merged
[#270] Fixed 'entityCreate()' base field expansion and dynamic id key resolution.#346AlexSkrypnyk merged 3 commits intomasterfrom
AlexSkrypnyk merged 3 commits intomasterfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDetects base-field properties present on entity stubs and auto-expands them through the field-handler pipeline; entityCreate/entityDelete now use the entity type’s configured ID and bundle keys (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code coverage (threshold: 95%) Per-class coverage |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php (2)
90-99: Exercise an entity-reference base field in this regression test.
nameonly provesDefaultHandlerruns. Since the fixed bug specifically mentions base entity-reference fields likeuser.roles, add arolesassertion so the test would fail ifEntityReferenceHandleris skipped.Suggested test expansion
use Drupal\Core\Entity\EntityInterface; use Drupal\Driver\Core\Core; use Drupal\entity_test\EntityTestHelper; use Drupal\KernelTests\KernelTestBase; +use Drupal\user\Entity\Role; use Drupal\user\Entity\User; use PHPUnit\Framework\Attributes\Group;public function testEntityCreateAutoExpandsBaseFieldsSetOnStub(): void { + $role = Role::create([ + 'id' => 'base_field_editor', + 'label' => 'Base field editor', + ]); + $role->save(); + $stub = (object) [ 'name' => 'uma', 'mail' => 'uma@example.com', 'status' => 1, + 'roles' => 'Base field editor', ]; - $this->core->entityCreate('user', $stub); + $created = $this->core->entityCreate('user', $stub); $this->assertSame(['uma'], $stub->name, 'base field "name" was routed through the handler pipeline.'); + $this->assertSame(['base_field_editor'], $stub->roles, 'base entity-reference field "roles" was routed through the handler pipeline.'); + $this->assertInstanceOf(User::class, $created); + $this->assertTrue($created->hasRole('base_field_editor')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php` around lines 90 - 99, Expand the testEntityCreateAutoExpandsBaseFieldsSetOnStub test to exercise a base entity-reference field: include a 'roles' entry on the $stub (in addition to 'name', 'mail', 'status') before calling $this->core->entityCreate('user', $stub), and add an assertion (e.g. $this->assertSame([...], $stub->roles)) that the roles property was transformed by the EntityReferenceHandler; this ensures the EntityReferenceHandler runs the same pipeline as DefaultHandler and will fail if it is skipped.
66-76: Assert the stub ID matches the created entity.
assertNotEmpty()provesuidwas set, but not that it is the ID returned by storage. A direct comparison makes this regression test tighter.Suggested assertion
$created = $this->core->entityCreate('user', $stub); $this->assertInstanceOf(EntityInterface::class, $created); - $this->assertNotEmpty($stub->uid, 'entityCreate populated the entity type id key (uid) on the stub.'); + $this->assertSame((string) $created->id(), (string) $stub->uid, 'entityCreate populated the created entity id under the entity type id key (uid) on the stub.'); $this->assertFalse(property_exists($stub, 'id'), 'entityCreate did not populate a generic "id" property on the stub.');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php` around lines 66 - 76, Add an assertion that the ID populated on the stub matches the ID of the created entity: after calling $this->core->entityCreate('user', $stub) and before deleting, assert that the created entity's id() (or the ID getter used by the returned EntityInterface instance, referenced as $created) equals the stub's uid property ($stub->uid) to ensure the stub was populated with the actual storage ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Drupal/Driver/Core/Core.php`:
- Around line 824-827: The code dereferences $entity->$id_key without ensuring
the stub contains a valid ID; update the guard around the EntityInterface check
in Core:: (the block using $entity, $id_key and
\Drupal::entityTypeManager()->getStorage($entity_type)->load(...)) to verify
that the stub has a non-empty property named by $id_key (e.g.
isset($entity->{$id_key}) && $entity->{$id_key}) before calling load; if the ID
is missing, handle it explicitly (throw a clear exception or return null/false
per surrounding contract) instead of calling load(NULL) so pre-v3 or invalid
stubs produce a clear failure path.
---
Nitpick comments:
In `@tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php`:
- Around line 90-99: Expand the testEntityCreateAutoExpandsBaseFieldsSetOnStub
test to exercise a base entity-reference field: include a 'roles' entry on the
$stub (in addition to 'name', 'mail', 'status') before calling
$this->core->entityCreate('user', $stub), and add an assertion (e.g.
$this->assertSame([...], $stub->roles)) that the roles property was transformed
by the EntityReferenceHandler; this ensures the EntityReferenceHandler runs the
same pipeline as DefaultHandler and will fail if it is skipped.
- Around line 66-76: Add an assertion that the ID populated on the stub matches
the ID of the created entity: after calling $this->core->entityCreate('user',
$stub) and before deleting, assert that the created entity's id() (or the ID
getter used by the returned EntityInterface instance, referenced as $created)
equals the stub's uid property ($stub->uid) to ensure the stub was populated
with the actual storage ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73c60999-c478-439a-8743-5c11e5e470be
📒 Files selected for processing (2)
src/Drupal/Driver/Core/Core.phptests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php
… via 'user.roles' and 'commerce_product.variations'.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityCreateCommerceKernelTest.php (1)
123-126: Reset entity storage cache before asserting the persisted relationship.The test is meant to verify the saved product state. Clearing the product storage cache before loading avoids accidentally asserting against a cached entity instance.
Proposed test-strengthening change
- $product = Product::load((int) $product_stub->product_id); + $product_storage = $this->container->get('entity_type.manager')->getStorage('commerce_product'); + $product_storage->resetCache([(int) $product_stub->product_id]); + $product = $product_storage->load((int) $product_stub->product_id); $this->assertInstanceOf(Product::class, $product); $variation_ids = array_map(intval(...), $product->getVariationIds());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityCreateCommerceKernelTest.php` around lines 123 - 126, The test should clear the product entity storage cache before reloading to ensure you assert against persisted data rather than a cached instance; in CoreEntityCreateCommerceKernelTest, obtain the commerce_product storage via the entity type manager (e.g. getStorage('commerce_product')) and call resetCache() on it before calling Product::load((int) $product_stub->product_id), then proceed with $this->assertInstanceOf and the variation id extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityCreateCommerceKernelTest.php`:
- Around line 123-126: The test should clear the product entity storage cache
before reloading to ensure you assert against persisted data rather than a
cached instance; in CoreEntityCreateCommerceKernelTest, obtain the
commerce_product storage via the entity type manager (e.g.
getStorage('commerce_product')) and call resetCache() on it before calling
Product::load((int) $product_stub->product_id), then proceed with
$this->assertInstanceOf and the variation id extraction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d05f9c8-4f91-4293-b91f-1a045a4edde2
📒 Files selected for processing (3)
composer.jsontests/Drupal/Tests/Driver/Kernel/Core/CoreEntityCreateCommerceKernelTest.phptests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php
✅ Files skipped from review due to trivial changes (1)
- composer.json
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.php
… the resolved id key.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #270
Summary
Fixes two bugs in the generic
entityCreate()/entityDelete()pipeline that blocked setting base entity-reference fields on entity stubs (e.g.commerce_product.variations,user.roles) and caused stub-based round-trips to fail for any entity type whose id key isn't literallyid.expandEntityFields()previously only processed configured fields (FieldStorageConfig); base fields were filtered out unless the caller explicitly passed them in$base_fields.entityCreate()never did, so base entity-reference fields set on the stub reached entity storage in their raw scalar form and failed to resolve targets by label. The method now auto-detects base fields present as properties on the stub (excluding the id and bundle keys, which are system-managed) and routes them through the field-handler pipeline alongside configured fields.entityCreate()populated$entity->idandentityDelete()loaded from$entity->id, hardcoding the generic property name. For entity types whose id key isn'tid(user→uid,node→nid,taxonomy_term→tid, plus most commerce and custom types), this broke stub-based round-trips and was inconsistent withnodeCreate/userCreate/termCreate, which already populate the proper id key. Both methods now resolve the id key via$definition->getKey('id')and use it consistently.v3.x behaviour change:
entityCreate()now populates$entity->$id_key(e.g.$stub->uidfor user entities) instead of$entity->id. Consumers that read$entity->idafterentityCreate()must switch to$entity->$id_keyor read from the returnedEntityInterface.Changes
src/Drupal/Driver/Core/Core.phpexpandEntityFields(): merges explicitly passed$base_fieldswith a new auto-detected set before callinggetEntityFieldTypes().detectBaseFieldsOnEntity(): walksEntityFieldManager::getBaseFieldDefinitions()and returns the base field names that exist as properties on the stub, excluding the entity type's id key and bundle key.entityCreate(): looks up the entity type definition once, resolves$bundle_keyand$id_keyfrom it, and populates$entity->$id_key = $created_entity->id()after save. The bundle-validation guard andstep_bundlepromotion are unchanged.entityDelete(): when a stub is passed (rather than a loadedEntityInterface), resolves the id key the same way and loads via$entity->$id_key.tests/Drupal/Tests/Driver/Kernel/Core/CoreEntityMethodsKernelTest.phptestEntityCreateAndDeleteWithStub: asserts$stub->uidis populated and$stub->idis NOT, and uses$stub->uidfor the round-tripentityDelete().testEntityCreateAutoExpandsBaseFieldsSetOnStub: callsentityCreate('user', $stub)with a plain scalarname, then asserts$stub->name === ['uma']— provingDefaultHandler::expand()ran on the base field.Before / After
Summary by CodeRabbit
Bug Fixes
Tests
Chores