multiple channels support

This allows calls to adsense_display to optionally set the channel string manually, by passing a string instead of an integer.

which is, by the way, a nice feature to have even when you have a lot of custom channels and are creating classic single-channel ad units . . .

Those should all still work perfectly fine! (The filter tags should continue working too!)

As far as the filter tags, do you want the _adsense_process_tags regexes expanded to include 'plus-separated-numbers'? The problem there is distinguishing robustly whether a single number is a literal channel or an ADSENSE_AD_CHANNEL index. You could pick a magnitude cut-off, but that's a hack.

If you're thinking about replacing a drop-down-list UI with something more difficult (like a text field) that allowed multiple channels, why not go all the way and get rid of all the ADSENSE_AD_CHANNEL stuff completely, and use string channels everywhere?

I think I understand where you're coming from. You see the design goal of the adsense module as following along with, and replicating a portion of the functionality of the "adsense setup" wizard (https://www.google.com/adsense/code)

The ADSENSE_AD_CHANNEL variables are for storing a mirror of up to ADSENSE_MAX_CHANNELS of your custom channels, and refer to them with integers instead of names.

And adsense_display(), the filters, and the block setup are *replacements* for going through https://www.google.com/adsense/code

The reason that wasn't initially clear to me is that ADSENSE_MAX_CHANNELS is defined to be 7, which is *much* smaller than the google limit of 200. I didn't how the module was supposed to work if you wanted to use more than 7 different channels. Increasing ADSENSE_MAX_CHANNELS to 200 would make things more confusing, because instead of worrying about a google channel ID, you now have to worry about a google channel ID and also an integer between 1 and 200.

I've actually been commenting out that variable_get look-up (similar to the patch I submitted here) and using channel ID strings directly because I didn't want to deal with that extra ADSENSE_AD_CHANNEL index.

Given where you're coming from, I have three ideas.

1. Do the obvious extention. A checkbox set/multi-select menu for the block setup, a list/array of ADSENSE_AD_CHANNEL indexes for the filter tag and the adsense_display call. I am uninterested in doing a patch for this, because with large numbers of channels, the ADSENSE_AD_CHANNEL variables become too much to deal with. Plus, it would still require me to maintain a patch on top of the stock module (at the very least, increasing the value of ADSENSE_MAX_CHANNELS).

In any case, something like the patch above is a requied step for this kind of change. If you'd prefer array() instead of string, that'd be easy to change.

2. Get rid of ADSENSE_AD_CHANNEL variables completetly. Use channel IDs instead of ADSENSE_AD_CHANNEL variables. Create an extra "friendly name" which could be associated with channel IDs. Everything else as above, except that the "friendly name" would be used instead of the channel ID for display in UIs.

Personally, I find it more comfortable (and less error prone) to generate a (possibly multiple) channel ID using the google UI and then cut and paste that channel ID than I find generating a channel ID, looking at the adsense settings page to find which of {1..200} it is, then using that number. And certainly much easier than trying to remember which of the numbered channels in the variable table corresponds to which channel name in google.

Again, the patch submitted above would be a required step for this change.

3. Change the design goal. What if the adsense module tried to, as much as possible, leverage google's adsense setup wizard (https://www.google.com/adsense/code)?

Here's one possibility: There would be a page on the site where you could paste wholesale a google-generated adsense block. You submitted that block, and the page would generate for you both a filter tag and an adsense_display() PHP call.

The block config would also contain a text field where you could paste a google generated adsense block.

You could get rid of the database and UI for ADSENSE_AD_CHANNEL variables, as well as the ADSENSE_COLOR_ variables. You could get rid of adsense_ad_formats(), as all these things could be pulled from the pasted block.

It would be less error prone than the current model, in that you'd be cutting and pasting the entire generated adsense code block. Google doesn't let you even select less than the whole block.

This UI would be forward compatible with future google adsense features.

It would cut back on variable lookup calls, which couldn't hurt performance, and the module file itself might end up smaller.

I get your point about themes. One could support that in option 3 by adding a "color group" index to the UI.

But let's focus on the smaller change described in #8. If we do get rid of the ADSENSE_AD_CHANNEL variables (hooray!), then I believe there's one more change necessary: to the filter tags, which would change in syntax from:

[adsense:piggy:1:1]
to
[adsense:piggy:1:35250938] for a single channel
or
[adsense:piggy:1:35250938+235987752+34598235] for multiple channels

(I don't fully understand these tags, but I'm describing them anyway after reading _adsense_process_tags(), so please correct me if I'm wrong)

Thus these things would need to change:

1. the change to adsense_display in the patch above
2. the change to blocks and the new table that you just described
3. the change to _adsense_process_tags (it looks like only the patterns needs to change?) described here
4. update functions for the blocks and tags

The variables could optionally remain in the variables table (?) to support previous adsense_display calls. If not, the patch in #2 could be made stronger -- support for integers could be dropped.

And by "piggy" in the above example, I mean "468x60" =)

So would it be possible to commit the small patch above as a first step?

I started looking into doing the whole thing, but it would require significant changes to the settings page (in that it can't be processed with system_settings_form anymore), or the channel definition would have to go into its own form.

Here's a little bit of work toward getting this done, if that's useful to you. I noticed that the install file is a bit weird: adsense_update_1 is calling adsense_install, which means that any tables created in the install get called again in update_1? They have "IF NOT EXISTS", but it still seems strange.

Let me clarify:

Would it be possible to commit http://drupal.org/files/issues/118876.patch as a first step?

Thinking about this some more, what might be more useful is a function which accumulates channel IDs in a static variable, and appends them with variables passed as parameters to the adsense_display() call.

An example of how this might be used:

A function in a phptemplate theme would detect that the URL is /forum/something, and call adsense_channel_add('02398750'), where 02398750 is a channel ID corresponding to "forum section". A custom module then calls adsense_channel_add('49793234'), where 49793234 is the "profile: female" channel ID. Later on in the theme, adsense_display('928357') is called where 928357 is the "between the topic and comments" channel. Even later, in one of the blocks, adsense_display('43298') is called where 43298 is the "all pages right sidebar" channel.

The two ad blocks would then have these channels set:

1. forum section + profile: female + between topic and comments
2. forum section + profile: female + all pages right sidebar