Sitelet https://web.archive.org/web/20201206200116/https://github.com/nunomaduro/phpinsights/issues/370
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for custom presets #370

Open
olivernybroe opened this issue Mar 5, 2020 · 3 comments
Open

Add support for custom presets #370

olivernybroe opened this issue Mar 5, 2020 · 3 comments

Comments

@olivernybroe
Copy link
Collaborator

@olivernybroe olivernybroe commented Mar 5, 2020 •

Q A
Bug report? no
Feature request? yes

Right now we can create custom configuration based on presets, however I think it would be worth allowing custom presets also.
This would mean opening up the Preset interface so for example a framework could maintain their own Preset file and users can then use the preset, just by pointing their preset configuration to the class.

Implementation

  • In Configuration::resolveConfig, the OptionsResolver now has to allow preset to also be a valid class.
  • In ConfigResolver::resolve, add a new private method resolvePreset, which has the current logic for resolving a preset, but also adds support for the preset variable could be a class fqn.
  • Add docs
  • Add a test in ConfigResolverTest for setting preset from a class
  • Remove internal from phpdoc in Preset interface
  • Consider if we should also remove internal from Composer class
  • Remove shouldBeApplied and getName from Preset interface and add it to a new internal interface, which our presets implements. (this method is used for guessing the preset from composer, but does not make sense with custom preset as their are not registered in our application.)

Usage

Using this should be rather straight forward, creating a custom Preset should be like this

class LaravelLumenPreset implements Preset
{
    public static function get(Composer $composer): array
    {
        $config = [
            // My custom configs
        ];

        return ConfigResolver::mergeConfig(LaravelPreset::get($composer), $config);
    }
}

And our config file would then look like

return [
    'preset' => FQN\LaravelLumenPreset::class,
     ...
@Jibbarth
Copy link
Collaborator

@Jibbarth Jibbarth commented Mar 7, 2020

👍 I'm totally okay with this feature request !

@Andrew-Shook
Copy link

@Andrew-Shook Andrew-Shook commented Oct 11, 2020 •

So, I've done some work on this request, but I want to make sure my implementation wasn't going to rock the boat too much before I go any further. I'm proposing a change to how the Configuration object holds and validates the preset option. Since all preset should implement the Preset Interface, I think the OptionsResolver should check the preset value using a closure to determine if the 'preset' option is the name of a class implementing the interface. This also means that the PRESET const in the Configuration could go away since it would no longer need to know the package maintained presets. It would be up to the ConfigurationResolver to map the package's presets from just a name to classes. Thoughts?

@olivernybroe
Copy link
Collaborator Author

@olivernybroe olivernybroe commented Oct 12, 2020

@Andrew-Shook, hmm, I don't mind changing stuff there at all.

However I am not sure I totally follow how this would work then.

Right now if you supply no preset, it will run the guess in the config resolver. All of the predefined presets have a method for determine if they should be activated.

If we remove the presets constants, how would the config resolver know which presets to check this method on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.