Date FAPI element type do not allow AJAX via #ajax API

Tested against 8.3.x and couldn't get the date selection message to show up via AJAX.

Not sure what happened. Reinstalled 8.3.x, reinstalled the module and applied the patch and now it's working well.

To get this to 8.2.x I think you'll need to get a beta target from a release manager.

Note: This could still land in 8.2.1 for example

Its sooooo great to see javascript tests written by people!

1. +++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
   --- /dev/null
   +++ b/core/modules/system/src/Tests/Ajax/AjaxCallbackTest.php
   
   +++ b/core/modules/system/src/Tests/Ajax/AjaxCallbackTest.php
   @@ -0,0 +1,34 @@
   +/**
   + * Tests an AJAX form elements.
   + *
   + * @group Ajax
   + */
   +class AjaxCallbackTest extends JavascriptTestBase {
   +
   

   The tests should live in core/tests/Drupal/FunctionalJavascriptTests/Ajax to be picked up by the testbot

2. +++ b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestAjaxElementsForm.php
   @@ -0,0 +1,48 @@
   +    $object = new Callbacks();
   ...
   +        'callback' => array($object, 'dateCallback'),
   

   Small suggestion: use a maybe slightly better variable name like $callback_object or so

To be fair it is a bit of a blocker, when we write tests and they aren't actually executed :)

The tests should live in core/tests/Drupal/FunctionalJavascriptTests/Ajax to be picked up by the testbot

I was looking at this and thinking the same thing, but oddly enough it is showing up in the logs as being run:

Nice, more javascript tests!

Bit of nitpicking:

1. +++ b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestAjaxElementsForm.php
   @@ -0,0 +1,48 @@
   +    // of #2781103: Date FAPI element type do not allow AJAX via #ajax API
   

   I think we use full URLs for this, not just issue numbers.

2. +++ b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestAjaxElementsForm.php
   @@ -0,0 +1,48 @@
   +      '#default_value' => date('Y-m-d'),
   

   Wouldn't it make more sense to not have a date by default? Since that is what the message says.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbackTest.php
   @@ -0,0 +1,34 @@
   +use Drupal\simpletest\ContentTypeCreationTrait;
   +use Drupal\simpletest\NodeCreationTrait;
   

   Unused Use statements

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbackTest.php
   @@ -0,0 +1,34 @@
   + * Tests an AJAX form elements.
   

   Tests AJAX form elements or Tests an AJAX form element, one of the two

Did a quick look at this; don't have time to manually test this.

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbackTest.php
   @@ -0,0 +1,36 @@
   +  public function testAjaxCallbacks() {
   

   For the long term, I think calling this testDateCallbacks() would be better in the long term so that additional elements can have their own test methods in that class (instead of lumping them all here). Also would adjust the docblock on the method accordingly.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbackTest.php
   @@ -0,0 +1,36 @@
   +    // @todo: add all FAPI elements to test all AJAX callbacks as a follow-up
   +    // of https://www.drupal.org/node/2781103: Date FAPI elements do not allow
   +    // AJAX via #ajax API.
   +    // @see AjaxFormsTestAjaxElementsForm
   +  }
   

   Nobody is around on IRC right now to verify, I don't think we can commit a patch with a @todo w/o a reference to the followup issue that takes care of the todo. Also elsewhere in the patch.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbackTest.php
   @@ -0,0 +1,36 @@
   +    $this->drupalGet('ajax_forms_test_ajax_element_form');
   +    $this->assertSession()->responseContains('No date yet selected');
   +    $this->getSession()->getPage()->fillField('edit-date', date('Y-m-d'));
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   +    $this->assertSession()->responseNotContains('No date yet selected');
   

   Two things here. Calling date() is unnecessary; a hardcoded value should be used to make this predictable. Also, negative assertions should be followed by positive ones. So, that responseNotContains() should be followed by asserting that the response contains something expected.

   Setting NW mainly for this bit.

Note that you can add tests against a particular branch using the "Test with" dropdown when you upload a file or the "Add test" link under the patch. Thanks!

Note: this seems to work for field type 'date', but not yet for type 'datetime'.

Patch in #36 worked for me. +1 for RTBC.

#36 doesn't apply, so it needs to be rerolled.

1. +++ b/core/modules/system/tests/modules/ajax_forms_test/src/Form/AjaxFormsTestAjaxElementsForm.php
   @@ -0,0 +1,47 @@
   +      '#ajax' => array(
   +        'callback' => array($callback_object, 'dateCallback'),
   +      ),
   

   Nits, needs to be updated to [] syntax.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxCallbacksTest.php
   @@ -0,0 +1,35 @@
   +    // @todo: add tests for all FAPI elements Ajax callbacks.
   +    // @see: AjaxFormsTestAjaxElementsForm
   

   We can remove these; we try to avoid @todo w/o f/up issues to point to.

I think this looks OK, but don't think I know enough about the Ajax subsystem to RTBC this once rerolled.

Reroll and fixed 43.1 and 43.2.

Please remember to remove Needs Reroll tag.

Works for me!

Also noticed it does not work with datetime, but this should be adressed in https://www.drupal.org/node/2783615. I briefly tried to modify the patch to also work with datetime, but that seems to require more work.

The changes to the date logic already are commited to 8.5.x with https://www.drupal.org/node/2783615 now.

All thats left to do here is to add the test for the single date element.

I cannot do an interdiff, because the old patch does not apply anymore...

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!