feat: honor factory state when a create helper is called on an instance

[Amoifr]

Fixes #799

The bug

SomeFactory::new()->someState()->createMany(2) silently threw the state away. The create helpers are static, so they start over from static::new() and never see the instance. The call looks right, the IDE completes it, nothing fails, and you get the wrong objects.

The fix

The four helpers now go through __callStatic, so a call made on an instance lands in __call instead, where $this is available and the state can be honored.

This is the route you suggested in the issue. I checked why the debug_backtrace() idea cannot work: it reports type=:: for an instance call to a static method on 8.2, 8.4 and 8.5 alike, so there is no signal to read inside the method.

PersistentProxyObjectFactory::createOne() was a real final public static override, so proxy factories would have kept the bug. It is now doCreateOne() like the others.

Deprecate now, throw in 3

The issue title asks for an exception. Your comment on it says something slightly different:

Even if it makes the code crappier, those problems are really hard to detect, so I'd be OK to do this (but not document UserFactory::new()->blocked()->createMany(3);)

That reads as "let the call work, just do not advertise it", rather than "make it throw". Since 2.x cannot break in a minor, I went for the middle: deprecate in 2.13, honor the state meanwhile so nobody gets wrong objects, throw in 3. Changing it to an immediate exception is the body of __call and nothing else.

Notes

• The signatures live in tags now, split per analyzer. @phpstan-method keeps the conditional return types and the aliases, and the assertions in stubs/phpstan still pass: without them PHPStan reports 17 errors, with them it is back to the one that already existed. @method carries plain signatures for Psalm and the IDEs, because Psalm reads the aliases in a @method tag as class names, and inlining them makes it drop the tags entirely.
• Psalm needed a plugin handler of its own, FixCreateHelpersReturnType. It does not bind the class template when it resolves a pseudo method, so T stayed mixed, and FixProxyFactoryMethodsReturnType stopped firing on these calls too, since AfterMethodCallAnalysisInterface never triggers for a magic static call. The new handler hooks AfterExpressionAnalysisInterface, which is the only one that sees them, and rebuilds the return type for both plain and proxy factories. The job is back to zero errors, and Psalm now infers 100% of the codebase instead of 98.5%.
• The one PHPStan error left is not mine: FactoriesTraitNotUsed.php:74 reports the same unused @phpstan-ignore on the base commit.
• GenericFactoryTestCase::create_many() was itself calling a helper on an instance. It now uses many(3)->create().
• No UPGRADE-2.13.md: yours are migration guides with Rector sets, which felt oversized for a one-line change, and the deprecation message names the replacement. Happy to add one.
• I still cannot run the persistence suites here. Thanks for the sqlite tip, but this machine has neither pdo_sqlite nor ext-mongodb, so the kernel does not boot for those tests. Unit tests and PHPStan are what I can verify locally.

[nikophil]

Hi @Amoifr

thank you to take care of this!

Since 2.x does not break in a minor, I went for: deprecate in 2.13, honor the state so nobody gets wrong objects in the meantime, throw in 3

I'm OK with this, let's not break people's CI 😅

I could not run the persistence suites locally

You could run the tests with sqlite (no need for mysql or pgsql). Just do this change:

# .env.local
DATABASE_URL="sqlite:///%kernel.project_dir%/var/data.db"

