Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
#1812866: Rebuild the server side AJAX API introduced a new, cleaner server-side API for Ajax commands. FAPI needs to be updated accordingly so that we can remove the old code.
Change notice with instructions is here: http://drupal.org/node/1843212
Comment | File | Size | Author |
---|---|---|---|
#76 | drupal-1843220-fix-76.patch | 2.39 KB | mkadin |
#68 | interdiff.txt | 2.47 KB | mkadin |
#67 | drupal-1843220-67.patch | 8.35 KB | mkadin |
#64 | drupal-1843220-64.patch | 7.72 KB | mkadin |
#62 | drupal-1843220-62.patch | 7.57 KB | mkadin |
Comments
Comment #1
mkadin CreditAttribution: mkadin commentedThis should mostly be really straight forward. The only thing that might take some thnking / real work is the function ajax_prepare_response() which is only called in ViewSubscriber.php.
Is this ViewSubscriber.php thing still going to be around when D8 is all done? Or is every page callback going to return a response object?
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedTagging for Crell to weigh in on that.
My current thinking is that most
_content
controllers (what most page callbacks will become) should return something that can be inserted into either an HTML page or an AJAX response. I don't know whether it matters architecturally or for DX whether that is a render array or a Response object. If it's a Response object, then ViewSubscriber will go away, but some other piece of code will need to get the content out of the Response and call ajax_prepare_response() (or whatever ajax_prepare_response turns into as part of this issue) on it.Comment #3
Crell CreditAttribution: Crell commentedViewSubscriber in its current form I hope SCOTCH kills. We likely will still want a view subscriber of some sort, but if rejiggering that makes sense, absolutely go for it.
If a controller returns a response object, then the kernel.view event never fires in the first place.
Comment #4
mkadin CreditAttribution: mkadin commentedThe way Drupal's AJAX stuff works now, is that an AJAX callback could return any number of different things (an integer HTTP response code, a render array, a set of AJAX commands, or a string of HTML). This is pretty convenient for simplifying the AJAX callbacks themselves. The Book module has a good example:
The book_form_update() function just returns a little render array. The alternative would be to force developers to put a bit of extra time in:
I think the former is a better DX for sure...so if we want to stick with it, ViewSubscriber::onAjax() needs to stay or we need some other cool symfony way (of which I know very little) of dealing with HTML or render arrays that get returned.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedThere's two slightly different things here I'd like to clarify. One is Ajax responses in general. The other is the role of the #ajax['callback'] function.
For the first one, a really important concept to understand is that *any* Drupal 'page callback' result can be delivered as an Ajax response. With #1667742: Add abstracted dialog to core (resolves accessibility bug) recently committed, this is actually really easy to see. Add
'#ajax' => array('dialog' => array('modal' => TRUE))
to any element of'#type' => 'link'
, and poof, no matter what the 'href' of that link, the result will be sent back as an Ajax response.In the old routing system, this is accomplished by the 'page callback' function returning a render array, and ViewSubscriber::onAjax() packaging that into a Response.
In the new routing system, most 'page callback' functions will be replaced by '_content' controllers. An example of one is for /user/register, defined in user.routing.yml. UserRouteController::register() currently returns a render array, but according to #3, Crell would like that changed to it returning a Response object. I don't know if we've actually agreed on that anywhere, so we might need a spin off issue to discuss it. I don't have a strong preference yet one way or the other: I'd like to see wider feedback on what people consider better DX.
However, even if we do make it return a Response object, this Response object won't be an AjaxResponse. It'll be a regular Response, because UserRouteController::register() shouldn't need to know whether someone is viewing the registration form as part of a full HTML page, or in a modal dialog. So this Response object isn't what's returned to the browser. Instead, RouteProcessorSubscriber::onRequestSetController() is responsible for setting the _controller that needs to wrap the _content. Currently, it always sets that controller to HtmlPageController, but that's wrong. It needs to content negotiate like ViewSubscriber::onView() does. If the request is an Ajax request, it needs to set the controller to AjaxController. AjaxController can be modeled on HtmlPageController: i.e., invoke _content as a subrequest, take the content out of the returned Response, and package it into a new Response. This new Response can be an AjaxResponse.
Ok, on to #ajax['callback']. Essentially the 'system/ajax' path needs to be converted to the new routing system, and ajax_form_callback() needs to be made into either a
_content
controller or into a_controller
controller. Logically, I think it makes more sense as a_controller
. But it would need to have a lot of similar code as AjaxController. Maybe it could even be another method on AjaxController or be a subclass like AjaxFormController? Its functionality is to process the form, call the triggering element's #ajax['callback'], and then turn the result of that into an AjaxResponse. I think we still have room to decide on what we therefore want the interface of #ajax['callback'] itself to be. Unless there's a compelling reason to change it from what it currently is (a function that (usually) returns a render array), let's leave it as that for now. Leaving it as that does not require us to keep ViewSubscriber, since AjaxFormController can handle the packaging to an AjaxResponse.array('#markup' => 'Foo')
isn't that much harder than returning'Foo'
.Comment #6
mkadin CreditAttribution: mkadin commentedThis makes a lot of sense to me, but I'm not sure I've got the Symfony chops to write this patch, but I'll try to get it started. To confirm, we're talking about the following steps:
1) Change RouteProcessorSubscriber::onRequestSetController() to route AJAX requests to AjaxController.
2) Create the AjaxController class, modeled after HtmlPageController which invokes _content as a subrequest, takes the content out of the returned Response, and package it into a new AjaxResponse.
3) Rewrite the 'system/ajax' path to use the new routing system, route to a new controller that will process the form and handle the data returned by whatever is in #ajax['callback']. If its an AjaxResponse, just return that, if its a render array, prep it and stick it in an AjaxResponse and return it.
Sound right?
I'm assuming this doesn't need to be finished in the next few days?
Comment #7
Crell CreditAttribution: Crell commentedThe irony is that I'm working on a pjax-ish Drupal 7 implementation *right now*, so I'm staring some of these issues right in the face this week. :-)
I don't think we can forbid returning render arrays, not yet. However, in *most* cases we want controllers returning response objects because that's what gets cached... as strings. Or, actually, if most content objects are actually blocks, then in whatever BlockResponse we end up using that can carry data about attached JS and CSS and meta tags and so forth.
One of the most important things that the WSCCI rewrite is doing is moving the negotiation from post-body callback to pre-body callback. In D7, the delivery callback happens after the page callback is called. Having written code a week ago to take an arbitrary page and turn it into an ajax_replace() call, it's not necessary for it to be a render array that gets returned. A string would work just as well for my use.
ViewSubscriber is, in current form, a total hack to keep the system working. It was always intended to get replaced by something more robust, hopefully SCOTCH.
The request listener that currently says "if you just specify a _content, then you get this _controller" was also a temporary measure. The assumption was that class, and HtmlPageController, would both get removed entirely by SCOTCH and replaced with something more robust; conceptually that could even happen via contrib right now, but I'd rather do it in core.
IMO, the correct solution is, as effulgentsia said and as I noted in the prior Ajax issue, to change the Ajax callbacks in JS to have a custom mimetype. Then the current default-_controller code gets enhanced (mechanism TBD, but this could even be baked right into the router, perhaps) to specify a different _controller if that mimetype is detected. That controller can call _content() the exact same way that HtmlPageController does (via a subrequest, which hopefully turns into a render(), which makes it HTTP cached), and then takes the resulting string and puts it into an AjaxReplace command object, and then puts that into an AjaxResponse object.
URIs that we know in advance will only be used for Ajax purposes can just specify that _controller in the route definition.
In either case, ViewSubscriber becomes unnecessary.
Make sense?
Comment #8
mkadin CreditAttribution: mkadin commentedThis does make sense to me. So it sounds like we need to start by converting core's AJAX JS to use a custom MIME type?
The for the routing piece, I'm not sure how to proceed...is this something we need to wait on SCOTCH for?
Comment #9
Crell CreditAttribution: Crell commentedYes, that sounds like step 1. And no, no need to wait for SCOTCH here. We'll both be editing the same default controller-setting class (the name of which I forget off hand), but as long as this issue gets there first we should be fine. :-)
Comment #10
mkadin CreditAttribution: mkadin commentedIt seemed like a simple thing to do :) Adding the contentType: 'application/vnd.drupal-ajax' parameter to the jQuery's ajax options works well. Ajax requests get sent with that mime type. It works perfectly for an ajax enabled link.
However, PHP doesn't parse the posted data from an ajax submitted form into the $_POST array because it isn't interpreting the data as urlencoded form data. If I add something like this:
(from: http://stackoverflow.com/questions/8213974/empty-post-without-content-type)
It works, but I imagine we don't want to do something like that. Seems hackish to me.
There's an additional problem, which I believe is related to jquery.form.js. Forms that have files in them are submitted via a hidden iframe or some craziness like that. It doesn't seem to be picking up the mime type from the jquery options that are passed along.
There's a patch attached, its not intended to be used, but is just to illustrate the point. I've had to wrap the parse_str hack in a conditional so that it doesn't break forms with files. But again, those forms are not submitted with the new MIME type, and I couldn't figure out how to make it so.
I've also attached a sample module that can help to easily debug this stuff. There at a test form at /mymodule-test-form and a test link a /mymodule-test-link. Both will return the mime type of the request in an alert box. You can comment the file field in and out of the module to see how the mime type changes.
Given the 'forms with files' issue and the hackey parse_str thing necessary for this (is there a better workaround?)...is the mime type worth it? Or should we just stick to ajax calls to system/ajax a la D7?
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedI suppose Crell meant to say that the
Accept:
header of the XHR call needs to be changed, not theContent-Type:
one. But in any case, as you noted it is not possible to do this in the iframe fallback.Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso to be noted:
jquery.form
can do direct XHR file upload on browsers which have a XHR level 2 implementation. This means not every browser, but very close.Comment #13
effulgentsia CreditAttribution: effulgentsia commentedFrom what I can tell, D8 core is not using XHR2 for file uploads on Firefox 12. It's not even using XHR for form submissions with empty file fields. Are we not using a recent enough jquery.form, or not invoking it correctly?
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThat's an unrelated issue. Where we do or do not want to use system/ajax is unrelated to mime type, because even without mime type, we can detect an XHR submission as ContentNegotiation::getContentType() already does.
For IFrameUploads, not setting mime type is the HTTP proper way, because what we return in that case isn't a JSON string, but a JSON string wrapped in a
<textarea>
, or in other words, HTML, as per the mime type the browser requests.I don't know offhand what a good solution to getting form decoding working with custom mime types is. I'll post back here if I think of anything, or await to see what others come up with.
Comment #15
Crell CreditAttribution: Crell commentedErf. Yes, I meant set the Accept header, not Content-Type.
Comment #16
mkadin CreditAttribution: mkadin commentedOk here's a little something to get started with. In this patch:
1) An update to core's copy of the jquery.form.js plugin. I couldn't get the accept header to work on forms with file fields without it. This needs to be tested across browsers.
2) A tiny change to ajax.js to set the accept header to include 'application/vnd.drupal-ajax'
3) An additional conditional in RouteProcessorSubscriber::onRequestSetController() that checks the accept header for the presence of 'application/vnd.drupal-ajax' and sets the controller to the new AjaxController (see next item). It was discussed in earlier comments that this should be done differently (perhaps baked into the router), but I leave that to someone else who knows a bit more symfony.
4) A new AjaxController which extends HtmlPageController. The only overridden method is the content method, which is a combination of HtmlPageConroller's content method and ajax_prepare_response() from D7's ajax API. This method should probably have the most eyes on it.
Everything seems to work well as far as I can tell.
One issue: The new dialog stuff introduced a complication that doesn't really fit in with our new AJAX API.
From ajax_prepare_response():
We can't just insert additional pieces to the insert command's array anymore. That doesn't fit in with the new API. Do we need to include a new title command? A parameter on every insert comment? To me, shouldnt this just be a SettingsCommand?
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedIn #12, I meant to say that we cannot rely on XHR2, as it has only fairly recently be implemented in one of the major browsers.
Comment #18
Crell CreditAttribution: Crell commentedI'd say setting the page title should be its own discrete command.
Why does this need to extent HtmlPageController? It's overriding the only useful method already, isn't it?
This is an awesome level of documentation. :-) I don't see where the title is being added, though. The comment says it, but the code disagrees.
This is probably OK for now, but once the content negotiation stuff goes in we should have an attribute that we can rely on to determine the format to use. This also needs to get generalized, but I'm not sure yet how.
I can't really speak to the JS parts here.
Comment #19
effulgentsia CreditAttribution: effulgentsia commentedRelated: #1851414: Convert Views to use the abstracted dialog modal. So I agree that simply adding 'title' to the insert command the way HEAD now does was just a hack to unblock the modal dialog issue, and that we need a better way. Per #18.2, the thinking I had when adding this was that in many modal dialog situations, the code that is processing the AJAX request shouldn't know or care whether the result will be shown in a dialog or in some other kind of DOM element. As far as the server is concerned, it just needs to return some content, and the client-side code should deal with deciding how to display it. So I've been opposed to 'openModal' / 'closeModal' commands in the general case, though per #1851414-6: Convert Views to use the abstracted dialog modal, I'm not necessarily opposed to such commands for Views or other places where it makes sense for the PHP code handling the AJAX response to control that behavior.
If we want to at least allow some AJAX workflows where the PHP code processing the AJAX response can return content without concerning itself about whether it's in a dialog or not, then we need some generic command that JS code can use for determining how to initialize the #ajax['wrapper'] element that triggered the AJAX request. Moving this from "the Insert command that magically contains a title property" to "a Title command" seems like a fine step. Maybe there's something even better that we can come up with either in this issue or in a follow up (e.g., a "initializeWrapper" command or something like that).
Comment #20
Crell CreditAttribution: Crell commentedRelated thoughts:
1) Yes, we do want any page to be Ajaxable. In fact, any block should be ajaxable, automagically, by design.
2) Perhaps instead of a "replace this wrapper element with this new wrapper and this body", we should just have a "replace the contents of this wrapper element with this body" command? (innerHtml, basically). Would that simplify matters at all? (I would certainly find the latter far more useful.)
3) While a SetPageTitleCommand sounds like a good idea in general (really just a subclass of insert?), do we also want to allow a title to piggyback on some other command? Would that make it easier for a response to be flexible about where it's going to go, but note that it needs a title associated with it? (We've talked about block responses needing that capability anyway to get rid of the global drupal_set_title().)
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedRe #20.2, it would simplify some things, but not anything related to this issue. I opened a new issue for discussing that: #1858562: Change default Ajax insertion method from replaceWith to html.
Comment #22
mkadin CreditAttribution: mkadin commentedRe @Crell:
True. This could easily be re-implemented as its own class if that's a better choice.
Well I'll take the thanks, but I didn't write it. Its a copy and paste from the ajax_prepare_response() docs. I should have taken it out, but I didn't want it to be lost that this code broke the dialog stuff.
I don't think a set title command is really possible - can't individual themes set the id / class of the page title as they see fit? Thus, I don't think we could make a generic solution.
Can't we just send the title along as a JS setting when needed? I don't see it as such a big deal. @effulgentsia, can you elaborate on what you mean by "Maybe there's something even better that we can come up with either in this issue or in a follow up (e.g., a "initializeWrapper" command or something like that)." I'm not really following what you'd like to see happen.
Comment #23
mkadin CreditAttribution: mkadin commentedI'd like to continue to move this forward. Here's a patch that incorporates Crell's feedback from #18.
1) I've split off the AJaxController class off from the HtmlPageController, by doing a little more copying and pasting from HtmlPageController.
2) The comments for the modal dialog title stuff are still there...but a solution for the modal title is not. Does anyone see an easy solution?
Comment #24
dawehnerFrom an issue standpoint the jquery form.js should have been maybe put in it's own issue.
Btw. is there something like composer but for javascript we want to use?
Rerolled against HEAD and fixed some minor problems just to get the flow going on.
Comment #25
dawehnerForgot the interdiff.
Comment #27
dawehnerCore is moving fast.
Comment #28
dawehner.
Comment #29
podaroka lot of trailing whitespace cleanup needed
This patch has a huge amount of logic, hard to review, possibly needs manual testing with different input parameters
Comment #30
podarokstatus
Comment #31
mkadin CreditAttribution: mkadin commentedDo we care about / do we change the formatting of third party libraries? I'm guessing no...
Comment #32
podarok#31 I`m talking not about third party, but about patch additions
Comment #33
mkadin CreditAttribution: mkadin commentedThe extra logic and white space that you're referring to is all in jquery.form.js, which is a third party JS library that is being updated in this patch.
It was mentioned in #24 that this should have probably been its own patch
Comment #34
podarok#33 woops
ok
looks like this still need review without #29 comment
Comment #35
Wim LeersNOTE: #1848880: Test order of AJAX commands: add Ajax\FrameworkTest::testOrder(), strengthen testLazyLoad(), fix JS settings not being prepended is kind of blocked on this issue: without this issue, the tests are testing the old AJAX rendering pipeline instead of the new one (which was introduced at #1812866: Rebuild the server side AJAX API). This in turn has allowed for a regression at #1875632-31: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings), which is causing problems at #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms.
Review
ajax.inc/ajax_render()
being removed yet?system_library_info()
was not updated to indicate the new jquery.form.js versionAjaxController.php
is the single most important change to review; I'm not in a good position to review it because I don't know the entire request/response architecture well enough. Despite that, I tried to give as useful feedback as possible.s/Ajax/AJAX/
s/ajax/AJAX/
s/clean off/clean up/
Missing leading space.
s/Ajax/AJAX/
"in some relationship to" sounds very strange.
Is this entire massive comment really necessary?
Shouldn't this check whether
theme('status_messages')
returns a non-empty string?Looks sensible, except that this is not checking if the request has a "_controller" attribute. Is that intentional? And if so, then why must this be checked for the next if-test?
Comment #36
sunPlease note that we are using "Ajax" in all strings, comments, and documentation in Drupal core. This is documented in the Drupal core string terminology guidelines (somewhere in the d.o handbooks). Therefore, these parts of #35 should be ignored.
Comment #37
dawehnerLet's have separated issues for the ajax controller and the jquery forms update: #1843220: Convert form AJAX to new AJAX API
This is the central question before we should forward. So we have basically two use-cases, the automatic way by #ajax and the manual ajax callbacks, that support custom commands etc. This patch seems to be used just for the first one, while the custom ajax callbacks need for example ajax_prepare_response.
Comment #38
mkadin CreditAttribution: mkadin commentedYes this patch is just for the automatic Form API #ajax property stuff.
#1812866: Rebuild the server side AJAX API covered creating a new API for ajax commands. Once this is in, I was thinking a big 'Convert All Ajax to the new API' issue would be next, which would remove ajax.inc (and hence ajax_render()) all at once.
If we want to split off the new jquery.forms.js plugin into a separate issue, that'll have to happen first as this will be dependent on it.
Comment #39
dawehnerOh damn I linked the wrong issue: #1928202: Update jquery form to the current version (3.27)
Comment #40
Wim Leers#36: huh? Really? That doesn't seem to make much sense? :( Can you link to those docs?
Comment #41
mkadin CreditAttribution: mkadin commentedThe new Ajax API functions / classes (I.e. AjaxResponse) also use Ajax, not AJAX. Just a note in case we end up changing.
Comment #42
mkadin CreditAttribution: mkadin commentedI'm going to hop on this today.
Comment #43
Wim Leers#41: It *can* make sense in function names et al. (cfr. #1627350: Patch for: Case of acronyms in class names (SomethingXSSClassName versus SomethingXssClassName)). I just don't see how it makes sense in comments. It'd be like writing "Html" in all comments, or "Css". We don't do that either. Hence my astonishment.
Comment #44
tstoecklerRe #43: Ajax is "officially" not an acronym (anymore). That still trips me up, too, but that's how it has been decided (don't know by whom).
Comment #45
mkadin CreditAttribution: mkadin commentedWant to get this through, but its been a while since I've done any D8 stuff. Applying the most recent patch gives the following error on any Ajax (or is it AJAX ;) operation:
RuntimeException: Controller "Drupal\Core\AjaxController::content()" requires that you provide a value for the "$_content" argument (because there is no default value or because there is a non optional argument after this one). in Symfony\Component\HttpKernel\Controller\ControllerResolver->doGetArguments() (line 134 of /var/www/drupal8/core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Controller/ControllerResolver.php)
Tried to dig and figure this out but to no avail. Any WSCCI folks know what's up?
Comment #46
Crell CreditAttribution: Crell commentedThat error means that the controller has a $_content parameter, but the $request->attributes bag doesn't have a _content key. That usually means an issue with the route definition.
On what URL did you get that? As written, the AjaxController will only be able to handle fronting for routes that have _content callback defined, which is apparently not all of them. I suspect that code needs to be made more flexible. Without knowing which URL that happened on I couldn't tell you if that's a problem with new routes or old routes.
Comment #47
mkadin CreditAttribution: mkadin commentedIt was doing this on core Ajax form elements which send requests to 'system/ajax'
Comment #48
mkadin CreditAttribution: mkadin commentedThe page callback ajax_form_callback comes in the _callback parameter which was being overwritten. Now it gets stuck in _content before shipped off to AjaxController. I have no idea if that's the best way to do that, but it works.
I've left the capitalization of AJAX as is, but incorporated the rest of wim's feedback.
My biggest concern is what's going on to check for the accept header in RouteProcessorSubscriber::onRequestSetController(), at this point, is there no better way to route on a header?
Comment #49
Wim LeersAn interdiff would've been nice :)
I'm wondering if this:
should be converted to the routing system as part of this patch.
Because, modules that do use the routing system (as they should in D8), like the edit and editor modules, are stuck with a nasty work-around as an alternative to the theme callback in the menu item:
It seems like it might be out of scope, but in that case, a follow-up issue should be created for it.
We should document why the selector is NULL.
Rendering status messages twice.
Unchanged from #35, last remark.
This looks extremely weird.
Comment #50
Wim LeersComment #51
mkadin CreditAttribution: mkadin commentedI do think the ajax_base_page_theme belongs in a separate issue because it applies to non-form AJAX requests as well.
As for Wim's last two points in 49, someone with a better understanding of all things WSCCI will have to guide me, because I'm sure the stuff in RouteProcessorSubscriber::onRequestSetController() needs work.
Also, after further testing, this isn't exactly working. Wrappers in the form are being replaced by a full drupal page, html tags and all, which contains the replacement content. Looks like the subrequest is wrapping the new content in an entire page. How can this be avoided?
Comment #52
mkadin CreditAttribution: mkadin commentedInterdiff
Comment #53
Wim Leers#51:
- Ok, could you create a new issue for that then? :)
- That's scary, that means the current test coverage is insufficient! How could it possibly that the AJAX tests pass if it's not working at all? :/
Comment #54
mkadin CreditAttribution: mkadin commentedRE:#53, Triggering the testbot for #51. I wasn't getting that behavior yesterday, so maybe something has changed.
The html that replaces the wrapper contains all the right stuff, but just has extra, so maybe thats why the tests are happy.
Comment #55
mkadin CreditAttribution: mkadin commentedStrips off the '_legacy' attribute from the subrequest, and solves the 'whole page insert' problem, which was caused by ViewSubscriber::onView()
Comment #56
Wim LeersInterdiff please? :)
Comment #57
mkadin CreditAttribution: mkadin commentedFunny how I always make it and then forget to post it.
Comment #58
Wim LeersI can't really sign off on the WSCCI parts of this patch. But testing it manually causes no problems.
As per #37/#38, the patch of #55 covers the full intended scope of this issue, and a follow-up issue will be created where "the big conversion" and the removal of
ajax.inc
will have to happen.So, I think that the last thing that needs to happen before #55 can become RTBC is sign-off from a WSCCI person. Pinging Crell on Twitter.
Comment #59
mkadin CreditAttribution: mkadin commentedA little more symfony-y way of checking for the accept header.
Comment #60
Crell CreditAttribution: Crell commentedFor the rest and serialization modules, we've been adding new mime types to the request's list of known types on the fly, and then checking against that. See the "HalSubscriber" class in the patch at #1924220-14: Support serialization in hal+json
We should probably do the same for consistency.
Also, this block is guaranteed to conflict with #1934832: Provide a dedicated approach for using forms in routes, which is mucking with the same code. :-( (We're trying to land that by Sprint Weekend.) So this will need to get rerolled once that's in. A follow up on my agenda is to refactor this subscriber to be just a series of RouteEnhancers, which will largely eliminate this sort of conflict.
Other than that, this looks awesome. :-) Nice work, guys!
Comment #61
Wim LeersAs per #60.
Comment #62
mkadin CreditAttribution: mkadin commentedHows this? I used the hal module as a model.
Comment #64
mkadin CreditAttribution: mkadin commentedRebase! interdiff from 62 applies.
Comment #66
Wim LeersYay, proper test failures! If you want to test locally while attempting to fix test failures, I recommend the CKEditor tests — its whole suite runs in <1 minute.
Comment #67
mkadin CreditAttribution: mkadin commentedThis should pass, if HEAD has been fixed ;)
There was a little name conflict for the 'ajax' content type that existed before for all XHR's and the new 'ajax' content type I had created for Drupal form ajax requests. This changes the form ajax requests to 'drupal_ajax' which we can keep, or refactor back to ajax in a follow up issue for this where we remove all the old ajax stuff.
Comment #68
mkadin CreditAttribution: mkadin commentedInterdiff
Comment #69
Crell CreditAttribution: Crell commentedThis looks good to me. We can tidy up that subscriber after this is in. If the bot approves of the latest patch, so do I.
Comment #70
mkadin CreditAttribution: mkadin commentedtagging
Comment #71
Wim LeersHurray :) And yes, HEAD has been unbroken :)
http://drupalcode.org/project/drupal.git/commit/2af2c93
Comment #72
webchickI winced when clicking into this issue, but actually this patch is not bad at all. It's also nice because it starts to show off some of the power we get from all this work around the routing system/Symfony.
Committed and pushed to 8.x. Thanks!
I believe this needs a change notice.
Comment #73
mkadin CreditAttribution: mkadin commentedDoes this need a change notice? Using the #ajax property of a form element should be exactly the same.
Comment #74
jessehsThis commit seems to have broken the diff dialog added by this commit: #1821548: Add a "diff" of some kind to the CMI UI
A form was triggering a dialog via the following lines:
After this commit, however, the dialog does not appear.
I'm not experienced enough to know the correct fix, and nothing jumped out at me looking through the D8 form api documentation.
Comment #75
mkadin CreditAttribution: mkadin commentedGrrrr....I even noticed that this was going to break the dialog stuff in #16...I just never fixed it. That's what I get for finishing an issue months after I start it.
The inserting of the page title into the insert command was sort of hackish though...we need to find a cleaner solution.
Does this mean we need some more dialog tests as well?
Comment #76
mkadin CreditAttribution: mkadin commentedThis ought to fix the modal dialog problem in a quick and dirty way...though I contend...this is no dirtier than the original methodology for getting the title to the modal dialog.
I've created a follow up issue to remove this hacky-ness and come up with a better solution: #1939758: Figure out a better way to get the page title to a modal dialog
Comment #77
mkadin CreditAttribution: mkadin commentedOK, that's green, let's get it in quickly to fix my mistake.
Also, change notice: http://drupal.org/node/1939862
Comment #78
mkadin CreditAttribution: mkadin commentedLike me holding a beer in a facebook picture, you know I be de-tagging.
Comment #79
michaelfavia CreditAttribution: michaelfavia commentedConfirmed proper operation with CMI xml modal as well. RTBC as easy as ABC's.
Comment #80
catchCommitted/pushed the follow-up. #1939758: Figure out a better way to get the page title to a modal dialog is fine for me - the whole page title handling needs to change dramatically.
Since there's a change notice moving this to fixed.
Comment #81
Wim Leers.
Comment #82
swentel CreditAttribution: swentel commentedActually, this hasn't been pushed at all, the dialog on the configuration is still broken, Patch still applies and makes it work, so setting to RTBC once more.
Comment #83
swentel CreditAttribution: swentel commentedOk nevermind, patch in #1870764: Add an ajax command which makes it easy to use the dialog API in complex cases fixes this too.