#2228141: Add authentication support to REST views fixed a bug in the REST module's Views support. But in doing so, the upgrade path lives in the REST module, yet the upgrade path test lives in the Views module.

We should move that test coverage to the REST module.

See #2228141-165: Add authentication support to REST views.

CommentFileSizeAuthor
#6 move_rest_update_8202-drupal-2756527-6.patch7 KBchgasparoto
#3 move_rest_update_8202-drupal-2756527-3.patch2.64 KBchgasparoto
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

I'll review this patch immediately someone comes up with a patch.

chgasparoto’s picture

chgasparoto CreditAttribution: chgasparoto as a volunteer and at CI&T commented
Status: Active » Needs review
FileSize
move_rest_update_8202-drupal-2756527-3.patch2.64 KB

I'm not sure if this is the only thing we have to do. Please review it.

Status: Needs review » Needs work

The last submitted patch, 3: move_rest_update_8202-drupal-2756527-3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Thank you for providing a patch!

+++ b/core/modules/rest/src/Tests/Update/RestExportAuthUpdateTest.php
@@ -0,0 +1,36 @@
+      __DIR__ . '/../../../tests/fixtures/update/rest-export-with-authentication.php',

+++ /dev/null
@@ -1,36 +0,0 @@
-      __DIR__ . '/../../../tests/fixtures/update/rest-export-with-authentication.php',

Note: You have to move this file as well.

chgasparoto’s picture

chgasparoto CreditAttribution: chgasparoto as a volunteer and at CI&T commented
Status: Needs work » Needs review
FileSize
move_rest_update_8202-drupal-2756527-6.patch7 KB

Oh thanks! Moved the file plus added a comma on lines 59 and 73.

Please review again.

Status: Needs review » Needs work

The last submitted patch, 6: move_rest_update_8202-drupal-2756527-6.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

That looks like a random failure. Queued for retesting.

chgasparoto’s picture

chgasparoto CreditAttribution: chgasparoto as a volunteer and at CI&T commented
Status: Needs work » Needs review

Test is green :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks!

+++ b/core/modules/rest/src/Tests/Update/RestExportAuthUpdateTest.php
@@ -0,0 +1,36 @@
+   * Ensures that update hook is run for views module.

Nit: s/views/rest/ — can be fixed on commit by committer.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Normal » Minor
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed d97b413 and pushed to 8.2.x. Thanks!

  • catch committed d97b413 on 8.2.x 
    Issue #2756527 by chgasparoto, Wim Leers: Move test coverage for...

Status: Fixed » Closed (fixed)

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