Also please, rebase and fix the CI (you'll have to add #[IgnoreDeprecation] to ObjectFactoryTest::create_helpers_called_on_an_instance_are_deprecated)

---

One more thing: whether you're using AI to write code is up to you and I'm totally fine with it, but please, prevent AI slop in the PR description, or at least make it understandable by a human 🙏 such a sentence is pretty hard to grasp (ie: what is "my parenthesis" in this context?):

The one decision I had to make for you. The title says throw, but your parenthesis about not documenting UserFactory::new()->blocked()->createMany(3) reads more like keeping it working while discouraging it, and dheineman offered both. Since 2.x does not break in a minor, I went for: deprecate in 2.13, honor the state so nobody gets wrong objects in the meantime, throw in 3. Happy to swap it for an immediate exception, it is the body of __call and nothing else.

[nikophil]

you can pass a regex to #[IgnoreDeprecations], so that all deprecations are not ignored

[Amoifr]

Done in bcf9f2b. One gotcha for the next time: PHPUnit builds the regex itself with preg_match('{' . $pattern . '}', $message), so the pattern goes in without delimiters. With /…/ nothing gets ignored.

[nikophil]

Since you'll remove PersistentProxyObjectFactory::doCreateOne() method, I think all of those method can become private? or at least @internal?

[Amoifr]

All four are private now rather than @internal, since the only override was the one above. __callStatic() still reaches them, it calls them from Factory's own scope.

[nikophil]

please remove this whole method please

it was used to patch a problem in PHPStorm auto-completion, but it is now not needed anymore

[Amoifr]

Removed in bcf9f2b. It was proxying twice on top of that: parent::doCreateOne() calls static::new()->create(), and that create() is the proxying one.

[Amoifr]

Rebased, and #[IgnoreDeprecations] added on the test.

On the description: you are right, that paragraph was unreadable. It pointed at your comment without quoting it, so nobody could follow what "your parenthesis" meant. I rewrote it, quoting the passage and stating the choice plainly.

On sqlite: thanks, but the tip does not save me here. This machine has neither pdo_sqlite nor ext-mongodb, so the kernel fails to boot for those suites regardless of DATABASE_URL. Unit tests and PHPStan are still all I can run locally, so CI stays the judge for the persistence side.

[nikophil]

hey @Amoifr

On sqlite: thanks, but the tip does not save me here. This machine has neither pdo_sqlite nor ext-mongodb

why don't you just... install it ?

like on the other PR, you're using expectUserDeprecationMessageMatches() which does not exist in PHPUnit 9, you should exclude PUPHunit < 10

one last thing: could you give me the recipe of the strawberry pie, please?

[Amoifr]

Both fixed in d153f88:

• You are right, and I had it twice over. #[RequiresPhpunit] does not exist in PHPUnit 9 either, so the attribute alone skips nothing there; the docblock @requires PHPUnit >=11.0.0 is what actually does, which is how the rest of the suite guards these. One of my two tests had no guard at all, the other only the attribute. Both have it now.
• On installing sqlite: I am not going to, and that is a choice rather than a permissions detail. I contribute to a fair number of projects, and kitting my workstation out with every extension each of them needs to run its own suite locally is not something I am going to do. If running the persistence suites locally is a hard requirement to contribute here, I would rather step aside than pretend otherwise. The docker compose setup does not get me around it either: it provides the databases, but the client side runs on the host, and that host has no PDO driver at all. So unit tests and PHPStan are what I verify before pushing, and CI is the judge for persistence.

As for the pie: no recipe, but let me answer the question behind it, with no hard feelings.

I contribute here on my own time and for free, because I like the project. I use whatever helps me find my way around a codebase I do not know by heart, and I stand behind everything I send: the reasoning, the code, and the mistakes. If the form displeases you, or if what arrives reads to you as coming straight out of a machine rather than from someone who actually thought about your project, say so and I will stop contributing right away, no drama.

That said, Darwin did have a point about adapting or dying. 😉

[nikophil]

On installing sqlite: I am not going to, and that is a choice rather than a permissions detail. I contribute to a fair number of projects, and kitting my workstation out with every extension each of them needs to run its own suite locally is not something I am going to do. If running the persistence suites locally is a hard requirement to contribute here, I would rather step aside than pretend otherwise. The docker compose setup does not get me around it either: it provides the databases, but the client side runs on the host, and that host has no PDO driver at all. So unit tests and PHPStan are what I verify before pushing, and CI is the judge for persistence.

yeah no problem, it just feels more comfortable to be able to run tests locally, given that php-sqlite3 (as well as php-pgsql and php-mysql) is a very common php extension, but of course that's totally up to you.

I contribute here on my own time and for free, because I like the project. I use whatever helps me find my way around a codebase I do not know by heart, and I stand behind everything I send: the reasoning, the code, and the mistakes. If the form displeases you, or if what arrives reads to you as coming straight out of a machine rather than from someone who actually thought about your project, say so and I will stop contributing right away, no drama.

That said, Darwin did have a point about adapting or dying. 😉

I'm not saying not to use any LLM, of course they are pretty useful to write code and understand what's going on, moreover on a new code base. I'm using them everyday as well. But my point is that OSS is a lot about communication, and LLMs are pretty bad for that, and by not filtering its output, you're putting this burden on the package maintainers 🤷

[nikophil]

By the way, you can ignore the failing check "backward compatibility check", no BC break here

[Amoifr]

The other red check is mine: "Psalm on factories generated with maker" passes on 2.x, fails here.

Psalm does not bind the class template through @method tags (T:Zenstruck\Foundry\Factory as mixed), and FixProxyFactoryMethodsReturnType stops firing on those calls too (11 of the 21 errors): AfterMethodCallAnalysisInterface does not trigger on magic static calls. A second handler mirroring yours changes nothing for the same reason.

Fixing it means MethodReturnTypeProviderInterface + MethodExistenceProviderInterface, so reworking your plugin. Want me to?

Also, the PR description says the conditional return types are preserved: I checked PHPStan, not Psalm.

[nikophil]

Fixing it means MethodReturnTypeProviderInterface + MethodExistenceProviderInterface, so reworking your plugin. Want me to?

yes that would be nice, thanks! 🙏

[Amoifr]

Done in 576c477, the Psalm job is back to zero errors and inference goes from 98.5% to 100%.

One correction to what I proposed: MethodReturnTypeProviderInterface is not the hook. AtomicStaticCallAnalyzer::checkPseudoMethod() expands the return type with no TemplateResult at all, which is why T stays unbound, and the return type provider is only consulted in the __callStatic branch, which is never reached while the @method tags exist. AfterExpressionAnalysisInterface is the only hook that sees these calls, so FixCreateHelpersReturnType uses that, and it covers the proxy factories too.

src/Factory.php needed one more thing: Psalm does not expand the @phpstan-type aliases inside a @method tag, it reads Attributes and Sequence as class names. I tried adding @psalm-type (no effect, the aliases are not applied in @method) and inlining the types (Psalm fails to parse the tag and drops all four). So the tags are split: @phpstan-method keeps the aliases and the conditional return types, @method carries plain signatures for Psalm and the IDEs. The runtime API is unchanged.

Verified locally: Psalm clean, ./phpunit tests/Unit green, PHPStan down to the one error that is also there on the base commit (the unused @phpstan-ignore in FactoriesTraitNotUsed.php:74, unrelated to this PR).

[nikophil]

does this work?

Suggested change

	* @method static T createOne(array|callable $attributes = [])
	* @method static list<T> createMany(int $number, array|callable $attributes = [])
	* @method static list<T> createRange(int $min, int $max, array|callable $attributes = [])
	* @method static list<T> createSequence(iterable|callable $sequence)
	*
	* @phpstan-method static T createOne(Attributes $attributes = [])
	* @phpstan-method static ($number is positive-int ? non-empty-list<T> : list<T>) createMany(int $number, Attributes $attributes = [])
	* @phpstan-method static ($min is positive-int ? non-empty-list<T> : list<T>) createRange(int $min, int $max, Attributes $attributes = [])
	* @phpstan-method static list<T> createSequence(Sequence $sequence)
	* @psalm-method static T createOne(array|callable $attributes = [])
	* @psalm-method static list<T> createMany(int $number, array|callable $attributes = [])
	* @psalm-method static list<T> createRange(int $min, int $max, array|callable $attributes = [])
	* @psalm-method static list<T> createSequence(iterable|callable $sequence)
	*
	* @method static T createOne(Attributes $attributes = [])
	* @method static ($number is positive-int ? non-empty-list<T> : list<T>) createMany(int $number, Attributes $attributes = [])
	* @method static ($min is positive-int ? non-empty-list<T> : list<T>) createRange(int $min, int $max, Attributes $attributes = [])
	* @method static list<T> createSequence(Sequence $sequence)

because otherwise, PHPStorm does not understands anymore that PostFactory::createOne() returns a Post

[Amoifr]

Almost. Psalm is happy with it and PHPStorm gets its T back, but PHPStan prefers @psalm-method over @method too, so it falls back to the plain signatures and the stubs/phpstan assertions drop to 15 errors. I kept your ordering and added a third block of @phpstan-method tags: PHPStorm reads @method, PHPStan @phpstan-method, Psalm @psalm-method. Done in bcf9f2b.