Refactor ajax_render() and clean up 'ajax' element type

Looks much cleaner to me. If file upload for image/filefield is still working, i think we are rtbc. lets see what bot thinks too.

Alrighty. RTBC.

+++ includes/ajax.inc	17 Feb 2010 05:03:41 -0000
@@ -405,7 +406,13 @@ function ajax_deliver($page_callback_res
+  // We don't want to do everything done in drupal_page_footer(), but we want to
+  // do some of it.

Why?

+++ modules/file/file.module	17 Feb 2010 05:03:42 -0000
@@ -214,7 +214,7 @@ function file_ajax_upload() {
+    return array('#type' => 'ajax_commands', '#ajax_commands' => $commands, '#ajax_header' => FALSE);

On a unrelated note:

I really wonder why I have to read over "ajax" 3 times here...

#type => ajax
#commands => $commands
#header => FALSE

would be more than sufficient...

Powered by Dreditor.

This is looking great.

Does it make sense to just get rid of ajax_render() in favor of drupal_render()? We could do special processing in a #theme=theme_ajax and a #pre_render could handle the header/js stuff. just a thought.

ok, works for me. rtbc under the assumption that the test failures are unrelated.

#pass on http://qa.drupal.org/pifr/test/32528

I agree with the changes here. Truth is that the AJAX framework, albeit committed months ago, is still relatively new, and that there are almost no modules using and leveraging it. Well, at least those innards that this patch is touching. The only "module" I can think of is the "Examples" module.

As effulgentsia already stated above, it makes much more sense to commit this now, instead to have to work with the current system for the next years.

Subscribing.

OK, let's do this. Committed to CVS HEAD. :)

This commit broke all the AJAX Commands Framework.

You can demonstrate with the Examples module, AJAX Example, "AJAX Framework Commands" . None of them work any more, not even the alert.

Was this an interface change?

subscribing

It is interesting how fast things move along. This didn't just break Examples. It broke D7 views3 (completely): #748168: Fix ajax interface.

It seems like we should have a complete checkout of D7 code so that we could develop an early warning system for seemingly small issues like this. It was a perfectly reasonable thing to do, but we should have some way to notify about interface changes. How could we do that? Hmm.

Not sure if your question is tongue in cheek. We have such a system. It is the simpletest for contrib modules. For example: http://qa.drupal.org/pifr/test/26130.

Not sure how broadly available it is at this time. And, it has no links from the project page which is silly.

No, it wasn't tongue-in-cheek, and Examples module does have tests and is on the testbot plan. It doesn't have tests for the AJAX stuff, and I think the bogo-tests that we usually use to pretend we're testing AJAX wouldn't have caught this. Views also has tests (and I think is on the bot).

If you read through the issue, you'll see that there was only *speculation* about how many contrib modules this would affect (and it was only 50% correct, or perhaps less). What I'm saying is that if we could at least grep through current code to find an interface change, it could provide an early warning system for an API change. The devs who were considering an API change could actually notify people it would break.

The contrib simpletest program also doesn't lend itself to providing any warning of core shifting underneath it. You do not get notified when tests go haywire until you do a commit and see it fail. So it's not exactly what I had in mind.

@moshe, does that make any sense?

Again, I'm not complaining about this API change, just speculating on how we can improve our process.

At this point in time, we do not change APIs anymore, unless we are talking about critical issues. This issue however, without being critical, changed APIs, and, you're right in that we assumed that it would only break 1 (or perhaps an amount near to 1) module.

Therefore, this issue should have been critical, or otherwise, you perfectly have the right to fight against a technically not necessary API change at this point in time. However, this issue wasn't critical. So I can understand the underlying reason for any complaint. I guess it was me who suggested the API change, and eff simply turned that stubborn idea into an awesome but still API changing patch, so it's probably me who's to blame for suggesting it in the first place.

That said, the changes of this patch will most probably make all AJAX implementations much easier. Moshe is right that the testing framework "should" basically warn about such issues, but then again, contributed module branches are only re-tested after a commit. On the other hand, given that we still have 140+ critical issues in the queue, I would naturally expect API changes for each one of those. Normally, API changes like this would be noted in subsequent Drupal core releases, so already ported contributed modules have a chance to react on them.

Please blame me if you want to assign blame.

I am definitely not trying to assign blame, nor do I have a problem with this issue, nor am I complaining that I had to fix one piddling little problem.

I'm asking if there's a way our process can be improved.

So let's just call this a critical, absolutely necessary fix/API change. What could we do in cases like this to notify owners of code that are dependent and will certainly break?

Webchick suggests a note to the developer's list. That's a cheap and easy shot. Better than nothing. But could we authoritatively determine what modules were going to break with an API change? Is there a way? Could we build a way?

If we could figure it out, it would save an enormous number of work hours in D8. Chasing HEAD is enormously painful, as we all know... and it might be that we could make it easier the next time around.

I've used this page http://drupal.org/node/277268 before to grab a local copy of Drupal.org's CVS repository, which can then be grepped. It's a bit trickier than it used to be since 7.x code might be in HEAD, DRUPAL-7--1, DRUPAL-7--2... as you can see at http://drupal.org/node/97084 there's apparently already one project that's up to DRUPAL-7--4 (?!?) Ugh.

So let's just call this a critical, absolutely necessary fix/API change. What could we do in cases like this to notify owners of code that are dependent and will certainly break?

Thanks for your concern Randy. We actually had this API change break the Lullabot DVD, jQuery and JavaScript in Drupal, which was filmed a few months back. At least we hadn't sent it to production yet, but I have to say, the syntax changes here are nice but pretty unnecessary.

@quicksketch: I'm now the official notifier of API change commits. Angie tries to notify me and I try to follow API change commits, and send out an explanatory note to the Developer's list. That's the best we could come up with. We tried to send to the CVS maintainer's list, but it seems to be broken. Sorry it broke you too!

reopening: did this get added to the 6/7 module update page, and should it have been? If it doesn't need to be added there, please revert status/tags.

@jhodgdon, this is not a D6->D7 change but rather a change that occurred to the interface of the new D7 AJAX commands. Since it did its damage way back in March, and only to people who were already using the new D7 stuff, I think it probably doesn't need any additional documentation. Marking fixed - overrule if you think I'm wrong.

Do we need to update any existing parts of the 6/7 update guide that might have mentioned the old way of doing things in 7?

I can't find any reference to AJAX commands in the 6/7 update guide. I *think* that's as it should be, as this capability wasn't thought of (except in CTools) for D6. So there's no "update" required, but rather a new feature.