Skip to content

Add LookupStrategy to DoctrineWriter (port of #11 for 2.x) - #31

Merged
slashrsm merged 1 commit into
masterfrom
feature/lookup-strategy-2.x
Jul 23, 2026
Merged

Add LookupStrategy to DoctrineWriter (port of #11 for 2.x)#31
slashrsm merged 1 commit into
masterfrom
feature/lookup-strategy-2.x

Conversation

@slashrsm

Copy link
Copy Markdown
Contributor

Summary

Port of stale #11 (“Add lookup strategy to writer”, ddeboer, 2017) onto current 2.0 master (PHP ^8.2, portphp ^2, doctrine/common ^3 / Doctrine\Persistence).

Related: #3 (custom QueryBuilder lookups), discussion in #9.

What lands

Piece Role
Port\Doctrine\LookupStrategy Interface: lookup(array $item): ?object
LookupStrategy\FieldsLookupStrategy Default field/criteria strategy
DoctrineWriter::withLookupStrategy() Factory for custom strategies
Optional 5th ctor arg ?LookupStrategy Same as factory, without deprecations

Default / BC behavior (2.x)

No intentional BC break for existing 2.x call sites:

  • new DoctrineWriter($om, $class) — unchanged
  • new DoctrineWriter($om, $class, $index, $lookupMethod) — unchanged
  • truncate = true (default) still always creates a new instance (no lookup)
  • disableTruncate() + no index → ObjectRepository::find(current($item))
  • disableTruncate() + index → repository method (default findOneBy) with field criteria
  • Lookup miss (null) → create new instance (upsert-friendly)

$index / $lookupMethod are not deprecated in 2.x; LookupStrategy is additive. Deprecating those args can wait for a 3.0 discussion.

Fixes vs original #11

The 2017 branch was incomplete and could not be cleanly rebased (CONFLICTING). Notable bugs fixed in this port:

  1. withLookupFields mutated $this instead of the clone
  2. Called missing withIndex() on FieldsLookupStrategy
  3. Factory passed LookupStrategy as the 4th ctor arg ($lookupMethod string) — type error / broken API
  4. Factory argument order was ($objectName, $om, $strategy); now ($om, $objectName, $strategy) to match the constructor
  5. findOrCreateItem returned null from strategy without creating a new entity (regression)
  6. Default strategy used identifier fields instead of master’s empty-fields → find(current($item)) behavior
  7. Namespaces modernized (Doctrine\Persistence), PHP 8.2 types, PHPUnit 9

Usage

// Existing (still supported)
$writer = new DoctrineWriter($em, User::class, 'email');
$writer->disableTruncate();

// Custom strategy (e.g. QueryBuilder — fixes #3)
$writer = DoctrineWriter::withLookupStrategy(
    $em,
    User::class,
    new class($em) implements \Port\Doctrine\LookupStrategy {
        public function __construct(private $em) {}
        public function lookup(array $item): ?object {
            return $this->em->createQueryBuilder()
                ->select('u')->from(User::class, 'u')
                ->where('u.email = :email')->setParameter('email', $item['email'])
                ->getQuery()->getOneOrNullResult();
        }
    }
);
$writer->disableTruncate();

Tests

  • Full suite green locally: 16 tests, PHP 8.5.8 / PHPUnit 9.6
  • Covers factory, custom strategy lookup/miss, constructor $index path, FieldsLookupStrategy immutability, invalid method

Original #11

Please close #11 as superseded once this merges (do not merge #11).

Checklist

  • BC defaults match 2.0 master writer behavior
  • PHPUnit green locally
  • CI matrix 8.2–8.5 green
  • Copilot / human review
  • Do not merge until maintainer (@slashrsm) confirms

Rebase/port of #11 onto the
2.0 line (PHP ^8.2, doctrine/persistence, typed writer APIs).

Adds:
- Port\Doctrine\LookupStrategy interface
- Port\Doctrine\LookupStrategy\FieldsLookupStrategy (default)
- DoctrineWriter::withLookupStrategy() factory
- Optional 5th constructor arg for a custom strategy

BC preserved for 2.x:
- Constructor ($objectManager, $objectName, $index, $lookupMethod) unchanged
- Default truncate=true still creates new instances without looking up
- disableTruncate + empty index still uses repository find(current($item))
- disableTruncate + $index still uses findOneBy (or custom method) criteria
- Null lookup results still create a new entity (original #11 regressed this)

Fixes in the port vs original #11:
- withLookupFields mutates the clone (was mutating $this)
- withIndex implemented (original called a missing method)
- Factory arg order matches constructor (ObjectManager first)
- Factory no longer passes LookupStrategy as the $lookupMethod string
- lookupMethod stays a method name string (not a mixed callable)
- Doctrine\Persistence namespaces; PHP 8.2 types
- No deprecation of $index/$lookupMethod in 2.x (additive API)

Closes #3 (custom strategies can use QueryBuilder).
Supersedes #11.
@slashrsm
slashrsm requested a review from Copilot July 23, 2026 08:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@slashrsm
slashrsm requested a review from Copilot July 23, 2026 08:42
@slashrsm

Copy link
Copy Markdown
Contributor Author

@copilot please review this PR when quota allows.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

2 participants