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:

  1. $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.)

  2. -$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.)

  3. -  'link_domain' => '~',
    +  'link_domain' => NULL,
    

    A ~ in YAML means NULL in PHP. This was a simple/small oversight. (Which had no consequence in those tests because nothing was testing the link_domain value.)

These mistakes were introduced in:

  1. #2308745: Remove rest.settings.yml, use rest_resource config entities
  2. #2228141: Add authentication support to REST views
  3. #2721595: Simplify REST configuration
  4. #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Version: 8.3.x-dev » 8.2.x-dev
Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
3.81 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2830333-2.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.82 KB

Forgot --binary. Identical patch otherwise.

Wim Leers’s picture

Title: All REST upgrade path test coverage fixtures contain no-op code and other mistakes » All REST update path test coverage fixtures contain no-op code and other mistakes
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@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"

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed c1325da to 8.3.x and f3b493f to 8.2.x. Thanks!

  • alexpott committed c1325da on 8.3.x
    Issue #2830333 by Wim Leers, tedbow: All REST update path test coverage...

  • alexpott committed f3b493f on 8.2.x
    Issue #2830333 by Wim Leers, tedbow: All REST update path test coverage...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.