Turn the SchemaException in mysql\Schema::getNormalizedIndexes() into an assertion

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -316,6 +316,7 @@ protected function getNormalizedIndexes(array $spec) {
         // Check whether the field is defined in the table specification.
+        assert('isset($spec['fields'][$field_name])', "MySQL needs the '$field_name' field specification in order to normalize the '$index_name' index");
         if (isset($spec['fields'][$field_name])) {

The if statement becomes unnecessary here.

The conceit with assertions is that the error does not get as far as production, so production doesn't need additional error checking in production for that situation.

(If we're not comfortable with that caveat, then we shouldn't switch it to an assertion.)

+++ b/core/modules/system/src/Tests/Database/SchemaTest.php
@@ -298,18 +298,6 @@ function testIndexLength() {
-    // Ensure expected exception thrown when adding index with missing info.
-    $expected_exception_message = "MySQL needs the 'test_field_text' field specification in order to normalize the 'test_regular' index";
-    $missing_field_spec = $table_specification;
-    unset($missing_field_spec['fields']['test_field_text']);
-    try {
-      $schema_object->addIndex('test_table_index_length', 'test_separate', [['test_field_text', 200]], $missing_field_spec);
-      $this->fail('SchemaException not thrown when adding index with missing information.');
-    }
-    catch (SchemaException $e) {
-      $this->assertEqual($expected_exception_message, $e->getMessage());
-    }

We ought to convert this to test for the assertion rather than removing it entirely. Unfortunately, that may be blocked on #2536560: Runtime Assertion unit and functional testing. Otherwise I'm good here.

+++ b/core/modules/system/src/Tests/Database/SchemaTest.php
@@ -298,18 +298,6 @@ function testIndexLength() {
-    // Ensure expected exception thrown when adding index with missing info.
-    $expected_exception_message = "MySQL needs the 'test_field_text' field specification in order to normalize the 'test_regular' index";
-    $missing_field_spec = $table_specification;
-    unset($missing_field_spec['fields']['test_field_text']);
-    try {
-      $schema_object->addIndex('test_table_index_length', 'test_separate', [['test_field_text', 200]], $missing_field_spec);
-      $this->fail('SchemaException not thrown when adding index with missing information.');
-    }
-    catch (SchemaException $e) {
-      $this->assertEqual($expected_exception_message, $e->getMessage());
-    }

Yeah we should keep the test, this is for sure

I'm not sure it is OK to silently "pass" returning the unmodified index here if assertions are turned off. The system might pass, but if it fails it is going to throw an exception from inside PDO that is going to be a lot less clear.

Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes: ALTER TABLE .. ADD INDEX ...; Array ( ) in db_add_index() (line 659 of /.../core/includes/database.inc).

This would be valid as an assertion only if the data has already been validated elsewhere. From @neclimdul#10 it is clear that it is not true.

So either we keep the exception as it is, or we need to add actual validation in the caller.

The rule of a thumb is that assertions are for invariants that should be true if there is no programming error in the same unit. Programming errors in other units (passing invalid arguments, calling a method whose pre-requisites are not met, etc.) are just standard error handling as part of the unit contract, and should use normal exceptions.

I've never heard the "in the same unit" caveat when describing assertion use cases. I generally only here "if it can only possibly happen because of programmer error, assertion. If it's user/environment dependent, exception". Which programmer generally doesn't come up. :-)

@Crell: Assertions document logically true conditionals, or said differently, they document conditions that should be absolutely impossible short of a programming error. In most cases, you can only define "logically impossible" conditions inside a single unit, as an unit doesn't control how its exported methods are used.

Or to enforce invariants that go beyond what the type system allows. Eg, the legal values of this parameter are the strings "header" and "footer"; if it's not one of those, it may as well be a type error. Hence assertion.

> Or to enforce invariants that go beyond what the type system allows. Eg, the legal values of this parameter are the strings "header" and "footer"; if it's not one of those, it may as well be a type error. Hence assertion.

@Crell: Described like this, in the vast majority of cases those should use normal exceptions (UnexpectedValueException, BadMethodCallException, etc.). Note that TypeError is *not* an assertion. It is not a subclass of AssertionError, nor should it.

Seems like there is still some discussion going on. Can the IS be updated with the final decision? Also this will need a reroll for 10.1