The Ajax framework was originally intended to be used with FAPI. It was later extended to make it possible to use it with links or other elements.
The current framework is not flexible enough and the code is more complicated than it needs to be. It is also unclear how to use it from the frontend.
Some practical goals (bold are what is addressed by the patch, the other are kept for historical purpose and putting initial comments in context):
- try to simplify and merge certain portion of the code,
- find a better place than Drupal.ajax to keep created ajax objects,
Lay down the API and clean it up,check the implementation of each method of the API,Validate the way we're dealing with ajax commands on the JS side,BONUS: Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events
This is a JS issue, no changes on the PHP side of this framework.
New API
There is no need to have a dummy DOM element to trigger an ajax request. The ajax object has a new 'execute' method to programatically trigger an ajax request that uses the Drupal API.
var ajaxObject = Drupal.ajax({url: 'some/url.json'});
ajaxObject.execute();
// All created ajax objects are stored in the Drupal.ajax.instances array.
Before
new Drupal.ajax('ckeditor-dialog', $content.find('a').get(0), {
/* ajax settings */
event: 'ckeditor-internal.ckeditor'
});
$content.find('a')
.on('click', function () { return false; })
.trigger('ckeditor-internal.ckeditor');
After
var ckeditorAjaxDialog = Drupal.ajax({/* ajax settings */});
ckeditorAjaxDialog.execute();
Beta phase evaluation
Issue category | Task |
---|---|
Issue priority | Major because it's very hard to use the Drupal ajax API from the frontend, making it useless. |
Prioritized changes | The main goal of this issue is developer experience, coding standards conformity, and code clean-up. |
Disruption | Disruptive for core/contributed and custom modules/themes because it require an API break and a slight internal code refactoring. |
Comment | File | Size | Author |
---|---|---|---|
#120 | core-simplify-ajax-1533366-120.patch | 515 bytes | nod_ |
#119 | Screenshot 2015-05-26 16.03.39.jpg | 628.49 KB | LewisNyman |
#118 | cantona-seagulls3.jpg | 26.6 KB | alexpott |
#103 | interdiff.txt | 3.91 KB | nod_ |
#103 | core-simplify-ajax-1533366-103.patch | 29.11 KB | nod_ |
Comments
Comment #1
litwol CreditAttribution: litwol commentedTo whomever going to tackle this,
see this module (it is in the patch in linked issue) that makes use of various ajax initializers from php side to trigger front end ajax behaviors: http://drupal.org/node/1473314#comment-5871142 . Useful for testing during ajax.js refactoring. Good luck.
Comment #2
Ralt CreditAttribution: Ralt commentedI may have missed something, but it looks like there is not many functions in ajax.js (11, to be exact). And except minor issues, I haven't found anything needing a complete rewrite.
What is there that I completely missed?
Comment #3
Wim LeersI encountered a use case for Drupal.ajax that is not well supported by Drupal.ajax in the Edit module (part of Spark).
Copy/pasted from a chat:
So:
Drupal.ajax
. (Why not usejQuery.ajax
? Because then you still need to deal with calling the right AJAX commands, animations, etc. —Drupal.ajax
does this for you.)It also needs to be less "wrapper/id"-centric: maybe you want to use
Drupal.ajax
just to retrieve custom AJAX commands, for which you don't actually need to specify a wrapper that's going to be replaced/deleted/appended to/whatnot.Comment #4
effulgentsia CreditAttribution: effulgentsia commentedI'm happy to help on the PHP side (if anything is needed there) and with patch reviews, but need nod_, Wim, or another JS developer to lead the effort here on the JS side.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedAdding WSCCI tag, because to the extent we refactor things here, let's also use it as an opportunity to follow proper REST standards. I don't know yet what that means in detail, but will add links/comments here as I learn, and maybe other WSCCI developers will as well.
Comment #6
nod_Yep, I'm still thinking about it. We should have a JS sprint/meeting in a month or so where this will come up. Hopefully we'll have a plan after that.
Comment #7
nod_oups, tag
Comment #8
sunOne potentially major problem I see for this refactoring:
While I believe we have at least some basic test coverage for the fundamental Ajax framework functionality, AFAIK that does not cover all details of the expected functionality, and I also think we do not have many tests for the individual commands. The original patches that introduced the Ajax framework in D7, as well as various follow-ups that fixed regressions and added some tests, never added a complete suite of test coverage.
Technically, refactoring a subsystem can be done relatively easily, if there's a decent amount of test coverage, since that ensures that no expected or required detail in functionality is lost. Without that, very detailed reviews and careful coding/collaboration will be required.
Obviously, we never had frontend/JS tests, so I'm mainly talking about the PHP interaction layer here, for which we extended Simpletest with drupalGetAjax() and drupalPostAjax() methods, which apparently parse the Drupal.settings in the page output and resemble the JS Ajax commands in PHP.
Comment #9
nod_If that can make people feel better I've been all over the AJAX framework as well as pretty deep with ctools ajax JS code, especially the modal commands. I know how it work and I have time to spend on it, that should help things during the refactor.
The ultimate regression tests will come from VDC I guess. But you're right, it might be a good time to go over AJAX testing and see how it can be made better as well, should that be in an other issue ?
Comment #10
attiks CreditAttribution: attiks commentedRegarding the testing part, if you want to use/extend the testswarm module, I can try to free up some time to help.
Comment #11
nod_Another use case to take care of: #1671432: Drupal.ajaxPageState.theme overwritten with the front-end theme on ajax request, causing second ajax request to load theme css
Comment #12
Crell CreditAttribution: Crell commentedJust spotted this via a tag. I cannot speak to the JS side, but from a REST perspective, IMO this should be a custom mime type.
That is, the browser sends an XHR request with mime type application/vnd.drupal-ajax (or something like that; I forget the proper format off hand) to /system/block/$block_id/$node/$user/$whatever. That in turn maps to a controller (nee page callback) that returns a Response object. But not just any response object. It returns a subclass of JsonResponse (probably; maybe just Response, I don't know). That subclass has extra methods on it for us to use that correspond to the various array elements of the current ajax return array. That lets us focus on making that Response class have a really really nice API, and separate out the concerns of the internal data structure (which could be very similar to what we have now, potentially).
Then on send(), that object takes whatever data has been passed to it and parses that down into an appropriate JSON string, then sends that. That JSON string could potentially be exactly the same as what we have now, or it could get cleaned up. I have no preference there.
At least that's how I envisioned it working. Someone else feel free to suggest an alternative, but that's my current thinking there.
Comment #13
nod_The PHP side sounds lovely like that. I'm not sure we have much to clean-up in the json data that is passed around.
The PHP side definitely needs it's own issue too :)
Comment #14
Wim LeersCrell++ :)
Comment #15
Steven Jones CreditAttribution: Steven Jones commentedFollowing on from #8 do we need to come up with a list of things we need to test, and get those into Drupal 8 first, and then tackle the refactor?
Comment #16
klonos...coming from #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors
Comment #17
Crell CreditAttribution: Crell commentedI'm inclined to say "write the new system, unit test it properly, and throw out the old tests". :-)
Comment #18
kristiaanvandeneyndeI'm willing to try and tackle (parts of) this issue.
If I get this correctly, a complete rewrite on both back and front end is a viable option?
In that case:
Comment #19
Crell CreditAttribution: Crell commentedI am not an expert, but I think we do want to add history/pushstate support, per: http://groups.drupal.org/node/247073#comment-801003
Definitely we want to improve the error handling, which feels funky at times now.
The internal API is also something we want to improve, which the custom response object should help with by providing real methods rather than anonymous arrays.
Anyone else?
Comment #20
Steven Jones CreditAttribution: Steven Jones commentedSeems really short-sighted to implement each of the current arrays as a method on a response object, if you go that way then how do I extend the ajax commands with my own? Would it be better to build up a list of objects that represent the commands if we're going to perform? Then I can extend by just creating a new subclass, and adding that to the response object somehow.
You can the current 'API' for the PHP side here:
http://api.drupal.org/api/search/7/ajax_command
Also, it's currently practically impossible to use the AJAX framework with GET requests, #956186: Allow AJAX to use GET requests, because of the amount of data being unnecessarily sent by the server. It's not abnormal for the requests to be significantly bigger than the responses at the moment. Ideally we'd get that cleaned up, even if by default most of Drupal core's AJAX request used POST.
Comment #21
nod_see also:
#1372840: Add more triggers in ajax.js.
#1060296: ajax_command_invoke - does it work with $selector = NULL?
Comment #22
kristiaanvandeneyndeFrom all the links posted here so far I've gathered that the new AJAX implementation should:
I've also noted a wish for PJAX, i.e. using the HTML5 History API.
This might not be very practical in combination with forms, though.
Comment #23
Crell CreditAttribution: Crell commentedRe #22, that seems like a good target list.
Re #20, a more robust framework-esque approach to shuffling commands forward seems sensible, but I worry about complexity and time to implement. Do you have an idea for how that can be done expediently without introducing several additional layers of complication (both for implementers and for people using the API)?
Comment #23.0
Crell CreditAttribution: Crell commenteddetails
Comment #24
Kiphaas7 CreditAttribution: Kiphaas7 commentedUpdated the issue summary (see 'bonus').
Comment #25
Steven Jones CreditAttribution: Steven Jones commentedSo what we have at the moment in AJAX commands is not completely dire, in the case of implementing your own menu callback and returning an AJAX response you'd do something like this in Drupal 7:
But it should be noted that most of the time when dealing with AJAX most people will probably be using forms, and actually you don't really have to do anything different at all then, and you don't have to care about the commands really.
I guess that in an ideal object orientated world then we'd replace the methods with objects, and define an interface something like this (and yes, this would need to be namespaced):
Then the above callback would become more like:
At some later point in the chain, the response is rendered and renders each individual command passed to it, as it does now.
Not sure if that really makes anything clearer, and at some point we still have to render our command down to an array suitable for converting to JSON.
We really should get some feedback from the people originally involved in the AJAX framework, which I think came from ctools?
Comment #26
Crell CreditAttribution: Crell commentedMy initial thinking was to have a fixed list of commands with known methods for each, which is therefore super easy to document and for IDEs to auto-hint. However, I like the idea, in general, of making it extensible with command objects. I believe that's still an improvement over the current status quo, because we can encapsulate cases where we're doing more than just throwing a string into an array. It's also still more self-documenting than an anonymous array is. (I couldn't actually find any documentation in core for the various commands the last time I was looking; as command objects, the docblocks are entirely self-evident.) It also makes it more obvious that you can (presumably) extend it with your own commands. (We'd need to figure out how the client side of that works, of course. Maybe go all hypermedia on it's arse and send the necessary command JS along with the response? Or rather, have no client-side code waiting for it at all, and have the command object just compile down into the code to run on its own, thank you very much, have a nice day.)
Comment #27
Steven Jones CreditAttribution: Steven Jones commentedAs I previously hinted at the current documentation for the ajax commands are here: http://api.drupal.org/api/search/7/ajax_command
Comment #28
Crell CreditAttribution: Crell commentedWow, there's more than I thought! Which is itself indicative of something. ;-)
Since we want to OOPify that code anyway so that it can get taken out of the bootstrap process, we should probably get started here. Who wants to take a first crack at it? (Assign to yourself and get cracking!)
Comment #29
kristiaanvandeneyndeI'm still interested in trying to tackle this, but I'm currently tied up in (paid) work.
So if anyone with more time on his hands wants to kick things off already, don't let #18 stop you.
Comment #30
Crell CreditAttribution: Crell commentedIn today's WSCCI meeting, cosmicdreams said he would dig in on this issue to get it bootstrapped.
Comment #31
Wim LeersSounds like this issue is moving forward :) Thanks to all of you!
Comment #32
mkadin CreditAttribution: mkadin commentedI'd be willing to help. I'm very familiar with Drupal's AJAX API, though I'm not quite as familiar with all of the new WSSCI changes. Has there been any bootstrapping of this yet?
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commentedYes please help move this one forward. I haven't been able to do so.
Comment #34
mkadin CreditAttribution: mkadin commentedSeems to me (who has admittedly very little context in these discussions) like this #1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply might need some doing before the AJAX stuff can be worked on no?
Comment #35
Crell CreditAttribution: Crell commentedI don't think it's related. VIEW listeners are only called if what the controller returns is not a Response object. If it is a Response object, they are not even called.
If the AjaxResponse class described in #25 is a subclass of Response (or, probably, JsonResponse), then it would do its ow self-compiling in its send() method. None of the View listeners would get called, so ViewSubscriber (which is just a collection of View listeners) would be ignored.
Thank you for stepping up, mkadin! If you have questions, pop into #drupal-wscci in IRC and we'll try to help you out.
Comment #36
RobLoachCould Backbone help us out here? #1149866: Add Backbone.js and Underscore.js to core
Comment #37
Crell CreditAttribution: Crell commentedI don't know backbone well enough to have a firm opinion, other than to ask "do we really need something that big for a task this small?" It feels like overkill.
Comment #38
mkadin CreditAttribution: mkadin commentedOK i've been reading up in an attempt to get this started. If we want to follow through with #12...what are the first steps?
Am I right in saying that we need to set up routing for a custom mime-type to a controller that is specific for our ajax work? If so, I can't seem to figure out how the mime-type routing is implemented.
Comment #39
nod_This one is the JS issue, it needs renaming or taking the PHP side of things in an other issue. I'm happy with the json that's returned for JS to take care of things, I don't have an opinion on the PHP side of this thing but this issue was meant to rewrite the handling and interraction with the ajax framework from the JS side.
Please rename this issue as a meta with an issue summary or open a new one PHP-specific. Thanks.
Comment #40
Crell CreditAttribution: Crell commentedmkadin: The mime-based routing doesn't exist yet, so for now just have normal controllers that return a Response object, which should skip any of the existing mime-sensitive logic.
I don't much care what issue it's in, so go ahead and spin up a new one if you want. :-)
Comment #41
mkadin CreditAttribution: mkadin commented#1812866: Rebuild the server side AJAX API is a new issue for rebuilding the PHP end of things.
Comment #42
Crell CreditAttribution: Crell commented#1812866: Rebuild the server side AJAX API just landed. Yay!
A thought I had recently: Currently we've a lot of code on the client side to take in a bunch of array data from PHP, parse it, map it out to a series of Ajax functions that we are then loading whether it's needed or not (not on every page, but even if you're just using replace you need the whole file), etc. And it's still fairly inscrutable.
What if instead we stripped the API down to bare-bones, and allowed the AjaxCommand object to generate not an array for Drupal.settings but the actual Javascript to execute? So an "insert before" command simply turns into rendering out the one-line jQuery command to run an insert(), which in the browser just gets eval()ed (or whatever the safe alternative is). PHP generating Javascript is not an issue, and this way the browser needs barely any code in it, and processing the commands that come back is a for/eval loop, basically. It also makes the code you have to actually read much much simpler, and you don't need to worry about loading ajax.js. Its replacement gets thin enough that we don't need to break it out to a separate file. Writing a new command gets easier, too, because everything is in one place.
Thoughts?
Comment #43
pounardWhat you're suggesting is a big eval() on server side generated code?
Comment #44
Wim Leers#42: I have a hard time imagining what that would look like exactly — either your explanation isn't sufficiently clear, or I'm not getting it (very possible — I'm tired). To be honest, it sounds like a JS developer nightmare. It sounds like you intend to move *all* (or as much as possible) logic to the server side. That's already what Drupal.ajax is about today. We need to move it the other way: Drupal.ajax should become easier to use for JS developers.
Comment #45
Crell CreditAttribution: Crell commentedWim: Well, right now we do all the JS client side, but control it all server side. That makes both sides gross.
Currently, AlertCommand (which I use as an example because it's the simplest of our available commands) generates a PHP array, which then gets JSON-ified. There's then Javascript code on the client-side (and a non-trivial amount of it for some commands) that takes that array and translates it into an alert() call. Most of the commands are also jQuery-dependent.
I'm suggesting instead that AlertCommand look like this:
And then the client side is dead-simple code to eval() that. (Again, I don't know client-side standards these days well enough to know if eval() is still as legit client-side as it used to be, or if there's a preferred approach.) Then, writing a server-sent JS command (which you no matter what need server-side code for) consists of "one PHP class in one place that can do whatever it wants", rather than "one PHP class in one place and a Javascript function in another place that you have to make sure is already loaded on the client just in case it may get used, communicating back and forth via arrays that in general seem to presume jQuery although it's not clear if that's a hard requirement". It's probably faster, too.
Drupal.ajax becomes easier for JS devs in that it goes away, and they don't need to try to balance JS and PHP together and keep it all lined up. It's all packaged up in one PHP class; and that doesn't introduce a "need to know PHP" requirement, because to write a new Drupal.ajax command you already have to know PHP, because, well, that's what that system is: Server-side actions that respond with client-side actions.
Comment #46
nod_We can't eval() JS returned from the server like that. Alert is an easy example but it's an example nobody uses. Try having the insert command JS on the PHP side. We use JSON to do the transport and that's great, that's what it's made for. The jQuery dependency is just something that needs to be taken out of commands that don't need it. I agree that we might want to split commands into their own file, and have each command depend on a JS file implementing the command but JS on the PHP side, not going to happen.
The refactor is not about commands which are a good concept with a sensible implementation, it's about the mess that is Drupal.ajax, how to make an Ajax call that is using Drupal commands, calling attachBehaviors when it needs to, etc. Currently you need to have a DOM object and a DOM event to trigger a call. We shouldn't need either to call anything from contrib (or core).
Comment #47
kristiaanvandeneyndeAlthough it sounds like a good and clean(er) idea at first, passing server generated code into eval() can quickly become a nightmare, both implementation and security wise.
Keep in mind that not all PHP devs know what can and can't be done safely on the frond end. Same goes for JS devs and back end code. I'm afraid we'll unwillingly open the gates for XSS attacks.
Comment #48
Crell CreditAttribution: Crell commentedHow is that any worse than it is now, where writing a new Ajax command requires writing both PHP code and JS code, just in separate files and in more abstracted and indirect form?
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedThe part of this that I agree with is that half of our server-side commands reduce to an 'insert', and I'd be in favor of removing them, and making PHP code just use the Insert command and pass the desired 'method'. Then again, now that we have these commands as Drupal\Core\Ajax\* classes, if we want to leave the specific InsertCommand subclasses around, but maybe change their render() implementation to call parent::render(), or remove the render() override and instead give them a constructor that sets $this->method, we could do that too.
We also have the 'invoke' command for simple 1-line jQuery method invocations, and if we're willing to keep it, then we can drop the 'css' and 'data' commands, unless in #1842576: Review AJAX system commands, decouple from jQuery we decide that we want those commands to be available without jQuery.
What we then have left is a pretty small set of commands: insert, remove, changed, invoke, add_css, settings, restripe, and alert. We can remove 'restripe' if we get #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors in. I think we might want to replace 'alert' with some more generic way of dealing with errors, warnings, and notices. The remaining commands, though, seem like the right "API" for server-side code to communicate to the client, and I don't think it makes sense to reduce it further. However, we may want to look at other AJAXy frameworks like Backbone to see if they've come up with better or more standard APIs we'd rather model.
Anyway, I agree with #46 that rethinking the command API should happen in #1842576: Review AJAX system commands, decouple from jQuery, and that this issue should focus on the other details of Drupal.ajax(), so retitling this issue. Please feel free to further improve the title as it becomes clearer on what exactly should be done.
Comment #50
sunI agree with the aforementioned arguments, writing actual JS in PHP doesn't sound like a good idea.
We already introduced the
invoke()
Ajax command, which essentially is a catch-all command to apply any kind of jQuery method on the jQuery object of matched elements, and which can be used for many jQuery plugins without having to write a custom/specialized Ajax command on the PHP side nor the JS side.The main differentiation however is that the Ajax commands only transfer data to be used as function arguments in JavaScript. They don't attempt to be the actual, executed JS code. (Aforementioned
invoke()
command borderline crosses that line, but as mentioned, it has a clear purpose and allows one to do really neat things.)I guess that @merlinofchaos had very good additional reasons for designing the original Ajax framework with this separation, and I could very well imagine that he tried a pure PHP-driven solution first. It would probably make sense to ask him for his experience and reasoning as the original architect of the Ajax framework before discussing this further.
Comment #51
sunComment #52
nod_The PHP issue was split off, tag doesn't make sense for the new scope.
Comment #53
merlinofchaos CreditAttribution: merlinofchaos commentedMore or less I agree with nod.
Changing how the commands are processed isn't really going to benefit, I don't think; fixing up Drupal.ajax to be simpler and easier to deal with would be important. Drupal.ajax is a merger of my original AJAX code from CTools Drupal 6, which didn't have that kind of a construct, and ahah.js from Drupal 6 core, which did.
Comment #54
merlinofchaos CreditAttribution: merlinofchaos commentedTo elaborate, these are my reasons for the separation:
1) I don't really like eval() to begin with, and didn't feel confident I understood the security implications. Whereas, a simplified "macro language" which is what I think of, is much safer in general.
2) The AJAX commands, at least the non-trivial ones, can need context, so if you're writing complex code, you're more or less going to have to do what is done now anyway, only you'll have to eval something that calls to your object. This would likely be more brittle than what we have now.
3) Debugging eval() processed from JSON is much harder than debugging Drupal.AJAX.command.foo with input data.
Comment #55
nod_from #1810934: Adding custom ajax progress indicators more easily
Comment #56
nod_I know it's past API freeze and all but we can't make thousands of developers miserable because of Drupal.ajax in D8 cycle. Raising priority.
Comment #56.0
nod_detach/events bonus point
Comment #57
nod_Comment #58
Jelle_SI've done some really, really small stuff, just to get the discussion going again and maybe get some actual coding done.
Drupal.ajax
toDrupal.ajaxInstances
.execute
method was added.I've read the discussion on being able to do GET requests. I've not implemented it yet as I'm not sure what the implications would be for the html ids and css and js files (ajax_page_state)
Comment #59
Jelle_SComment #60
Wim LeersJelle_S++
Comment #61
Jelle_SThanks! Any other suggestions/improvements? I'll be happy to work on this if and when I have the time to do so :-D
Comment #62
joelpittetThis looks great @Jelle_S.
Drupal.ajax(null, null, 'my-url');
It would be nice if the arguments were reversed, but that would be an API change, though I like this feature regardless!@nod_ what do you think of #58. Can we use it as a jumping off point to move this forward?
Comment #63
Jelle_S#62: Yeah I would have loved to do that as well, but I'm not sure what the policy is regarding API-changes and JavaScript... I'm guessing it's the same though.
I'd love to hear more comments/feedback regarding #58. I'm not really sure what else should be added/removed/refactored?
Comment #64
nod_Way overdue RTBC. This is good, we need it to keep our sanity.
Doesn't apply though, when it does, I'll RTBC. Need reroll.
Comment #65
Wim LeersComment #66
valthebaldRerolled patch attached.
2 chunks failed to apply in patch#58:
- Drupal.ajax[element_settings.selector] = new Drupal.ajax(base, this, element_settings);
+ Drupal.ajaxInstances[element_settings.selector] = new Drupal.ajax(base, this, element_settings);
changed to
- Drupal.ajax[element_settings.selector] = new Drupal.ajax(baseUseAjaxSubmit, this, element_settings);
+ Drupal.ajaxInstances[element_settings.selector] = new Drupal.ajax(baseUseAjaxSubmit, this, element_settings);
becaus of the change in constructor parameter name
Comment #67
Jelle_SStraight reroll of #58. RTBC as per #64.
Comment #68
nod_Looks good to me, there are a few eslint errors, please get them fixed before I can RTBC. Also a small improvments in the progressbar stuff:
I was going to complain about progressindicator but 2 years ago I put this in scope for this issue :p anyway a little improvment:
should be replaced with the more explicit:
if (progressIndicatorMethod in this && typeof this[progressIndicatorMethod] === 'function') {
Comment #69
Wim Leers#68: wow, I never even would've known the the current code is equivalent with what you suggest :D
Comment #70
valthebald@nod_: can you list eslint errors that you get? I'm sure not everyone uses this tool. Or - if possible - they can be fixed in a followup issue?
Comment #71
nod_Everyone working on core JS should be using this tool, it's in our docs : eslint configuration. For information: it can't be fixed in a follow-up, the policy is to not commit code that doesn't lint. We went to the trouble of fixing every last linting error, it's not to introduce new ones :)
For the sake of making this easy here is the error (thought there were more):
Fixing that and the comment in #68 and we're RTBC.
Comment #72
valthebaldFixed spacing and changed check as per #68
Comment #73
nod_Tested it, there is a problem when using quickedit, the progress type is undefined and it crashes the JS.
Need to add some code to check that
this.progress
is defined before messing with it.Comment #74
nod_Comment #77
nod_testbot gone wild
Comment #78
Wim LeersThis issue is basically about fixing the absolutely horrendous DX of logic triggering AJAX commands, rater than letting the AJAX Framework trigger it itself — therefore this is basically a requirement for making front-end developers doing advanced things in Drupal 8 sites not shoot themselves in the head, like I had to (see #3).
s/ajax/AJAX/
Hallelujah!
I think it'd be nice if Quick Edit's JS were updated to use this, then there also is an example in core of using this.
Comment #79
dawehnerI kinda dislike that the patch not only change one thing, but also renames the variable for the instances. This kinda makes it hard to review ...
Comment #80
nod_You won't like this one but Wim will :p
Made some more changes. Most important one is that
Drupal.ajax
is not a constructor anymore and it's parameters got reordered to something sensible.Before:
After:
The old
Drupal.ajax
has been renamedDrupal.Ajax
, because constructors get capitalized, it's in the standards. Drupal.ajax throw an error to help things when people refactor.Drupal.ajax automatically tracks all ajax objects created in
Drupal.ajax.instances
, that way no need to polluteDrupal.ajax
or think about putting the object somewhere everyone can see.I replaced all the wonky uses of Drupal.ajax in quickedit, editor, ckeditor. Everything works fine for me, views included.
Comment #81
Fabianx CreditAttribution: Fabianx for Acquia commentedRTBC + 1. but agree this needs a change record and IS update to explain the changes introduced here.
The split of ajax and ajaxInstances is a real real real win!
When I read code like that a year ago for the first time, I was like: But why ... how ... what?
With splitting the class and the instances this makes it way way way cleaner ...
Comment #82
nod_Comment #83
nod_Made a CR to explain changes.
Comment #84
Wim LeersInconsistent capitalization: sometimes "AJAX request", sometimes "ajax request".
Should use the same capitalization as the serverside code.
So much better :)
But, the
progress: {type: null}
bit can now also be removed, I think?So very much better!
Comment #85
nod_Changed comments. About the progress type, looks like it could be removed but don't really want to remove it here.
Comment #86
Wim LeersComment #87
Fabianx CreditAttribution: Fabianx as a volunteer commentedLooks great now!
Comment #88
alexpottAre we not concerned about any confusion between
ajax
andAjax
. Also should we make the params consistent?Comment #89
nod_About the naming it's to make things easier on people who port modules. If they don't want to change their code they can replace
Drupal.ajax
withDrupal.Ajax
and things will work for them, the API will not be used as I expect it to but it's good enough.Now for anyone who used the drupal Ajax api and know how hard it is to work with, the benefits of this change will be clear and it shouldn't be an issue to move a few parameters around (because I also want to get to a point where we have some JS documentation at least).
One thing we could do though, is to go jquery-like and have only one parameter that is the whole config object, essentially putting
base
andelement
(the two extra parameters) in the config object like the rest of the options. It would make detecting a bad use of the improvedDrupal.ajax
easier too.What do you think?
Comment #90
Fabianx CreditAttribution: Fabianx as a volunteer commentedAssigning to alexpott for feedback, but the proposal in #89 looks sensible to me.
Comment #91
nod_With some perspective, I agree it's a good idea :p
Reroll + API change described in #89.
Views, quickedit, node forms are working fine.
Comment #94
nod_Comment #95
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, looks great. We need a CR update for #89.
Comment #96
nod_CR updated.
Comment #100
nod_#2156295: ajax_views.js has some extraneous code? broke it. Reroll.
Comment #101
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC!
Comment #102
alexpottI think the changes in #91 look great and will indeed make porting much simpler. Looks like they need documenting in the code though. The CR looks good to go.
Comment #103
nod_Not sure what more to add.
Comment #104
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedChanges look great!
Comment #105
Wim LeersI bow to Théodore, for making this obtuse mess a thousand times less painful to deal with.
Comment #106
nod_Bow to Jelle_S first then :)
Comment #107
Wim LeersD'oh, stupid me! Of course, I bow to Jelle_S too!
Comment #108
alexpottThanks for addressing all my minor nits. This does indeed look like a great clean up. Given the support for this change and how hard it is to use the current implementation I think we should proceed with this change under the maintainer discretion provision in the beta evaluation. The changes necessary to a contrib module will be minor and now the error message will help them.
Committed 4c49e1e and pushed to 8.0.x. Thanks!
Comment #110
nod_Thanks!
Comment #111
Wim LeersIf I think too much about how much pain, misery and lost time I suffered because of the state of
Drupal.ajax
prior to patch, I may actually end up crying and curling up in a corner.This makes it much, much more feasible for JS to use Drupal's AJAX system.
Comment #112
Jelle_S@Wim haha, thanks! Don't throw your back out with all that bowing ;-)
Glad to see this fixed!
Comment #113
yched CreditAttribution: yched commentedI totally second @Wim's #111.
Thanks so much @Jelle_S & @nod_
/me throws his hat in the air and makes loud high-pitch noises
Comment #114
Wim Leers:D
Comment #115
pounardLike seagulls you mean?
Comment #116
yched CreditAttribution: yched commented@pounard: right, a yodeling seagull, something like that
Comment #117
Wim LeersROFL
Comment #118
alexpottIn homage to french men talking about seagulls...
Comment #119
LewisNymanThis broke the UI of the fullscreen AJAX throbber:
I think this code now inserts the throbber inline even if it a fullscreen throbber. The fullscreen throbber was designed to only be inserted after the body tag to prevent it accidentally inheriting styling in other contexts. See #2207695: Expand the throbber API to include a 'full screen' option
I'm not sure I understand the changes here enough to know if this can be fixed in a followup so setting to needs work.
Comment #120
nod_Leftover line. Should have been removed.
Comment #121
LewisNymanAh nice :) Thanks!
Comment #122
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedPlease open a new issue. Per some policy (that I can't find right now), regressions need to be either reverted and then a new whole patch uploaded or pushed to a follow-up issue to ensure that all data is correct.
Comment #123
alexpottYep please let's get a followup for #119 and #120. Sorry to be a pain but followups on issue mess with metrics, issue statuses and credit.
Comment #124
joelpittetFollow-up created here: #2496137: This broke the UI of the fullscreen AJAX throbber
Comment #126
joelpittetComment #127
webchickLooks like either way, credit gets messed with (nod_ and LewisNyman didn't comment in the spin-off issue, so can't be credited) so just committed/pushed #120 instead.
Thanks!
Comment #129
dawehnerReally nice API improvement!
Comment #130
joelpittetAwesome!