Comments

mparker17’s picture

Assigned:mparker17» Unassigned
Status:Active» Needs review
StatusFileSize
new7.53 KB
PASSED: [[SimpleTest]]: [MySQL] 57,884 pass(es).
[ View ]

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
StatusFileSize
new2.68 KB
new7.73 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/ajax_test/lib/Drupal/ajax_test/Controller/AjaxTestAjaxController.php.
[ View ]

Try this...

Status:Needs review» Needs work

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

mparker17’s picture

StatusFileSize
new932 bytes
new7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,136 pass(es), 8 fail(s), and 12 exception(s).
[ View ]

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

#5: drupal8.system-module.2066445-5.patch queued for re-testing.

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
StatusFileSize
new11.71 KB
FAILED: [[SimpleTest]]: [MySQL] 59,746 pass(es), 8 fail(s), and 1 exception(s).
[ View ]

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
StatusFileSize
new12.15 KB
FAILED: [[SimpleTest]]: [MySQL] 63,039 pass(es), 8 fail(s), and 1 exception(s).
[ View ]

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
StatusFileSize
new5.45 KB
FAILED: [[SimpleTest]]: [MySQL] 63,042 pass(es), 21 fail(s), and 64 exception(s).
[ View ]

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
StatusFileSize
new7.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ajax_test_controller-2066445-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
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

StatusFileSize
new7.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch ajax_test_controller-2066445-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Straight re-roll so no interdiff.

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
StatusFileSize
new7.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,142 pass(es).
[ View ]

Rerolled with a fresh git pull.

Mile23’s picture

Issue tags:+Needs reroll
rpayanm’s picture

Issue tags:-Needs reroll
StatusFileSize
new5.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,574 pass(es).
[ View ]
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
StatusFileSize
new2.35 KB
new6.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,845 pass(es).
[ View ]

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
StatusFileSize
new1.27 KB
new6.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,833 pass(es).
[ View ]

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
StatusFileSize
new1.75 KB
new7.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,883 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

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

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

StatusFileSize
new49.92 KB

In Non-pass section:

Probably array is empty?

develCuy’s picture

develCuy’s picture

Removed tag by mistake.

aspilicious’s picture

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,275 pass(es).
[ View ]

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
StatusFileSize
new3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,868 pass(es).
[ View ]
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
StatusFileSize
new3.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,932 pass(es).
[ View ]

Let's see, needed a reroll anyway

tkuldeep17’s picture

StatusFileSize
new3.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,942 pass(es).
[ View ]

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
StatusFileSize
new969 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
StatusFileSize
new3.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,219 pass(es).
[ View ]

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.