Skip to content

Conversation

@hrach
Copy link
Member

@hrach hrach commented Sep 4, 2025

Refs #739

Current status: failing unit tests. If somebody wants to investigate, help is welcome.

@JanTvrdik
Copy link
Member

JanTvrdik commented Sep 6, 2025

I let Claude Code give it a try and this is the solution it came up with:

	/**
	 * @param array<string, mixed> $data
	 * @return E|null
	 */
	public function create(array $data): ?IEntity
	{
		$entity = $this->createEntity($data);
		$id = $this->getIdHash($entity->getPersistedId());

		if (isset($this->entities[$id])) {
			if ($this->entities[$id] === false) {
				$this->repository->detach($entity);
				return null;
			}
			$existingEntity = $this->entities[$id]->get();
			if ($existingEntity === null) {
				// The entity was garbage collected, use the new one
				$this->entities[$id] = \WeakReference::create($entity);
				return $entity;
			}
			$this->repository->detach($entity);
			if (isset($this->entitiesForRefresh[$id])) {
				$this->repository->attach($existingEntity); // entity can be detached because of delete try
				$existingEntity->onRefresh($data);
				unset($this->entitiesForRefresh[$id]);
			}
			return $existingEntity;
		}

		$this->entities[$id] = \WeakReference::create($entity);
		return $entity;
	}

Used prompts:

read last commit; then run 'docker compose exec -T php composer tests'; then try to debug why the test started failing

the goal of the PR is to implement weakreference support in identity map; is there a way to resolve the issue while keeping the weak reference?

@hrach hrach force-pushed the weak-reference-identity-map branch from 535982d to 73048a1 Compare September 7, 2025 17:11
@hrach
Copy link
Member Author

hrach commented Sep 7, 2025

@JanTvrdik wow, thanks! Beyond my expectations 👏 🎉

@hrach hrach marked this pull request as ready for review September 7, 2025 17:12
@hrach hrach merged commit 96cbe6f into main Oct 19, 2025
10 checks passed
@hrach hrach deleted the weak-reference-identity-map branch October 19, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants