foundry icon indicating copy to clipboard operation
foundry copied to clipboard

minor: add $persistentManagerName param

Open nikophil opened this issue 2 years ago • 13 comments

fixes #447

I've not provided tests, because the code is trivial, but this would need to add a new document manager in the test :shrug:

/cc @kerstvo

EDIT: I just realized that adding a param to ModelFactory::repository() will create an error in psalm in userland, because of the @phpstan-method annotation :disappointed:

https://github.com/zenstruck/foundry/actions/runs/4707671150/jobs/8349671474?pr=450#step:8:18

nikophil avatar Apr 15 '23 12:04 nikophil

I just realized that adding a param to ModelFactory::repository() will create an error in psalm in userland

You mean on existing factories? I guess we should we update the maker/docs?

kbond avatar Apr 16 '23 01:04 kbond

You mean on existing factories?

yes, all existing factories will create an error with psalm if we make this move... We can update update maker and docs, but still, this could be pretty annoying...

nikophil avatar Apr 16 '23 13:04 nikophil

What about phpstan?

kbond avatar Apr 16 '23 13:04 kbond

even in max level, phpstan does not complain. But psalm is actually true: one param is missing in the methods prototype in the phpdoc

nikophil avatar Apr 16 '23 13:04 nikophil

Huh, well I don't want to not add a feature because it breaks static analysis, but that's just my opinion.

kbond avatar Apr 16 '23 14:04 kbond

What about starting a UPGRADE.md with a note for the next feature release?

kbond avatar Apr 16 '23 14:04 kbond

ok, this seems reasonable :)

I feel the same, otherwise it would restrict too much further updates... see this issue as well https://github.com/zenstruck/foundry/issues/445#issuecomment-1500440459

nikophil avatar Apr 16 '23 16:04 nikophil

It seems that problem is more complicated. I apply your changes in my project and got error image

kerstvo avatar Apr 16 '23 19:04 kerstvo

I think we need to have a Factory::withManagerName() where you actually configure the alternate manager name per factory.

This would remove the need for $persistentManagerName in ModelFactory::repository() (and not break static analysis for existing factories).

kbond avatar Apr 17 '23 13:04 kbond

this seems not possible because ModelFactory::repository() is static...

I think this needs to be hardcoded directly in the class, and then have a factory class which is bound to a given manager. ex:

class SomeFactoryForSpecificManager
{
    protected static function getClass(): string
    {
        return SomeEntity::class;
    }

    protected static function getManagerName(): ?string
    {
        return 'specific-manager;
    }
}

@kerstvo would this fix your problem?

nikophil avatar Apr 28 '23 11:04 nikophil

So ModelFactory would look like?

abstract class ModelFactory extends Factory
{
    // ...
    
    protected static function getManagerName(): ?string
    {
        return null;
    }
}

kbond avatar Apr 28 '23 15:04 kbond

That's what I'm suggesting :)

nikophil avatar Apr 28 '23 16:04 nikophil

Since we're deprecating ModelFactory soon, maybe we should just add this feature to #452?

We'll need to update some function parameters and the anon. proxy generator.

kbond avatar Apr 28 '23 18:04 kbond