Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

minor: add $persistentManagerName param #450

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from

Conversation

nikophil
Copy link
Member

@nikophil nikophil commented Apr 15, 2023

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 馃し

/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 馃槥

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

@nikophil nikophil requested a review from kbond April 15, 2023 15:30
@kbond
Copy link
Member

kbond commented Apr 16, 2023

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?

@nikophil
Copy link
Member Author

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...

@kbond
Copy link
Member

kbond commented Apr 16, 2023

What about phpstan?

@nikophil
Copy link
Member Author

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

@kbond
Copy link
Member

kbond commented Apr 16, 2023

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

@kbond
Copy link
Member

kbond commented Apr 16, 2023

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

@nikophil
Copy link
Member Author

ok, this seems reasonable :)

I feel the same, otherwise it would restrict too much further updates... see this issue as well #445 (comment)

@kerstvo
Copy link

kerstvo commented Apr 16, 2023

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

@kbond
Copy link
Member

kbond commented Apr 17, 2023

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).

@nikophil
Copy link
Member Author

nikophil commented Apr 28, 2023

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?

@kbond
Copy link
Member

kbond commented Apr 28, 2023

So ModelFactory would look like?

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

@nikophil
Copy link
Member Author

That's what I'm suggesting :)

@kbond
Copy link
Member

kbond commented Apr 28, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Can I specify $persistentManagerName for repository?
3 participants