Resolve #3421993 "Primitivetypeconstraintvalidator vs configschemachecker"
Closes #3421993
Merge request reports
Activity
168 168 */ 169 169 public function set($property_name, $value, $notify = TRUE) { 170 170 $this->value[$property_name] = $value; 171 // Changing the value requires re-parsing. 172 unset($this->elements); 
git blamewould've told you, as would the commit history
28 39 /** 29 40 * {@inheritdoc} 30 41 */ 31 protected static $configSchemaCheckerExclusions = ['config_test.validation']; 42 protected static $configSchemaCheckerExclusions = [ 43 'config_test.validation', 44 // @see ::testPrimitiveDataTypesConsistency() 45 // @see ::testPrimitiveDataTypesConsistencyWhenTrustingData() 46 'config_test.types', -
Depending what you mean by "occasionally": yes.
This is being excluded to allow the test coverage that this issue is adding to explicitly test config schema checker results (to test whether a certain value triggers a config schema checker error or not), without
\Drupal\Core\Config\Development\ConfigSchemaChecker::onConfigSave()interfering with that.(That's also why
config_test.validationis in the exclusions — that's being tested in\Drupal\KernelTests\Config\TypedConfigTest::testTypedDataAPI(), but it tests with far less detail as what I'm adding in this MR.)
170 * 6. whether we expect a complaint from the ConfigSchemaChecker when saving 171 * as trusted data (and hence bypassing the magic-cast-during-save logic). 172 */ 173 public function providerBooleanDataType(): \Generator { 174 // @see core/modules/config/tests/config_test/config/install/config_test.types.yml 175 $key = 'boolean'; 176 177 // No magic: values 2–4 are identical; no validation errors (value 5). 178 yield "$key: NO MAGIC: `true`" => ['boolean', TRUE, TRUE, TRUE, []]; 179 yield "$key: NO MAGIC: `false`" => ['boolean', FALSE, FALSE, FALSE, []]; 180 // Every key can be marked optional: the validator should not complain. 181 yield "$key: NO MAGIC: `null`" => [$key, NULL, NULL, NULL, []]; 182 183 // Magic: values 2–4 are not identical. 184 // First: intentional magic — no validation errors triggered. 185 yield "$key: 🪄 `0` → `false`" => [$key, 0, 0, FALSE, [], 'variable type is integer but applied schema class is Drupal\Core\TypedData\Plugin\DataType\BooleanData']; -
The use of the magic wand emoji is adorable here, but I think it would be clearer to just say
MAGIC.Or better yet,
WEIRD MAGIC. To distinguish it from sensible magic.
Edited by Adam G-H -
I don't feel very strongly about this, but I don't like this particular emoji because of:
- It's indistinct - it's literally just a thin little stick. Particularly in dark mode, VERY easy to get lost. A block of capital letter won't have this problem.
- You have to know and understand what "magic" means in the context of config schema, and you have to know the cultural idioms that equate a magic wand with magic (or "weird, unexpected behavior"). That might not be true for every contributor who ever looks at this.
I think
As I said, I don't feel super strongly here. It's really not worth bikeshedding on it, so we can leave it as is. But we should reconsider the use of emoji in these kinds of situations for the future.
Edited by Adam G-H 
I brought the file up in my editor of choice to view the emoji in question. For me, I truly can not tell that it is supposed to be a magic wand. And some are followed by a colon and some are not. Does that too have some meaning.
I think @phenaproxima, is correct here to advocate for explicit keys that are clear to everyone in our diverse community.
I do think this should change.
140 156 $this->assertEquals($expected_message, $result->get(0)->getMessage()); 141 157 } 142 158 159 /** 160 * Provides test cases for testing the `type: boolean` primitive type. 161 * 162 * @return \Generator 163 * Each returned value is an array with at least 4 values, potentially 6, to 164 * pass into ::assertPrimitiveDataTypesConsistency(): 165 * 1. the config key whose value to set 166 * 2. the value to set 167 * 3. the value we expect to be validated 168 * 4. the value we expect to be saved 169 * 5. the expected validation errors -
ConfigSchemaCheckeris not "validation".That is not clear to the casual reader. You have to live and breathing config schema's internals in order to make that distinction. I think that, for people who aren't as deep in these particular weeds, "validation" and "checking" are synonymous.
So let's clarify it:
195 } 196 197 /** 198 * Provides test cases for testing the `type: integer` primitive type. 199 * 200 * @return \Generator 201 * Each returned value is an array with at least 4 values, potentially 6, to 202 * pass into ::assertPrimitiveDataTypesConsistency(): 203 * 1. the config key whose value to set 204 * 2. the value to set 205 * 3. the value we expect to be validated 206 * 4. the value we expect to be saved 207 * 5. the expected validation errors 208 * 6. whether we expect a complaint from the ConfigSchemaChecker when saving 209 * as trusted data (and hence bypassing the magic-cast-during-save logic). 210 */ 200 * @return \Generator 201 * Each returned value is an array with at least 4 values, potentially 6, to 202 * pass into ::assertPrimitiveDataTypesConsistency(): 203 * 1. the config key whose value to set 204 * 2. the value to set 205 * 3. the value we expect to be validated 206 * 4. the value we expect to be saved 207 * 5. the expected validation errors 208 * 6. whether we expect a complaint from the ConfigSchemaChecker when saving 209 * as trusted data (and hence bypassing the magic-cast-during-save logic). 210 */ 211 public function providerIntegerDataType(): \Generator { 212 // @see core/modules/config/tests/config_test/config/install/config_test.types.yml 213 $key = 'int'; 214 215 // No magic: values 2–4 are identical; no validation errors (value 5). -
I disagree. Without this sentence, it's really hard to figure out what all these arrays actually mean.
So, replaced "values" with "arguments", that should be clearer?
WDYT?
changed this line in version 2 of the diff
247 public function providerFloatDataType(): \Generator { 248 // @see core/modules/config/tests/config_test/config/install/config_test.types.yml 249 $key = 'float'; 250 251 // No magic: values 2–4 are identical; no validation errors (value 5). 252 yield "$key: NO MAGIC: `0.0`" => [$key, 0.0, 0.0, 0.0, []]; 253 yield "$key: NO MAGIC: `1.0`" => [$key, 1.0, 1.0, 1.0, []]; 254 yield "$key: NO MAGIC: `-1.0`" => [$key, -1.0, -1.0, -1.0, []]; 255 yield "$key: NO MAGIC: `3.14159`" => [$key, 3.14159, 3.14159, 3.14159, []]; 256 yield "$key: NO MAGIC: `-3.14159`" => [$key, 3.14159, 3.14159, 3.14159, []]; 257 // Every key can be marked optional: the validator should not complain. 258 yield "$key: NO MAGIC: `null`" => [$key, NULL, NULL, NULL, []]; 259 260 // Rational magic: integers can be represented as floats; values 2 and 3 are 261 // identical, value 4 is a float. 262 yield "$key: RATIONAL MAGIC: `0`" => [$key, 0, 0, 0.0, []]; -
"Rational Magic": thus did I find the name of the next techno track I write.
That said, maybe "sensible magic" is a clearer term to use here.
Edited by Adam G-H -
Went with "reasonable magical conversions". ("rational" was a bad name too because it has specific meaning when it comes to numbers: https://en.wikipedia.org/wiki/Rational_number)
See: 3f618dbc. WDYT?
changed this line in version 2 of the diff
added 1 commit
- 7749088a - Update `core/.deprecation-ignore.txt` to ensure a complete fix in...
added 2225 commits
-
7749088a...711e9ad4 - 2221 commits from branch
project:11.x - 90f06026 - Add detailed test coverage for the status quo.
- 871aa6e1 - Fix trivial static caching bug in `ArrayElement::set()` that the test coverage revealed.
- 81a89ae6 - @phenaproxima review: nits.
- 938f7588 - Update `core/.deprecation-ignore.txt` to ensure a complete fix in...
Toggle commit list-
7749088a...711e9ad4 - 2221 commits from branch
