Silverstripe-framework: Warnings for misspelled yml config?

[sminnee]

From @SilbinaryWolf

I'd like warnings when you add Config to a non-class name. Sometimes you get the classname wrong by one letter and it takes a while to figure out if you're having an off-day

This would be a good idea. It would mean that the top level keys within config should always correspond to a class/trait/interface. Does this seem like a reasonable restriction?

[robbieaverill]

In SS4 if you're using MyClass::class, assuming its imported into the namespace already, there is minimal risk of this happening. In this case _PHP_ would naturally tell you that you've spelled it wrong.

This would be more obvious where you're using a FQ class name as a string instead. We should probably (?) be encouraging the use of MyClass::class instead.

Personally I like the fact that the configuration manifest is flexible enough that it doesn't require a concrete PHP class/trait/interface to bind to.

[sminnee]

The area where this comes up is in the yaml config, not the PHP code:

dumbconf.yml

SilverStripe\Member\Meber:
  really_important_setting: true

As an alternative, we could explicitly indicate which keys are acceptable, with an initial white list of classes implementing Configurable?

[robbieaverill]

A valid consideration would be that if this happens [prevent the ability to use arbitrary config names], how are people likely to work around it when they don't agree? Constants, global variables, custom configuration providers for their modules (that do), sessions...?

[micmania1]

I don't like the idea but I'm not strongly opposed either. I can see it saving a few people, but it just feels like unnecessary coupling. Tests would also capture mis-spellings

[sminnee]

It might be better left until if/when there's more awareness in the system of available config settings

[stevie-mayhew]

I half agree with @SilbinaryWolf and half with @micmania1

What if we build a test which gathers all config yml available and tested them for errors like this, instead of having it be part of runtime?

[robbieaverill]

I would've thought it's more a case of writing your own tests for your own config (not explicitly, but testing that some behaviour works after fetching config).

If you have a global test that checks all defined configuration against valid class names, and you fail a test suite when one doesn't match, you're basically enforcing that rigidity anyway

[tractorcow]

There could also be a separate config validator which can be invoked separately, rather than adding overhead to every request. I do something similar for fluent: https://github.com/tractorcow/silverstripe-fluent/blob/master/code/tasks/FluentValidateTask.php

[dhensby]

Personally I like the fact that the configuration manifest is flexible enough that it doesn't require a concrete PHP class/trait/interface to bind to.

I agree with this and @micmania1

@tractorcow makes a good point - how about a CLI task for looking for Config smells?

This change would also mean the class manifest would have to have run before the Config, which seems the wrong way round. To me the Config should be the first thing to run as perhaps the manifest should be Configurable.

[silbinarywolf]

I'm open to the idea of a validator task as long as it's well documented and tells a user how to integrate it with their workflow simply.

ie. Perhaps document how to make it run the validator task during ?flush=all. That way users can choose to put the error checking overhead in and those new to Silverstripe can quickly get setup and catch bugs that are obvious to an expert but not obvious to a newbie.

That way, I feel like everybody wins.

[dhensby]

Something we are seeing more with SS4 is people using Non-namespaced classes by accident.

ie:

Director:
  rules:
    url: Controller

If we are going to implement this, it would be nice to alert people to that on dev/build or flush...

[robbieaverill]

There are also some cases I've seen so far where a non-concrete config section is deliberately not namespaced to either keep it simpler or reduce upgrade burden. I guess it might have been a case of "it's not a class name anyway, so no need to namespace it"