Theme Autoswitch

I've taken a quick look at this module, and your module needs a bit of work before I can continue reviewing this.
First and foremost, your .info file needs some work. A lot of work. To begin with, what version of Drupal is this for? Check out Writing .info files (Drupal 6.x) or Writing .info files (Drupal 7.x)
Once you've got your .info sorted out, I'll install it on a local copy and take a look. When you've fixed the .info file issue, change the status from "needs work" to "needs review"
Also, just as a suggestion, it's always nice to see version history in the README.txt, although that's not a necessity.
Hopefully we can get you reviewed soon!

I went to set up a trial switch, from March 21 through March 25 (today is the 21st, wanted to see how it handled that use case), and I got the following error:
Fatal error: Call to undefined function date_make_date() in theme_autoswitch.admin.inc on line 237
If this is a call to a function in the Date module, you need to make that a dependency in the .info file. That being said, it does appear to have switched my theme, but I am unable to check to see if the rule is set up properly as if I go to /admin/settings/theme_autoswitch I get that error above.

While I hated this in my code review, I understand why they did it, so I'm going to do it too. A few suggestions!

• In the description of the module, specify that a theme needs to be enabled in order to be switchable. This may be a deal breaker for some sites that let users switch their themes, for instance between a dark and a light theme, but want to use this module as it will expose the theme they want to switch to to those users
• Allow end users to pick a year as well as a month and day. I can envision use cases where a user may want to specify a theme switch for the following year instead of assuming this year
• Instead of having drop down boxes with days and dates, use either your own or the Date module's calendar picker view to chose the dates. This is, IMO, an easier way to visualize exactly when you want the theme switched as opposed to the drop down boxes

Once you get the error fixed, let us know and I'll take another look.

When I got to enable this module through Drush, I get a few warnings and errors that seem to all be coming from one repeated function, these should be fixed.

WD php: date(): It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'America/New_York' for 'EDT/-4.0/DST' instead in theme_autoswitcher/theme_autoswitch.module

First, now that I've gotten it working, the admin menu for it looks great, simple and clean. My only note is that in the menu it gives the time the theme will switch, but they are all set to midnight giving the user the idea that they should be able to pick the time when the theme switches, which is not the case. Adding in the ability to choose the time the theme will switch will moot my point (again, something that doesn't necessarily need to be added to get approval, but something to look into adding later; could be very cool for, say. switching between a daytime theme and a nighttime theme)

An actual error that needs to be solved is, if a theme's name is greater than 10 characters, your module will not let a user use it. I tried to use the Acquia themes, both have machine names longer than 10 characters, and I got an error saying that I couldn't use them. This needs to be solved as there is no way to know the name length of every theme beforehand so hard coding in the 10 character limit will preclude the use of some themes. (Screen Shot attached)

A general question about this module, is the switch based on the user's time or the server's time?

Sounds good on both counts there, going to see if this works now that the bugs are sorted out.
One thing I notice, and this is because of the assumed time of the switching, is that if you have a rule that ends on the current day and you decide to add a rule to today (like I did for testing), it won't activate the new theme. The attached screen shot will give you a more clear view of exactly what I'm talking about. I set up the 3rd rule just now, but it assumed that I wanted it changed at midnight, which has already happened, and has not changed my theme. I've refreshed a few times and the theme still has not changed. It doesn't change if I run cron either. I believe this to be a logic bug caused by the fact that the end user cannot specify the time or year they want it to change.
Another logic bug exists if you try and add a timeframe that crosses over a year, probably due to the fact that I think you're assuming that the date range will fall within the current year. I put in a timeframe from 1 December to 1 February, and the rules admin page gives me a timeframe from Thu, 12/01/2011 to Tue, 02/01/2011. I think that you should either build in a year picker or account for this through logic (I personally believe a year picker would be more versatile) in order to correct this error.

Additionally, inside your README.txt, you should put some text about needing the themes enabled in order to switch to them, possibly with a reminder that if an admin lets users switch their own themes that any theme they want to use with this module those users would be able to switch to (that is, unless you are able to make the module work with disabled themes)

The function

