search

vierge-noire / cakephp-fixture-factories

CakePHP Fixture Factories
https://vierge-noire.github.io/
MIT License
85 stars 20 forks source link

Enhancements to configuration: Grouped configuration settings & custom bake paths #182

Closed jamisonbryant closed 2 years ago

jamisonbryant commented 2 years ago

References #178

My app wants to bake the fixture factories into a separate directory than the default, only I found that that was not possible in the current plugin. This includes a patch to fix that (with tests). Users can now set testFixtureOutputDir to override the output directory where factories get baked.

Secondarily, and very importantly, all configuration settings for the plugin have been namespaced. This is something I brought up in my original feature ticket (linked above) and no one seemed to have a problem with it so I went for it. It is common practice for plugins to keep their configuration settings out of the global namespace, and so I have moved all configuration settings for this plugin to the FixtureFactories namespace:

[
    'FixtureFactories' => [
        'testFixtureGlobalBehaviors' => [...],
        'testFixtureOutputDir' => 'my/output/dir',
    ],
]

The latter is not a backwards compatible change, so beware.

jamisonbryant commented 2 years ago

The three PHPStan errors are also showing up on the next branch, they were not introduced in this branch.

I believe commands are supposed to return $this->abort('msg') instead of calling the function and then returning something else, which is what it looks like BakeFixtureFactory and Setup are doing.

I think these two issues should possibly be addressed in a separate PR, thoughts?

jamisonbryant commented 2 years ago

@pabloelcolombiano I have made the requested changes, and also fixed the PHPStan errors. All tests are still passing even though I removed some returns (they weren't getting hit anyway because abort() throws a StopException).

I noticed you also have Psalm configured, that is currently not passing although it's because it is not inferring the same return type on two methods in the BaseFactory. I feel like its inferred return type is less detailed than the documented one, and so no change is necessary. More detail better than less. Let me know if you think different.

Ready for re-review.