Support for slashes in callback function

I included this patch in the Drupal 7 release of js_callback, I attributed you in the commit.

http://drupal.org/project/js_callback

The 7.x branch contains the fix, it still needs to be included in the 6.x branch. I'll apply the patch here soon.

Strong objection, I plead that we remove this functionality again asap.

My arguments:

1) Using the first patch version breaks wildcards (as neilnz already noted).
2) Iterating through arrays the way patch 2 proposes is at the cost of the "high performance" attribute in the module name and this...
3) ...is - IMHO - not counting with up the rather subjective advantage of having long, verbose callback paths like js/module/wants/to/do/something (while we could also have js/module/wants_to_do_something for the exact same purpose). The internal routing of any callback request is ideally done in a dispatcher function with far less overhead.

Thus, set back to "active".

I really think there is something to this patch. Using a slash in your path is more Drupalish that using underscores. This solution matches that of core and that's an advantage I think, having a consistent way of working is always a plus.

I totally agree that each addition should be weighed and tested on performance impact before implementing it. I did some benchmarks on both solutions. With a 100000 iterations the patch takes 0.3 seconds longer to finish, that's 0.000003 seconds longer per request, not the performance improvements we should be looking for I think.

About the wildcard, correct me if I'm wrong but I think that we didn't support this before either so that's no change. We just still don't support it (and probably never will as that would be just that bit too much I think). Or am I missing something?

The wildcard support would be nice, maybe even a very simple solution which would only impact the standard performance with 1 extra if might do the trick. I'm going to look into this, one option I'm thinking of is NOT including the % in the callback but do have a page arguments array. I'll create a separate issue (with patch) once I have something..

The js module actually only bootstraps to database by default, really the minimum level for Drupal. It only bootstraps to session when you provide an access argument or callback. Bootstrapping to session does have a significant impact on performance but it's still a lot faster than the full bootstrap, especially when your Drupal site has a lot of modules active.

I like the consistency with hook_menu, that's one reason for me to include the patches and the reason for me to look a bit further into the wildcard thing.

I created a patch for the wildcard support in #1872076: Add support for wildcards. I'd love some feedback..

I have thought again about all the feature things. As I don't want to dupe my comment, please see http://drupal.org/node/1872076#comment-6869210

Just two personal and additional notes about the wildcard and "closer to hook menu" stuff:

1.) It is pretty easy to work around the "no wildcards" thing IF NECESSARY. I make use of the callback handler in many of my custom modules, and just every information I want to pass can easily be put into the XHR post data and eventually transformed in the primary callback handler.
2.) Of course the way JS.module worked until now was NOT "drupalish". "Drupalish" means: Always use the common API. And this is, IMHO, what everyone should do even if he uses JS.module by times. A request that gets so complex that you want to work with wildcards, automatic object loading etc., can always be put in a standard drupal request by not naming it "js/module/function/%object" but "module/function/addition/%object/view/%".

And this is the way I personally would always deal with this module which is an "undrupalish" workaround.

For the rest, see the other comment thread, linked above. No offense, guys, but we should REALLY keep it as simple as possible.

Hi again, folks.

It turns out that this is not just additional performance cost, but in fact it breaks existing callbacks under circumstances as I just noted in a test.

Funniest sing aside: You already COULD use sort of wildcards before!

Actual issue:

• callback name = "js/module/callback"
• Request path: js/module/callback/param1/param2
• Callback function just uses arg(3); and arg(4); to read out the wildcards.

Now after the slashes patch, this does not work anymore, because now break the wildcard option. I have no more way to have a callback for the request path above which may vary from js/module/callback/1/1/ to js/module/callback/44444/66666. I wouldn't like to imagine how many existing sites this eventually breaks!

So I really suggest we leave it as it was before and let modules do the arg() thing where it's needed. With this we have wildcards, slash performance AND maximum performance :-D

The wildcard support itself does not break the existing callbacks. You can still have only part of the url in the callback. The rest of the parts of the url are still passed as arguments, I tested this and it works. Maybe the page arguments got in the way. You can optionally add this option to configure which arguments you want to pass along.

I had did some thinking on the suggestion of splitting up into seperate branches. This is not ideal but then I though of another way: Making the featureset configurable.

I just committed a huge change in the 7.x-1.0 branch with the following changes:
I added a configuration page for the js callback module. Here you can configure which features you want. All the features added compared to the original 6.x branch are configurable. Via a template file the js.php file is generated which you can download on the configuration page.

The .htaccess configuration is now also displayed on this form. It's dependent on the base_path of the Drupal installation.

You're right in the fact that small things add up but in this case, it's adding a few cpu cycles (if statements) on a huge amount of them (the smallest js.php implementation is still a lot of code executed). I really still think the performance difference with and without the options are too small to be noticeable but now you have a choice.

Set to review, I'd like to give some option of comments before making it a beta2 release.

Yes, this sounds like a good idea - we will only have to look very closely at all imaginable scenarios and test them.

I also had the simplest idea that keeps us from splitting oder deciding between functional and performance.

Let's just have two callbacks in one version.

First pattern like the original one:

js/module/callback

with as less features as possible, leave all to the dev.

Second pattern like:

jsx/module/callback/and/if/one/likes/even/with/9000/slashes/and

all new features which we consider making sense. This also minimizes the risk that new features touch existing implementations but still not limits any new idea from being brought up and even if it costs (few) CPU clocks, no one is forced to use it. I really feel like this would serve all our needs, even the real extreme ones. Let me know your thoughts.

If you like it, I'll have it built still before christmas is over ;)

Hi again.

I have identified the problem.

Abstract:
While before the recent changes $args was array_shift'ed twice, the second shift has been dropped in favor of implode(). Thus, the first argument being passed to call_user_func_array as $args is no longer the first argument but the callback name.

Solution:
I have added the second array_shift back again. Now everything works again in my test scenarios (they depend on arguments being passed).

I will fix this now in the 6.x branch, then check whether the same problem exists in the 7.x branch and fix it as well.

OK, The 6.x branch is fixed. While checking the 7.x branch, I must say, that I really like your idea.

But, there is one thing I didn't think of at first either and perhaps we underestimated yet: Config options have to be set by site builders. These folks are 1) not always into coding or just Drupal internals. They know how to download and install modules and if they read that JS would be "better" for their performance, they will install it - but not necessarily know whether and how to configure these options. So for that point, I would like to re-think the further development. But I will open a separate issue for that and I suggest we continue there. :)

Final information: array_shift($args) issue is now also in the 7.x dev branch. I also fixed some small issues regarding the admin menu paths and some code formattings, see commit notes.

The commits look good (I'm wondering how I missed some of the coding issues.. Thanks for fixing them) except for the $args thing in the 7.x branch. The array_shift call breaks the page arguments feature. As is required to not break compatibility I added another patch that fixes the page arguments feature, I committed it to the 7.x-1.0 branch.

Oh. I thought I had understood the logic of your arguments extension and was sure it would have also passed the callback name. Are you sure it didn't?

Because of the Added array_shift the first part of the defined callback was removed. This causes the supplied arguments to not align with the expected parts, this is why the latest commit was nessecary.

Ah, ok. I see. So the defined logic expects the callback name as an argument? I overlooked that as it seems :)