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

Configuration class maintainability #34

Open
redthor opened this issue Apr 29, 2018 · 1 comment
Open

Configuration class maintainability #34

redthor opened this issue Apr 29, 2018 · 1 comment

Comments

@redthor
Copy link
Contributor

redthor commented Apr 29, 2018

Scrutinizer gives Configuration class an F:
https://scrutinizer-ci.com/g/doesntmattr/mongodb-migrations/code-structure/master/class/AntiMattr%5CMongoDB%5CMigrations%5CConfiguration%5CConfiguration

It is not hard to see why.

There is this confusing inheritance model:

       Configuration (concrete)
            |
            v
   AbstractFileConfiguration (abstract)
      |                |
      v                v
XmlConfiguration YamlConfiguration  (concrete)

And there are a bunch of mutators on Configuration just so that the bundle can overwrite settings here: https://github.com/doesntmattr/mongodb-migrations-bundle/blob/master/Command/CommandHelper.php#L38

Suggest we apply:

  • Composition over inheritance;
  • Apply the single responsibility pattern;
  • Immutable configuration;
Class Responsibility
ConfigurationBuilder Create an immutable Configuration class
ConfigurationLoader Picks the way the configuration is loaded: `yaml
YamlLoader Reads YAML file. May not be required if we are just using Syfmony Yaml classes
XmlLoader Reads XML file. May not be required if it is too simple
ContainerLoader Gets container parameters
VersionCollection Container for the versions. Has the responsibility for looking up Versions etc

These are just ideas. Would need to look more into how the Configuration class is used.

Note that improvements here could also improve the Doctrine migrations library https://github.com/doctrine/migrations because it follows the same structure: https://scrutinizer-ci.com/g/doctrine/migrations/code-structure/master/class/Doctrine%5CDBAL%5CMigrations%5CConfiguration%5CConfiguration

@redthor
Copy link
Contributor Author

redthor commented May 2, 2018

I wonder if the better way to deal with the Bundle, is rather than use all those mutators on Configuration that the Symfony Commands are ContainerAware and that they get the mongodb-migrations Configuration (or something like it) from the container. Then we set up a services.xml file with classes required.

That way they could be overridden (not sure how useful that would be) by the bundle user. Obviously it'll mean we can remove all the mutators and just get the configuration created once.

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

No branches or pull requests

1 participant