Over in #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js we added some calls to drupal_container() inside drupal_get_js() to determine if we were dealing with an ajax request, with a @todo to clean it up.

// @todo Clean up container call.
    $container = drupal_container();
    if ($container->has('content_negotiation') && $container->isScopeActive('request')) {
      $type = $container->get('content_negotiation')->getContentType($container->get('request'));
    }

Yesterday over in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases we realised that the Request service already provides something similar via:

Drupal::service('request')->isXmlHttpRequest()

This patch simplifies the container call to use that Request method.
Patch to follow once I have nid.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Status: Active » Needs review
Issue tags: +JavaScript
FileSize
1.01 KB

And the patch

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Simple clean-up, passes tests and therefore works correctly.

quicksketch’s picture

Should Drupal::service be Drupal::Service?

quicksketch’s picture

Er, to answer my own question. Apparently no. </camelCaseNewb>

+1 RTBC. Using the same mechanism now in #1870764-86: Add an ajax command which makes it easy to use the dialog API in complex cases.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, less code! :)

Committed and pushed to 8.x. Thanks!

webchick’s picture

Status: Fixed » Needs work

Huh. I couldn't begin to tell you why, but this patch causes the GUI installer to completely explode:

Additional uncaught exception thrown while handling exception.

Original

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition &quot;request&quot; does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 799 of /Users/webchick/Sites/8.x/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Additional

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition &quot;request&quot; does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 799 of /Users/webchick/Sites/8.x/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Reverted c047547318fad605af46e7d1a7d6a7b78cc0e8e6. Thanks to larsmw for the heads-up. :)

quicksketch’s picture

Huh, strange. Sorry about the errant RTBC. I applied this and tried it out myself but didn't try a reinstall. I wouldn't have thought it would have any effect.

larowlan’s picture

Ah thats why we had the extra 'has' for the request, will re-roll.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
830 bytes

This looks more failproof

larowlan’s picture

So the installer works for this latest patch, tested it manually.
@nod_ pointed out that this doesn't update the ajax page state correctly.

larowlan’s picture

So turns out that the re-write of the Ajax commands in #1812866: Rebuild the server side AJAX API broke the ajax page state, because the Request object no longer returns that its Ajax, because its a sub-request. But bonus to this is we can inject a phony settings inside AjaxResponse::ajaxRender and drupal_get_js always returns the settings, which has the same effect.
We can safely do this because any page that fires an ajax response already has settings/js enabled - ie there is no front-end performance issue - and the intent of #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js remains intact.

nod_’s picture

So that's why it wasn't returning ajax anymore, interesting :)

Removed a stray error_log(). It works \o/

Wim Leers’s picture

I also apologize for the inappropriate RTBC. Like quicksketch, I didn't anticipate this breaking the installer.

Can we prevent regressions here by adding tests? (It worked for testbot, but not IRL…)

Also: *another* regression for the new AJAX command API! I already fixed two earlier regressions…

larowlan’s picture

Category: task » bug
Priority: Normal » Major

Given this fixes ajax_page_state

larowlan’s picture

Title: Cleanup container call in drupal_get_js() » ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice

oh and new title

Status: Needs review » Needs work

The last submitted patch, core-get-js-ajax-1941288-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.04 KB
10.89 KB

This seems to fix the issues.

I'm not really sure whether the changes in DialogTest are the best way to fix them, though at least they seem to more resistent
again future changes.

Status: Needs review » Needs work

The last submitted patch, drupal-1941288-17.patch, failed testing.

Crell’s picture

This seems to be the only remaining bug blocking #1938980: Fix Ajax system; the last remnants of the old API must be swept away. *sigh*

larowlan’s picture

Status: Needs work » Needs review
FileSize
6.23 KB
5.86 KB

Rewound back to 11 and tried a different approach.
Interdiff is against that at 11.
This should be going in the right direction wrt fails.

Status: Needs review » Needs work

