Discovered while working on #2758897, specifically #2758897-23: Move rest module's "link manager" services to serialization module. I looked at the existing upgrade path test coverage and built on top of that. Turns out it's wrong/broken. It only worked by accident.
Three flaws:
-
$connection->insert('key_value') ->fields([…]) ->fields([…]) ->fields([…]) ->execute();
Only the first of these is actually executed. I guess it's a huge DX flaw in that DB API too that it doesn't even complain about this… (Which had no consequence in those tests because the module whose upgrade path we were actually testing was always the first one. Hence we can safely delete everything except the first one.)
-
-$extensions['module']['basic_auth'] = 8000; -$extensions['module']['rest'] = 8000; -$extensions['module']['serialization'] = 8000; +$extensions['module']['basic_auth'] = 0; +$extensions['module']['rest'] = 0; +$extensions['module']['serialization'] = 0;
Those integers do not represent the current schema version, but the module weight. This was misinterpreted. (Which had no consequence in those tests because nothing that is being tested depends on module weight. But it provides a wrong example, so should be fixed.)
-
- 'link_domain' => '~', + 'link_domain' => NULL,
A
~
in YAML meansNULL
in PHP. This was a simple/small oversight. (Which had no consequence in those tests because nothing was testing thelink_domain
value.)
These mistakes were introduced in:
- #2308745: Remove rest.settings.yml, use rest_resource config entities
- #2228141: Add authentication support to REST views
- #2721595: Simplify REST configuration
- #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources
Comment | File | Size | Author |
---|---|---|---|
#4 | 2830333-2.patch | 3.82 KB | Wim Leers |
Comments
Comment #2
Wim LeersComment #4
Wim LeersForgot
--binary
. Identical patch otherwise.Comment #5
Wim LeersComment #6
tedbow@Wim Leers looks good went through patch and explination in the summary
1. Confirmed that \Drupal\Core\Database\Query\InsertTrait::fields only will have an effect the first time it is called on an object because in subsequent calls \Drupal\Core\Database\Query\InsertTrait::$insertFields will be set and nothing will happen the function.
2. Confirmed that core.extension row of config table does not store schema version, but the module weight
3. "~ in YAML means NULL in PHP" also that we aren't testing "link_domain"
Comment #7
alexpottCommitted and pushed c1325da to 8.3.x and f3b493f to 8.2.x. Thanks!