search

nette / schema

📐 Validating data structures against a given Schema.
https://doc.nette.org/schema
Other
905 stars 26 forks source link

Add option to prevent merging of arrayOf, listOf defaults #28

Closed colinodell closed 3 years ago

colinodell commented 4 years ago

13 and #24 describe a common unexpected behavior where default values for arrayOf() and listOf() schemas are always merged with the user-provided values. Although there are some uses cases where this is desired, there are others where it is not.

I considered four possible solutions but ultimately feel this approach might be best.

Option 1 - Make the current behavior opt-in

In a nutshell, we could change the current "always merge default" behavior to be opt-in by requiring developers to call a method like ->alwaysMergeDefaults() when defining their schemas. This would make the behavior better align with developers' expectations. Unfortunately, this would cause a major BC-break and therefore probably wouldn't be desireable by the Nette maintainers. (Although if you're open to releasing a new major version to accommodate this approach it would make me very happy :wink:)

Option 2 - Manually inject the Helpers::PREVENT_MERGING value

There seems to be a magic constant that can be used to prevent merging: https://github.com/nette/schema/blob/8956f866e1fea67a7ac8eed187b2c7ae7e3211d1/src/Schema/Helpers.php#L30-L34 Unfortunately, it's inside a class marked as @internal and is therefore considered unsafe for others to use. It's also a bit unwieldy to use outside of this library for this purpose.

Option 3 - Use a custom before() function to apply the defaults if no value is given

This is essentially what was proposed in this comment. Although it does work, it's not very intuitive and is very much a workaround and not a true solution. Upon further review this workaround doesn't provide the desired behavior when using nested structures.

Option 4 - Allow users to opt-out of always merging defaults

This is the approach I'm proposing here. In a nutshell, we expose an additional configuration method called preventMergingDefaults(). When added to a schema, it prevents the unexpected merging behavior as demonstrated in the included tests: https://github.com/nette/schema/blob/76da9f3ed6d34535e0c4684accc3ef93bd0485d0/tests/Schema/Expect.array.phpt#L234-L246

While this doesn't seem as clean as option 1 IMO I think it strikes a good balance between preserving backward-compatibility and being easy and clear to use.

This is an alternative to #14; it resolves #13 and resolves #24

colinodell commented 4 years ago

After doing some more testing I've determined that the workaround posted in #13 doesn't work with nested structures. I've updated this PR description and the test cases to reflect the desired behavior and how this proposed implementation addresses this.

For some additional context: I'm trying to leverage this library in the upcoming league/commonmark 2.0 release but the inability to exclude defaults is causing several issues. For example, we have a setting that allows users to specify which HTML tags they want to disallow for security reasons. Given the current behavior of nette/schema, those HTML elements will always be disallowed (due to the always-merge-defaults behavior) even if users set that list to something less restrictive.

I'm hoping we can find some kind of solution that works for both of our projects. I don't necessarily want to maintain my own fork if I don't have to, but I also recognize that your project's goals come first here, so I'd completely understand (and wouldn't be offended) if a solution isn't implemented here :)

dg commented 4 years ago

I think that automerging behavior of array/list was a mistake and it would be best to choose solution #​1. (And release version 2).

I'd like to hear what @f3l1x or other users think about it.

mabar commented 4 years ago

Imho it's the best choice, merging of defaults is rarely needed. Personally I don't even remember single case when it was useful to do so.

f3l1x commented 4 years ago

Hi folks ✋

I agree with solution (1). It could be useful to enable default merging somehow, but only if it's not too hard.

We could introduce after callback and therefor default merging and much more could be done in after callback.