Factories

[MGatner]

Description
Defines a new dynamic lookup class, Factories, for returning shared instances of named components.

Supersedes #3712 (but still read about the ModelFactory bug there, which also applies to the previous Config).

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[MGatner]

See what you all think of this. I will obviously add tests and docs but I want us to iron out the core class first. I would also like to see about implementing it for Config, though might need to tweak how it loads its own configuration if we do so they don't loop.

Also, should this move to system/ root instead of being part of system/Config? Or even be part of system/Autoload?

[michalsn]

I like how it looks. You added some nice config options so it's even better. I am very much into using this wherever it will bring some benefits.

I think it fits good to system/Config - alongside the Services class.

[MGatner]

@michalsn Thanks for the feedback! I have some improvements in the works I'm excited about - I think this will becomes a very versatile core aspect of the framework when it is ready.

[lonnieezell]

@MGatner Looking great! Those methods definitely multiplied over time. Will be great to have this pulled out into a central class.

[MGatner]

Thanks! I keep going back and forth on the names... @michalsn @lonnieezell have any input?

1. (current) Factory class to generate instances that uses a Factories config file.
2. Factories class that uses a Factory config file

IMO (1) is easier to type and makes more sense logically, but (2) is consistent with Services and sets up a nice parallel between the two.

[michalsn]

@MGatner I think I like option 2 more, but I don't mind 1 either. Glad I could help 😅

[lonnieezell]

@MGatner I'm ok with either, but do agree that option 2 parallels existing stuff in the framework better, keeping a nice consistency. And a decent IDE makes typing lots of things even easier than the naming itself. :)

[MGatner]

Glad to hear support from both of you because I already changed it :) Got some test failures to work through and working on the User Guide, then this should be set for review.

[MGatner]

Failing tests again :/ I don't have more time to work on this now - if anyone can figure out why the failures and post back I can take another shot at it later in the week.

[MGatner]

Okay I see the problem: App\Config\Whatever is not a namespace path because we use Config\Whatever - I will have to add some special handling for that.

[MGatner]

Down to what appears to be a discrepancy with ReflectionClass::getStaticPropertyValue() between PHP versions. I might need to find a way around using that.

[MGatner]

Test failure is unrelated.

[paulbalandan]

User guide syntax check still fails. Other than the phpstan ignored message, I approve. ;)

[michalsn]

I think it looks really good.

We should only mention about ModelFactory depreciation in the changelog.

[MGatner]

User guide syntax check still fails.

@paulbalandan Any ideas on what is still wrong?

[MGatner]

We should only mention about ModelFactory depreciation in the changelog.

I added a note for both ModelFactory and Config

[michalsn]

Could you also add this to the changelog in the user guide? I think this is the main one, and the changelog.md is always generated semi-automatically before release.

[MGatner]

Oops! Sorry, will do. I haven't touched the changelogs before, good for me to learn this :D

[paulbalandan]

User guide syntax check still fails.

@paulbalandan Any ideas on what is still wrong?

Found the fault. After the preferApp row, you should add the bottom table borders that matches above borders.

[MGatner]

@paulbalandan @michalsn

[paulbalandan]

Last two. :)

[paulbalandan]

I believe this is mergeable already. @MGatner any last additions?

[MGatner]

I think it's ready, I'm just scared. 😳

[paulbalandan]

I see this as a useful addition. 🎉