Problem/Motivation

Port over drupalPostWithFormat from WTB

Proposed resolution

Remaining tasks

  • Review
  • Address feedback
  • Commit

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#3 2795041-3.patch3.41 KBdawehner

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Assigned: Unassigned » dawehner

Working on this particular subissue

dawehner’s picture

dawehner
Primary language German
commented
Assigned: dawehner » Unassigned
Status: Active » Needs review
StatusFileSize
new 2795041-3.patch3.41 KB

Here is one implementation using guzzle

dawehner’s picture

dawehner
Primary language German
commented
Parent issue: #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) » #2794285: [meta] Add backwards compatibility layer to browsertestbase to ease conversion from webtestcase
dawehner’s picture

dawehner
Primary language German
commented
Related issues: +#2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)
dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Title: BTB: Add drupalPostWithFormat » BrowserTestBase: Add drupalPostWithFormat

Why do we need this on BrowserTestBase? A test would only use this to test AJAX behavior, which we should convert to JavascriptTestBase instead?

  1. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
@@ -88,4 +90,8 @@ public function error() {
+  public function post(Request $request) {

    doc block is missing 

  2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
@@ -86,4 +86,9 @@ public function testError() {
+  public function testDrupalPostWithFormat() {

    doc block is missing 

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -649,6 +649,35 @@ protected function buildUrl($path, array $options = array()) {
+    $client = \Drupal::httpClient();

    let's not use \Drupal in classes when we don't have to.

    Does this even use the correct HTTP client of the curent test, like the Simpletest cookie and stuff?

dawehner’s picture

dawehner
Primary language German
commented

Thank you for your review!

Why do we need this on BrowserTestBase? A test would only use this to test AJAX behavior, which we should convert to JavascriptTestBase instead?

That's a good question. All calls in core are indeed related with with JS in some form or another: 

Method
    drupalPostWithFormat
Found usages  (13 usages found)
    Method call  (13 usages found)
        d8  (13 usages found)
            core/modules/block/src/Tests/Views  (1 usage found)
                DisplayBlockTest.php  (1 usage found)
                    DisplayBlockTest  (1 usage found)
                        testBlockContextualLinks  (1 usage found)
                            366$response = $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => 'test-page')));
            core/modules/config_translation/src/Tests  (1 usage found)
                ConfigTranslationUiTest.php  (1 usage found)
                    ConfigTranslationUiTest  (1 usage found)
                        renderContextualLinks  (1 usage found)
                            1101return $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => $current_path)));
            core/modules/contextual/src/Tests  (1 usage found)
                ContextualDynamicContextTest.php  (1 usage found)
                    ContextualDynamicContextTest  (1 usage found)
                        renderContextualLinks  (1 usage found)
                            185return $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => $current_path)));
            core/modules/editor/src/Tests  (1 usage found)
                EditorSecurityTest.php  (1 usage found)
                    EditorSecurityTest  (1 usage found)
                        testSwitchingSecurity  (1 usage found)
                            407$response = $this->drupalPostWithFormat('editor/filter_xss/' . $format, 'json', $post);
            core/modules/quickedit/src/Tests  (8 usages found)
                QuickEditAutocompleteTermTest.php  (1 usage found)
                    QuickEditAutocompleteTermTest  (1 usage found)
                        testAutocompleteQuickEdit  (1 usage found)
                            182$response = $this->drupalPostWithFormat('quickedit/entity/node/' . $this->node->id(), 'json', $post);
                QuickEditLoadingTest.php  (7 usages found)
                    QuickEditLoadingTest  (7 usages found)
                        testTitleBaseField  (2 usages found)
                            331$response = $this->drupalPostWithFormat('quickedit/metadata', 'json', $post);
                            388$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
                        testUserWithoutPermission  (2 usages found)
                            113$response = $this->drupalPostWithFormat(Url::fromRoute('quickedit.metadata'), 'json', $post);
                            140$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
                        testUserWithPermission  (3 usages found)
                            173$response = $this->drupalPostWithFormat('quickedit/metadata', 'json', $post);
                            245$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
                            299$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
            core/modules/views_ui/src/Tests  (1 usage found)
                DisplayTest.php  (1 usage found)
                    DisplayTest  (1 usage found)
                        testPageContextualLinks  (1 usage found)
                            208$response = $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => 'test-display')));

I just fear for contrib tests they could leverage that also for other ?_format=foo tests. Do we care about them?

dawehner’s picture

dawehner
Primary language German
commented

I'm happy to skip this method, but this might result in more porting work of existing tests.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

I think that is fine - testing javascript should really use JavascriptTestBase, so even if porting is harder I think it is totally worth it.

Should we close this in the meantime? We can reopen if we find out that we need this method for something other than javascript testing.

dawehner’s picture

dawehner
Primary language German
commented

I think that is fine - testing javascript should really use JavascriptTestBase, so even if porting is harder I think it is totally worth it.

Oh yeah I totally agree on that, and well, at least for core, this seems to be indeed the case.

I'm just wonder about drupalGetWithFormat as that one is certainly used way more often, like in \Drupal\rest\Tests\Views\StyleSerializerTest which is unrelated to JS.

Maybe we should put both together into a trait, and call it a legacy day? To be honest for those tests you want to use guzzle, as you are not using a browser, you care about the HTTP requests.

dawehner’s picture

dawehner
Primary language German
commented
Related issues: +#2820182: Add a LegacyBrowserTestBase which uses every legacy trait we have

See #2820182: Add a LegacyBrowserTestBase which uses every legacy trait we have

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

This is certainly not in review as of now :)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

dawehner
Primary language German
commented

Note: We certainly should throw a trigger_error here.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Anonymous (not verified) commented
Related issues: +#2912887: Document conversion equivalent for WebTestBase::drupalGetWithFormat()

#7, #10:

Why do we need this on BrowserTestBase?

We can reopen if we find out that we need this method for something other than javascript testing.

What about security tests? Why should we limit ourselves to the JS-API only? Running security tests in a JS environment can add an additional number of js/browser validators, which may not be on the backend. Accordingly, the tests will give a false sense of security.

I think BTB should support no less diverse API than WTB for the convenience of working with requests in different formats.

The choice of conversion WTB -> BTB or JTB is not related to this issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I think BTB should support no less diverse API than WTB for the convenience of working with requests in different formats.

Adding methods with no usages tends to be problematic also we want to maintain less test API not more.

dawehner’s picture

dawehner
Primary language German
commented

I agree. I think being more explicit than implicit is also often better. One less question of how things work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Closed (won't fix)

So since @dawehner and I are both testing maintainers going to close this issue. 1 less in the queue.