AjaxBasePageNegotiator should not require _theme: ajax_base_page to be specified

I agree, it doesn't really make sense.
Should this issue remove the other instances of it in core?

NIce, it returned green. I guess we still have tests for the general feature. I guess we need a change record for that?

However, the route author should not need to know in advance that the route would be used in a modal, it should Just Work™.

This is an excellent, excellent point.

I think this is really just a remnant from D7 and before. We now have the ability to render anything in a modal. Which means it'd be even wrong to set _theme: ajax_base_page in those cases!

To me that indicates that we have NO test coverage for this functionality.

I'm not surprised, our test coverage for AJAX/dialog/modal is poor. :( Because it stems from D7 and before.

What would some minimal test coverage look like here? Obviously the JS testing isn't in place enough I don't think to actually test the ajax?

I tried quick edit with multiple themes set on the front end and back end. I didn't notice any difference in functionality.

Did some manual testing , and well, as you see, the right theme is used.

Obviously the JS testing isn't in place enough I don't think to actually test the ajax?

This statement is obviously wrong ;)

Note: The interdiff and the fail patch are identical.

Missing @group. Note, the interdiff is still relative to #9

1. +++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
   @@ -6,6 +6,7 @@
   +use Drupal\Core\EventSubscriber\MainContentViewSubscriber;
   

   This is not used anywhere.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxThemeTest.php
   @@ -0,0 +1,45 @@
   + * Ensures that ajax themes are rendered in the frontend theme.
   

   Tests that AJAX responses use the current theme.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxThemeTest.php
   @@ -0,0 +1,45 @@
   +  ¶
   

   Whitespace nit.

Thank you wim!

Do you sometimes have these moments when you really question how it could have passed locally?

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxThemeTest.php
@@ -0,0 +1,55 @@
+    file_put_contents('/tmp/foo.png', $this->getSession()->getScreenshot());
+    file_put_contents('/tmp/foo.html', $this->getSession()->getPage()->getHtml());

This is leftover debug code?

Sure, totally.

Do you sometimes have these moments when you really question how it could have passed locally?

Too often!

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxThemeTest.php
@@ -27,19 +28,28 @@ public function testAjaxWithAdminRoute() {
+    $this->assertJsCondition($condition, 10000);

10 seconds?!

10 seconds?!

Yeah not sure about this value. I just copied it from \Drupal\Tests\views\FunctionalJavascript\ExposedFilterAJAXTest::waitForAjaxToFinish. Note: this is also just a timeout when the actual functionality doesn't work.

Sure, there we go.

Here is the interdiff

I'm RTBC for everything beside the test. If someone else gives a +1 for the test I'm super happy.

1. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -67,17 +65,15 @@ public function __construct(CsrfTokenGenerator $token_generator, ConfigFactoryIn
   -    if (($ajax_page_state = $this->requestStack->getCurrentRequest()->request->get('ajax_page_state'))  && !empty($ajax_page_state['theme']) && !empty($ajax_page_state['theme_token'])) {
   +    if ($ajax_page_state = $this->requestStack->getCurrentRequest()->request->get('ajax_page_state')) {
   

   Hm… we remove those two conditions because the applies() check already tests them for us.

   But that makes the code read strangely now: it looks as if we don't care if the 'theme' and 'theme_token' array keys exist.

   I think this can be much simplified: remove the condition altogether. And just do

   $ajax_page_state = …
   $theme = …
   $theme_token = …
   

   … because the applies() check already ensures this is only called if all those things exist.

   This may seem a nit, but IMO makes the code more maintainable long-term.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxThemeTest.php
   @@ -0,0 +1,53 @@
   +    // First visit the site directly via the URL. This should render it in the
   +    // admin theme.
   ...
   +    // Now
   

   Second comment is incomplete.

When those things are addressed, I agree with RTBC.

Thank you tim!

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxThemeTest.php
@@ -32,7 +32,7 @@ public function testAjaxWithAdminRoute() {
+    // Now click the modal, which should use also use the admin theme.

"use also use" :)

Can be fixed on commit.

RTBC++

Nice, even more nits fixed than I noticed :P

@tim.plunkett++
Yeah I better setup storm to yell at me.

The change looks good and I can't see any security issues that would be brought about by this change and it is great to have the additional test coverage. I think we should have a change record for this change and also search existing ones that might need an update + docs.

Fox example https://www.drupal.org/node/2122201 will need an update for 8.2.x

@tim.plunkett: CR looks great, but IMO you can already make the changes to https://www.drupal.org/node/2122201. We need those docs to work for both D8.0/8.1 and >=8.2. So we need to mention both. What I've been doing is <sup>Since 8.2.0</sup> for version-specific things. For example: https://www.drupal.org/node/2617470/revisions/view/9118520/9372512 for https://www.drupal.org/developing/api/8/ckeditor.

No other CRs talk about this, at least not according to https://www.drupal.org/list-changes/drupal/published?keywords_descriptio..., so #39 is addressed.

Committed c2b2864 and pushed to 8.2.x. Thanks!

This also fixed #2404199: quickedit.attachments ajax calls change the theme — yay!

This didn't really seem to fit in the release notes, but feel free to retag if I've missed some importance. A CR seems sufficient.