The last submitted patch, drupal-get-js-ajax-1941288.20.patch, failed testing.

dawehner’s picture

Conceptually it feels great to move the ajax related handling out of drupal_get_js() into the AjaxResponse.

For sanity we could move this new logic into a method, so it's easier to scan.

larowlan’s picture

Retesting, seeing bot out of space issues

larowlan’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript

Status: Needs review » Needs work
Issue tags: +JavaScript

The last submitted patch, drupal-get-js-ajax-1941288.20.patch, failed testing.

larowlan’s picture

Nice down to 2 fails now - I believe these are in the mock implementation of an AJAX post in WebTestBase::drupalPostAJAX as when testing manually (by enabling ajax_forms_test) the page state is correctly managed. However this isn't the case in the tests. I spent about 3hrs on it last night to no avail.
Will try to summon strength to look at it again tonight, but I'm seriously over it, and given the manual tests work - I can't set a breakpoint and debug the issue.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
7.51 KB

Looks like HEAD has ContentNegotiation::getContentType() determining an 'ajax' format based on one header, and AjaxSubscriber::onKernelRequest() determining a 'drupal_ajax' format based on a different header, both of which get sent when testing via a real browser. The latter already has a @todo to unify these conflicting formats. In the meantime, that's not this issue's scope, so this just updates WebTestBase to also set both headers.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -133,6 +134,36 @@ protected function ajaxRender(Request $request) {
+      // Setup ajaxPageState.
+      global $theme_key;
+      // Provide the page with information about the theme that's used, so that
+      // a later AJAX request can be rendered using the same theme.
+      // @see ajax_base_page_theme()
+      $settings['ajaxPageState']['theme'] = $theme_key;
+      // Checks that the DB is available before filling theme_token.
+      if (!defined('MAINTENANCE_MODE')) {
+        $settings['ajaxPageState']['theme_token'] = drupal_get_token($theme_key);
+      }
+
+      // Provide the page with information about the individual JavaScript files
+      // used, information not otherwise available when aggregation is enabled.
+      $settings['ajaxPageState']['js'] = array_fill_keys(array_keys($scripts), 1);
+      unset($settings['ajaxPageState']['js']['settings']);
+
+      // Provide the page with information about the individual CSS files used,
+      // information not otherwise available when CSS aggregation is enabled.
+      // The setting is attached later in this function, but is set here, so
+      // that CSS files removed in drupal_process_attached() are still
+      // considered "used" and prevented from being added in a later AJAX
+      // request.
+      // Skip if no files were added to the page otherwise jQuery.extend() will
+      // overwrite the Drupal.settings.ajaxPageState.css object with an empty
+      // array.
+      $css = drupal_add_css();
+      if (!empty($css)) {
+        // Cast the array to an object to be on the safe side even if not empty.
+        $settings['ajaxPageState']['css'] = (object) array_fill_keys(array_keys($css), 1);

That's a lot of code being duplicated from drupal_get_js(). Is the plan to remove that duplication in this issue or in a follow up?

larowlan’s picture

I didn't get around to figuring out if ajaxPageState is set (in Drupal.settings) for a normal (non ajax) request or not.
If it isn't - it can go from drupal_get_js().
If it is, we need to remove the duplication.
Regarding headers, thats the same conclusion I came to in #1938980-28: Fix Ajax system; the last remnants of the old API must be swept away but your approach is more robust than mine.
Thanks heaps for getting this rolling again.

effulgentsia’s picture

I didn't get around to figuring out if ajaxPageState is set (in Drupal.settings) for a normal (non ajax) request or not.

It is, because the base page (text/html request) needs to know its theme and assets, so that later when it makes an ajax request, that ajax request can use that information.

Status: Needs review » Needs work

The last submitted patch, drupal-get-js-ajax-1941288.28.patch, failed testing.

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

Wim Leers’s picture

effulgentsia’s picture

Title: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice » Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice
Priority: Major » Critical
Status: Needs work » Needs review
FileSize
1.97 KB

This is a D7 to D8 regression, due to the refactoring of AJAX, therefore, raising to critical.

It's something we had a test for in D7, but then had a period of time in which Simpletest was executing a different AJAX flow than browser requests, and therefore, the bug got introduced undetected by bot. We then had to comment out the test in order to unblock getting Simpletest back into the correct flow. So, here's a patch with the assertions restored. It's expected to fail. The goal of this issue is to make it pass. #20 had code in that direction, but I'm not sure how much of it is still relevant, so leaving to larowlan or others to submit an updated patch that incorporates the desired parts.

Status: Needs review » Needs work

The last submitted patch, ajax-assets-1941288-35.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.34 KB

There are several problems involved here:

  • drupal_get_js in called from AjaxResponse::prepare() which is not in the container request scope, so $container->get('request') is not existant
  • On the other hand, the prepare method has the request (don't ask my what the idea is behind that), so let's pass them along to drupal_get_js
  • On there, just use the request if passed in.
  • The content negotiator returns either drupal_ajax, drupal_modal or drupal_dialog, but not 'ajax' as drupal_get_js currently assumes.
nod_’s picture

Status: Needs review » Reviewed & tested by the community

Ok so it's not super-pretty code but it does work. Tested manually on frontpage and when using modals it also passes #36 so that's cool.

One issue with this is that the list of acceptable negociation header is hardcoded but well, that works. I personally have no idea how this piece of code will evolve but now we have tests for this thanks to effulgentsia.

So in my humble opinion, RTBC because the critical is indeed fixed by #37.

larowlan’s picture

Yeah not sure on the hardcoded list, but not sure I can think of a better approach.
+1 RTBC

effulgentsia’s picture

FileSize
4.3 KB

This just combines #37 with the tests from #35. Leaving at RTBC, so long as bot approves. Replacing that hard-coded list could be a noncritical follow up.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Can live with the hardcoded list to fix the critical, should open a major follow-up to sort that out though.

+++ b/core/includes/common.incundefined
@@ -2312,7 +2312,7 @@ function drupal_js_defaults($data = NULL) {
  * @see drupal_js_defaults()
  */
-function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALSE) {
+function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALSE, Request $request = NULL) {
   if (!isset($javascript)) {

No docs for the new parameter.

dawehner’s picture

Status: Needs work » Needs review
FileSize
698 bytes
4.83 KB

Ha, I remember exactly when I wrote the code and thought: this for sure will not work.

nod_’s picture

Status: Needs review » Reviewed & tested by the community
effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/common.inc
@@ -2312,7 +2315,7 @@ function drupal_js_defaults($data = NULL) {
-function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALSE) {
+function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALSE, Request $request = NULL) {
...
+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -111,8 +111,8 @@ protected function ajaxRender(Request $request) {
-    $scripts_footer = drupal_get_js('footer', $items['js'], TRUE);
-    $scripts_header = drupal_get_js('header', $items['js'], TRUE);
+    $scripts_footer = drupal_get_js('footer', $items['js'], TRUE, $request);
+    $scripts_header = drupal_get_js('header', $items['js'], TRUE, $request);

Really sorry for kicking this back from RTBC, but if we make this change to drupal_get_js() now, then removing the $request param later will be an API change. Can't we simplify this by renaming that parameter to $is_ajax (or a better name if anyone has ideas), and have ajaxRender() pass in TRUE? Wouldn't that also solve the hardcoded format list problem?

nod_’s picture

comment probably need work, but it's much much cleaner, thanks!

dawehner’s picture

I really like this easier approach!

@@ -2304,6 +2304,9 @@ function drupal_js_defaults($data = NULL) {
+ * @param $is_ajax

Should be @param bool $is_ajax

nod_’s picture

added bool to $is_ajax and $skip_alter too.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Much nicer.

Committed/pushed to 8.x.

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