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.
Comment | File | Size | Author |
---|---|---|---|
#47 | core-ajax-settings-1941288-47.patch | 4.8 KB | nod_ |
#45 | core-ajax-settings-1941288-45.patch | 4.6 KB | nod_ |
#42 | drupal-1941288-42.patch | 4.83 KB | dawehner |
#42 | interdiff.txt | 698 bytes | dawehner |
#40 | ajax-1941288-40.patch | 4.3 KB | effulgentsia |
Comments
Comment #1
larowlanAnd the patch
Comment #2
Wim LeersSimple clean-up, passes tests and therefore works correctly.
Comment #3
quicksketchShould
Drupal::service
beDrupal::Service
?Comment #4
quicksketchEr, 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.
Comment #5
webchickYay, less code! :)
Committed and pushed to 8.x. Thanks!
Comment #6
webchickHuh. I couldn't begin to tell you why, but this patch causes the GUI installer to completely explode:
Reverted c047547318fad605af46e7d1a7d6a7b78cc0e8e6. Thanks to larsmw for the heads-up. :)
Comment #7
quicksketchHuh, 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.
Comment #8
larowlanAh thats why we had the extra 'has' for the request, will re-roll.
Comment #9
larowlanThis looks more failproof
Comment #10
larowlanSo the installer works for this latest patch, tested it manually.
@nod_ pointed out that this doesn't update the ajax page state correctly.
Comment #11
larowlanSo 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.
Comment #12
nod_So that's why it wasn't returning ajax anymore, interesting :)
Removed a stray error_log(). It works \o/
Comment #13
Wim LeersI 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…
Comment #14
larowlanGiven this fixes ajax_page_state
Comment #15
larowlanoh and new title
Comment #17
dawehnerThis 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.
Comment #19
Crell CreditAttribution: Crell commentedThis seems to be the only remaining bug blocking #1938980: Fix Ajax system; the last remnants of the old API must be swept away. *sigh*
Comment #20
larowlanRewound back to 11 and tried a different approach.
Interdiff is against that at 11.
This should be going in the right direction wrt fails.
Comment #22
dawehnerConceptually 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.
Comment #23
larowlanRetesting, seeing bot out of space issues
Comment #24
larowlan#20: drupal-get-js-ajax-1941288.20.patch queued for re-testing.
Comment #26
larowlanNice 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.
Comment #27
larowlanThis is blocking #1913618: Convert EntityFormControllerInterface to extend FormInterface, #1944472: Add generic content handler for returning dialogs and #1938980: Fix Ajax system; the last remnants of the old API must be swept away
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedLooks 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.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedThat'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?
Comment #30
larowlanI 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.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedIt 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.
Comment #33
dcam CreditAttribution: dcam commentedhttp://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.
Comment #34
Wim LeersDuplicate was opened at #2019097: Edit module performing an AJAX call for an attachment and an editor for each entity on the page resulting in redundant fetching. We really need to fix this.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #37
dawehnerThere are several problems involved here:
Comment #38
nod_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.
Comment #39
larowlanYeah not sure on the hardcoded list, but not sure I can think of a better approach.
+1 RTBC
Comment #40
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #41
catchCan live with the hardcoded list to fix the critical, should open a major follow-up to sort that out though.
No docs for the new parameter.
Comment #42
dawehnerHa, I remember exactly when I wrote the code and thought: this for sure will not work.
Comment #43
nod_Comment #44
effulgentsia CreditAttribution: effulgentsia commentedReally 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?
Comment #45
nod_comment probably need work, but it's much much cleaner, thanks!
Comment #46
dawehnerI really like this easier approach!
Should be @param bool $is_ajax
Comment #47
nod_added bool to $is_ajax and $skip_alter too.
Comment #48
dawehnerThanks.
Comment #49
catchMuch nicer.
Committed/pushed to 8.x.