function _getEditLinks($id) {

could best be renamed into

function _theme_autoswitch_get_edit_links($id) {

to prevent naming conflicts and CamelCasing :p

I saw a sandbox project that does this a few weeks ago, and suggested at the time to its creator that it a more extensible way of achieving this would be to write a plugin for Context module that reacts to the date, and then use Context module's theme switching.

Perhaps you can track down that issue and see how the other author is getting on?

Security review:

• SQL injection - there are no sql queries.
• CSRF - project uses Forms API for all forms
• XSS - the project does not have any user input that does not come from pre-populated lists

No security issues noted. I just took a quick look at this because others seem to have put it through the ringer. The author seems to have a good understanding of Drupal, coding style is good, documentation is good. I see no reason why this project should not be approved.

See my comment in #11.

I don't think our job is to hold up applications because we think they should be implemented a different way, unless there is a problem with overall understanding of Drupal APIs. I don't see that in this case. Since we have a disagreement, we'll have to get an admin to take a look at this.

Part of the review process is how a new project fits in with Drupal's ecosystem as a whole, and how it contributes to that ecosystem. My suggestion would make this a much more powerful and reusable piece of code.

There will be an approval process of some kind, similar to the existing CVS account approval process, but far less daunting and time consuming, because:

* It will focus only on "big impact" items: overall understanding of Drupal's APIs, security review, license compliance, blatant duplication of other projects.
* Reviewers can use standard tools like commit logs, diff, and the issue queue to understand the code and its evolution and get a "sense" of the maintainer, thus removing a lot of pain and guesswork from the approval process.
* All non-critical follow-ups (coding standards compliance, naming conventions, etc.) can be handled in the standard issue queue way, and addressed by the author either prior to or after their sandbox makes the transition to a regular project.

Originially posted by webchick. You can read the whole post here.

This is the best 'big picture' guidance we have on the project application review process. I think that what you have done here is part of the problem with the review process. You've substituted your own judgement as to how this should be implemented. That's not what code review is about. There are no real problems with this project. If you want a context plugin, then you should write it, and users will decide what they like better.

I'm changing the status to RTBC, because it needs the attention of an admin at this point. Please let the admin make the decision as to whether he/she will grant full project status or send it back to the drawing board. I've contacted zzolo on this. If you know other admins maybe you should contact them too.

Hi @ccardea and @joachim. Just wanted to throw my two cents in. IMO, you are both right. We should be allowing this application to go through (given security, coding, and best practices), but should be very helpful in telling them how they can improve it, by using other modules, like Context. The key is to provide links and more help so that the user would be more inclined to do it in the long term.

Indeed, this would be a much more extensible and more adoptable module if it was done in Context, but I don't think it's something to hold back this application.

This is just my opinion. Unfortunately this process is still very subjective. I would suggest checking out the Goals wiki page and commenting or adding to. Ideally this would be a resource I could say is definitive, but its not there yet.

Thanks to both @joachim and @ccardea for putting in time to review this, and to @markwittens for the patience.

Oh, sorry, here the link to the Process Goals: http://groups.drupal.org/node/142459

> Indeed, this would be a much more extensible and more adoptable module if it was done in Context, but I don't think it's something to hold back this application.

Yeah, that's fair enough.

Does this already duplicate http://drupal.org/project/themekey and its "a date/time-selected Christmas theme" condition?

This is not a theme :)

@markwittens: Please see #23. Can you investigate if your code duplicates the Theme key module and its time/date condition? Or if it doesn't provide an accurate description as how your module differs from that feature?

From the Application review process for Full Projects; what to expect:

Module 'Foo' is just too complex so I have written a much simpler version.
This is just straight duplication of functionality and applications like this will almost certainly be declined. It is worth remembering that many modules start out simple also and, over time, grow into more complex systems. If you have the know how to write a "simple version" from scratch rather than work out how a complex module works then maybe your effort would be better spent making the existing complex module easier to use (improve it's UI, write documentation, etc).

@markwittens

I think you need to know that Dave Reid is a git administrator, with the power to grant you vetted git user status. He asked you twice now, (#23 and #27) to investigate the duplication issue with Theme Key and provide an accurate description of how your module differs from that feature. Your answer in #28 was not sufficient. It is irrelevant that you think it doesn't matter, what is relevant is that Dave Reid thinks it does matter. So if you want your project approved, I suggest you rethink your previous answer.