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.

Sign up for GitHub

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

Jump to bottom

feat: Add fluent model methods to config classes #11591

Merged
merged 23 commits into from
Jul 22, 2022
Merged

feat: Add fluent model methods to config classes #11591

merged 23 commits into from
Jul 22, 2022

Conversation

khalwat
Copy link
Contributor

@khalwat khalwat commented Jul 13, 2022 •
edited
Loading

Description

This PR adds fluent model methods to the GeneralConfig and DbConfig objects, allowing you to write config files like this:

        return GeneralConfig::create()
            ->accessibilityDefaults([])
            ->devMode(true)
            ->allowAdminChanges(true)
            ->cacheDuration(120);
        return DbConfig::create()
            ->database()
            ->charset('utf8');

This gives you:

  • Auto-complete type hinting for all of the config file properties
  • Type checking for the parameters you pass into the config file properties
  • Inline documentation for all of the config file properties

...and makes it work similar to how the Craft Query class works currently, with both properties and fluent model methods of the same name, both fully annotated.

general-config-demo.mov

It also adds a BaseConfig object with a BaseConfig::create() factory method to make it nicer to work with when using the fluent model methods.

Also added craft\base\FluentModelTrait that is used in BaseConfig to allow fluent model methods to work via __call() magic method if no actual fluent model method is provided in the config class. It is preferred to add actual methods to the config class, to avoid the magic method overhead, but more importantly, to provide multi-line inline documentation via the method docblock, but the fallback is nice to have.

There should be no breaking changes from this PR, it just adds to the existing functionality.

Related Issues

Working with arrays in PHP is troublesome, especially for things like config files where a large number of specific keys are needed. It's easy to make typos, there's no immediate feedback on the type correctness, and you spend quite a bit of time going back and forth between the documentation and the config file.

This implementation should also make it easier to deprecate config file values that are removed or renamed.

Alternatives

  • I initially tried using PhpStorm's ArrayShape annotations on the GeneralConfig::__construct() method's $config parameter, however they don't allow for per-array element comments, so we are missing the inline documentation, which is critically important for config file settings

  • I had also tried implementing a fluent model using PHP magic methods, but @method and @property annotations only allow for a single line documentation, so we'd be missing out on the GeneralConfig class's copious documentation, and also then we have magic method overhead for setting each config property

@khalwat khalwat requested a review from a team as a code owner July 13, 2022 02:24
@khalwat
Copy link
Contributor Author

khalwat commented Jul 14, 2022 •
edited
Loading

So I really wanted to change BaseConfig to extend from craft\base\Model instead of the current yii\base\BaseObject because:

  • Validation rules could be added for GeneralConfig and DbConfig, etc.
  • Plugin developers could then have their Settings models extend from BaseConfig, and write the fluent model methods so they wouldn't have to create a separate config.php file that's a copy & paste of the Settings model properties, and could benefit from the same type hinting/autocomplete goodness

Unfortunately, DbConfig has a $attributes property, which then results in an attributes() method, which conflicts with the yii\base\Model implementation of that method.

Possible solution: rename the $attributes property in DbConfig -- but I'm assuming that'd be a breaking change, which is a bummer.

Wasn't sure how to solve this in a way you'd be happy with, so just leaving notes here in case you want to think about it and do something with it.

@bencroker
Copy link
Contributor

bencroker commented Jul 15, 2022 •
edited
Loading

Unfortunately, DbConfig has a $attributes property, which then results in an attributes() method, which conflicts with the yii\base\Model implementation of that method.

I don't see a problem with this, one is a property and the other is a method. The only issue is if a plugin/module is calling the magic getter attributes() on DbConfig, which seems highly unlikely. I'd suggest renaming the $attributes property in DbConfig, leaving the old property deprecated until Craft 5.

UPDATE: Ah I see the issue now, it conflicts with the fluent model method.

@khalwat
Copy link
Contributor Author

khalwat commented Jul 21, 2022

So I really wanted to change BaseConfig to extend from craft\base\Model instead of the current yii\base\BaseObject because:

Resolved this by:

  • BaseConfig now extends from craft\base\Model, so plugins can extend their Settings models from it
  • Refactored the $renamedSettings property to be a protected property of the BaseConfig class so all config classes can use it
  • Refactored the magic get/set methods from GeneralConfig to BaseConfig so all config classes can use it, to handle config properties that are renamed
  • Refactored $attributes to $pdoAttributes & pdoAttributes() in the DbConfig class
  • Added "attributes" => "pdoAttributes" to $renamedSettings[] in DbConfig to handle the renamed config property
  • Added public static $configCategory to BaseConfig which is the config category, to be overridden by anything subclassing it, so the deprecation/renamed log messages are correct

@brandonkelly brandonkelly changed the base branch from develop to 4.2 July 21, 2022 22:22
@brandonkelly brandonkelly mentioned this pull request Jul 21, 2022
Merged
@brandonkelly brandonkelly merged commit 1ddef26 into craftcms:4.2 Jul 22, 2022
@brandonkelly
Copy link
Member

Merged via #11656!

@brandonkelly
Copy link
Member

4.2.0 is out with this!

@khalwat khalwat mentioned this pull request Jul 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@khalwat @bencroker @brandonkelly