Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Active » Needs review
FileSize
7.53 KB

Try this...

disasm’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/tests/modules/ajax_test/ajax_test.routing.yml
    @@ -10,3 +10,27 @@ ajax_test_dialog_form:
    +ajax_test_dialog_close:
    +  pattern: 'ajax-test/dialog-close'
    +  defaults:
    +    _controller: '\Drupal\ajax_test\Controller\AjaxTestAjaxController::dialogClose'
    +  requirements:
    +    _access: 'TRUE'
    +ajax_test_dialog_render:
    +  pattern: 'ajax-test/render'
    +  defaults:
    +    _controller: '\Drupal\ajax_test\Controller\AjaxTestAjaxController::dialogRender'
    +  requirements:
    +    _access: 'TRUE'
    +ajax_test_dialog_order:
    +  pattern: 'ajax-test/order'
    +  defaults:
    +    _controller: '\Drupal\ajax_test\Controller\AjaxTestAjaxController::dialogOrder'
    +  requirements:
    +    _access: 'TRUE'
    +ajax_test_dialog_error:
    +  pattern: 'ajax-test/render-error'
    +  defaults:
    +    _controller: '\Drupal\ajax_test\Controller\AjaxTestAjaxController::dialogError'
    +  requirements:
    +    _access: 'TRUE'
    

    lets add a blank line between each route. Makes it more legible

  2. +++ b/core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestAjaxController.php
    @@ -0,0 +1,72 @@
    +    if (!empty($_GET['message'])) {
    +      $message = $_GET['message'];
    

    If you pass $request as an argument to this, these can be accessed via the request object.

mparker17’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
7.73 KB

Try this...

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.2066445-3.patch, failed testing.

mparker17’s picture

Whoops... that empty() language construct gets me every time.

Let's try this instead.

mparker17’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, drupal8.system-module.2066445-5.patch, failed testing.

mparker17’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal8.system-module.2066445-5.patch, failed testing.

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
11.71 KB

Re-rolling with current conversion changes...

The last submitted patch, 2066445-ajax_test-controller-11.patch, failed testing.

vijaycs85’s picture

Seems valid fails... seems test trying to see the js data loaded by drupal_add_js() but this has been changed. Not sure how to proceed further here... may need to update test case?

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs reroll

needs a reroll, instructions: https://drupal.org/contributor-tasks/reroll

rahulbile’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.15 KB

Re-rolling with current conversion changes.

Status: Needs review » Needs work

The last submitted patch, 15: 2066445-ajax_test-controller-15.patch, failed testing.

alvar0hurtad0’s picture

Rerolled patch

alvar0hurtad0’s picture

Status: Needs work » Needs review
FileSize
5.45 KB

Status: Needs review » Needs work

The last submitted patch, 18: 2066445-ajax_test_controller-14.patch, failed testing.

ianthomas_uk’s picture

#1971384: [META] Convert page callbacks to controllers has this as replacing ajax_test_dialog_close and other callbacks, but they aren't removed by the patch and there are no changes to \Drupal\ajax_test\Controller\AjaxTestController which I'd expect to see.

If this is the right issue to update that class, please can we also correct the docblock for ajax_test_dialog that was missed in #1987606: Convert ajax_test_dialog() to a new style controller

gokulnk’s picture

Assigned: Unassigned » gokulnk
tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
7.15 KB
xjm’s picture

Status: Needs review » Needs work

The last submitted patch, 22: ajax_test_controller-2066445-22.patch, failed testing.

xjm’s picture

Issue tags: +Needs reroll
mparker17’s picture

mparker17’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
mparker17’s picture

Assigned: gokulnk » Unassigned

Also, unassigned.

Status: Needs review » Needs work

The last submitted patch, 26: ajax_test_controller-2066445-26.patch, failed testing.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
7.43 KB

Rerolled with a fresh git pull.

Mile23’s picture

Issue tags: +Needs reroll
rpayanm’s picture

Issue tags: -Needs reroll
FileSize
5.92 KB
Mile23’s picture

Status: Needs review » Needs work

Thanks, @rpayanm.

Seems like we just need coding standards for method docblocks here. https://www.drupal.org/coding-standards/docs#functions

  1. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -19,32 +23,62 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_order().
    +   * Returns an AjaxResponse; settings command set last.
    +   * Helps verifying AjaxResponse reorders commands to ensure correct execution.
        */
    

    Needs @return in the docblock.

  2. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -19,32 +23,62 @@ class AjaxTestController {
        */
       public function dialogContents() {
         // Re-use the utility method that returns the example content.
    +    // This is a regular render array; the keys do not have special meaning.
         return ajax_test_dialog_contents();
       }
    

    Needs @return in the docblock.

  3. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -19,32 +23,62 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_render().
    +   *Returns an element suitable for use by
    +   * \Drupal\Core\Ajax\AjaxResponse::ajaxRender().
    +   * Additionally ensures that \Drupal\Core\Ajax\AjaxResponse::ajaxRender()
    +   * incorporates JavaScript settings generated during the page request by
    +   * invoking _drupal_add_js() with a dummy setting.
        */
    

    Needs @return in the docblock.

  4. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -19,32 +23,62 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_order().
    +   * Returns an AjaxResponse; settings command set last.
    +   * Helps verifying AjaxResponse reorders commands to ensure correct execution.
        */
       public function order() {
    

    Needs @return in the docblock.

  5. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -19,32 +23,62 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_error().
    +   * Menu callback: Returns AJAX element with #error property set.
        */
       public function renderError() {
    

    Needs @return in the docblock.

  6. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -19,32 +23,62 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_dialog().
    +   *
        */
       public function dialog() {
    

    Needs @return in the docblock.

  7. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -137,10 +171,12 @@ public function dialog() {
       /**
    -   * @todo Remove ajax_test_dialog_close().
    +   * Close the ajax dialog.
        */
       public function dialogClose() {
    

    Needs @return in the docblock.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
2.35 KB
6.49 KB

Please review.

Mile23’s picture

Thanks, @rpayanm.

Still needs some coding standards love: https://www.drupal.org/coding-standards/docs#functions

  1. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -16,35 +20,80 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_render().
    +   * Returns an element suitable for use by
    +   * \Drupal\Core\Ajax\AjaxResponse::ajaxRender().
    +   * Additionally ensures that \Drupal\Core\Ajax\AjaxResponse::ajaxRender()
    +   * incorporates JavaScript settings generated during the page request by
    +   * invoking _drupal_add_js() with a dummy setting.
    +   *
    +   * @return \Drupal\Core\Ajax\AjaxResponse $response
    +   *   The JSON response object.
        */
       public function render() {
    

    The format should be one line summary, then the big paragraph.

  2. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -16,35 +20,80 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_order().
    +   * Returns an AjaxResponse; settings command set last.
    +   * Helps verifying AjaxResponse reorders commands to ensure correct execution.
    +   *
    +   * @return \Drupal\Core\Ajax\AjaxResponse $response
    +   *   The JSON response object.
        */
       public function order() {
    

    Same here... One line and then supporting paragraph. In this case, just add an empty line after * Returns an AjaxResponse; settings command set last.

  3. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
    @@ -16,35 +20,80 @@ class AjaxTestController {
       /**
    -   * @todo Remove ajax_test_dialog().
    +   *  Returns the render array for the links.
    +   *
    +   * @return array
    +   *   The render array.
        */
       public function dialog() {
    

    The first line of the docblock should be indented by one space, not two.

Mile23’s picture

Status: Needs review » Needs work
adci_contributor’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
6.48 KB

Please review.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Addresses all the coding standards issues within scope... Great. :-)

Thanks, @adci_contributor and @rpayanm!

Mile23’s picture

Issue summary: View changes

Added beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Looks good one minor nit...

+++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
@@ -16,35 +20,81 @@ class AjaxTestController {
+    drupal_render($attached);

We should inject the render service into the AjaxTestController.

rpayanm’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
7.21 KB

Please review

Status: Needs review » Needs work

The last submitted patch, 42: 2066445-42.patch, failed testing.

Status: Needs work » Needs review

rpayanm queued 42: 2066445-42.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 42: 2066445-42.patch, failed testing.

rpayanm’s picture

Issue tags: +SprintWeekend2015Queue

What is wrong with my last patch, why failed?

mparker17’s picture

@rpayanm If you click on the [ View ] link under your patchfile, it will take you to the Drupal QA site where you can see the results of the automated testing: (currently the result is https://qa.drupal.org/pifr/test/946083, but next time the patch gets re-tested, that link will change).

rpayanm’s picture

I check the [ View ] link, but I don't know why the testing failed :(

valthebald’s picture

FileSize
49.92 KB

In Non-pass section:

Probably array is empty?

DevElCuy’s picture

DevElCuy’s picture

Issue tags: +SprintWeekend2015Queue

Removed tag by mistake.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
3.14 KB

Things got easier :)

Mile23’s picture

Status: Needs review » Needs work

It keeps getting smaller and smaller. :-)

Couple things...

+++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
@@ -60,10 +62,20 @@ public function order() {
+   * @return \Drupal\Core\Ajax\AjaxResponse $response
+   *   The JSON response object.

@return annotation shouldn't have a variable name. https://www.drupal.org/coding-standards/docs#return

Also just below that method, we see this:

  /**
   * @todo Remove ajax_test_dialog().
   */
  public function dialog() {

That could use a better docblock, don't you think? Fortunately there's an old crawl of the api docs here, for copypasting ease: https://drupalapi.alphanodes.com/api/drupal/core!core!modules!system!tes...

Note that this docblock is sort of in scope and sort of not in scope. But there you go.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Assuming the testbot passes, I say yay.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
@@ -60,14 +62,24 @@ public function order() {
   public function renderError() {
-    return ajax_test_error();
+    $message = '';
+    $query = \Drupal::request()->query;

I think you can get the request injected by just doing public function renderError(\Symfony\Component\HttpFoundation\Request $request)

aspilicious’s picture

Status: Needs work » Needs review
FileSize
3.3 KB

Let's see, needed a reroll anyway

tkuldeep17’s picture

FileSize
3.41 KB

hi aspilicious,

Just correcting coding standard in patch.

Status: Needs review » Needs work

The last submitted patch, 58: 2066445-58.patch, failed testing.

Status: Needs work » Needs review

aspilicious queued 58: 2066445-58.patch for re-testing.

Mile23’s picture

Mile23’s picture

Status: Needs review » Needs work
FileSize
969 bytes

Attached is the interdiff for that last patch.

ajax_test.module has been refactored away. AjaxTestController::renderError() now accepts injection of the request, in order to get its query and return the proper JsonResponse with an AlertMessage.

Other concerns have been refactored away elsewhere.

Looks good except for one thing....

+++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
@@ -80,14 +83,27 @@ public function order() {
+   * Menu callback: Returns AJAX element with #error property set.

This isn't actually true any more.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.4 KB

Fixed all doc-blocks, suppose patch ready

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Nice. All the docblock concerns are satisfied, and @andypost found more and fixed them.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Test code changes are not blocked by beta. Committed af1ea7b and pushed to 8.0.x. Thanks!

  • alexpott committed af1ea7b on 8.0.x
    Issue #2066445 by mparker17, rpayanm, aspilicious, tkuldeep17,...

Status: Fixed » Closed (fixed)

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