TARDIS

@ycshen will do.

but will that erase older versions of my code? I read that it's important for reviewers to investigate my coding style.

master branch removed.

something went wrong with the body of my original post.

(I had put the issue description in this comment by mistake. sorry.)

@Samuel Joos: thanks for the input, I appreciate it!

sure enough, I ran code sniffer because I didn't want to upset reviewers ;)

although TARDIS doesn't implement Views (yet!) I created a "custom link" field in which you can specify the path to your view and it'll link there. so instead of linking to the TARDIS page, it points to example.com/your/view (make sure you create a view that accepts arguments in the form of YYYY/MM though).

there's a Roadmap in the project page (Views integration is there), and I'm already bumping up the first request - Entity integration. I'll try to be as democratic as possible once people start chipping in their ideas, but the truth is I'm writing code as I learn...

as for the current month, it's by design. I had the idea to show past nodes, so it begins with the past month, but your mindset is more accurate. I'll make this change once I got to review other folks' modules - there's that too ;)

cheers,
Luciano

Added my participation in the Review Bonus program.

@Samuel Joos,

I worked on your solution and found a bug in my module that I had neglected. guess this means I owe you one! ;) I'll upload a newer version later on, so you can test this new feature tomorrow. thanks for the input.

please note: before you update, uninstall the module using the current version, then download the new version and install it. that's because I've changed a few variables around.

you'll notice that node types and show nodes from the current month both have been moved to a "general settings" area, because those settings affect both the block and the page.

so give it a try and let me know if you run into any errors, ok?

cheers

@FlakMonkey46,

sorry! I was replying at the same time as you and accidentally caused it to go back to "needs review". what are the odds, huh? ;)

anyway, I'll work on your input. thank you!

@FlakMonkey46,

I ended up solving the bug you mentioned while addressing the suggestion from Samuel Joos. also included a call to arg() following your suggestion. I'm unable to add DateTime and DateInterval classes yet, but I'll study those in the future.

and the Notice: Undefined offset bit has been corrected already; I think you grabbed an earlier version of my code.

thanks for the remarks! :)

changing back to needs review...

hi @20th,

thanks for the detailed review! yes, TARDIS is similar, but it's different in ways that matter. I thought the Archive module was long since abandoned, so I didn't even consider using it(*). also, it differs from the TARDIS in that it (Archive) shows a calendar that links directly to nodes.

the TARDIS aims at creating blog-archive-like functionality, and in the future it will encompass week (and fortnight) listings of nodes.

I have heard of Views and Date, hehehe. but what motivated me to start writing the TARDIS is that you can't list a view with arguments like YYYY/MM for date fields. also, you can't build a list of links to years and months without PHP code (**). I mean, come on! Wordpress does it, so how hard could it be? ;)

this functionality has never been very well implemented for Drupal; that is why Date and Views integration are in the TARDIS Roadmap. because I will do it. I saw the future, you know. (no flying cars yet, sorry.)

TBPH, at first I thought I'd write a simple Views style plugin and be done with it. I had (some) working knowledge of how to write a module, but it turned out that writing said code was harder than I thought. so I looked everywhere for a tutorial on how to create a Views style plugin and found Drupalize.me's videos.

They have a series on the basics of writing a module and, out of sheer curiosity, I watched it. it blew my mind: I knew nothing. so I decided I'd take the longer road, one in which I'd learn a great deal more, and here I am.

phew.

but yeah, you can simply set up the block and link it to your view, and it'll work. meanwhile, I'm learning how to integrate with Views, Date, Token, Display Suite etc. etc. etc. all the while giving redshirts like me a working solution that doesn't involve pulling their hair out.

as for the things that caught your eye:

• theme/tardis-month.tpl.php and theme/tardis-year.tpl.php: those are there to foster the noobs. initially I considered theme_* functions, but I decided it wasn't easy enough for the end user.
• js/tardis.js: I seriously thought I were following Drupal guidelines. I'll get right on that!
• _tardis_query_block() function: yeah, I have already gone over my code looking for stuff that could be, err, functionised - but I'll give it another look. granted, this function is complex, but that's working with dates right there.
• tardis.module, lines 53-55: this is weird... I used to have this run through t(), but now it looks like this: $tardis_page_title = filter_xss(variable_get('tardis_page_title', 'Recent nodes')); is this the code you saw? the idea here is that the page title should be customisable in several languages (I have seen people install i18n just so they could change strings in Drupal).

anyway, thanks for the attention. I appreciate it. I'll get this fixed and come back.
____________________________________

(*) mind you, I've been looking for such a solution a little after I joined Drupal. check out how old is this thread.
(**) here's an example, using views, in one of my own projects: http://shieldsaas.com/blog. it took me quite a while to write. if you look closely, years and months get jumbled in the same views row, which is why I can't format it as a UL. and, structurally, it should have been a UL.

hi @FlakMonkey46, I tested the code on my webhost and at home before pushing it - but I'll download a fresh copy of the TARDIS and check. thanks!

edit: I should have ran it again through the automated review. I'll remember to do it on the next commit.

@FlakMonkey46, I erased all local files and downloaded the latest version of each one from my sandbox. it's working.

please check line 128 of tardis.page.inc: it should read:
if (count($tardis_query_arg) == 1 && $tardis_query_arg[0] != '') {

if not, please use the attached file. also, maybe clearing your cache could help. if that's the code you're seeing and you still get this bug, please provide me with steps to reproduce the issue.

thanks for the support!

@20th,

the link you posted advised me to replace

$(document).ready(function(){
  // do some fancy stuff
});

with

Drupal.behaviors.myModuleBehavior = function (context) {
  //do some fancy stuff
};

but the TARDIS js is more like:

(function($){
  $(function(){
    // Time travel awesomeness
  })
})(jQuery);

and I couldn't exactly make it work otherwise. could you please link me to a more detailed article? thanks in advance.

cheers

cool! I'll make sure to include these contributions (page title variable, jQuery call).

the TARDIS is looking so much more awesome with all the input I'm getting. hope I can help other redshirts like me in the near future. I'm seriously thinking about sticking around even after my wooden sandbox project becomes a real module.

cheers

hi @20th,

I think I understand what you mean. here's the code I'll be committing later on:

  $tardis_page_title = variable_get('tardis_page_title', 'Recent nodes');
  $items[$tardis_page_address] = array(
    'title' => $tardis_page_title,
    'title callback' => 'filter_xss', // <-- replacing default call to t() function
    ...
  );

is that ok?

please note: I've checked earlier versions of my code and there was never a call to t() inside drupal_set_title().

cheers,
Luciano

edit: your patch worked just fine. I'll commit that change as well. thanks!

Added more reviews for the same modules.

ok, updated code according to latest input. changing to needs review. standing by.

Added a new review.

Added a review.

thank you, klausi! I'll look into this and get back to you.

I thought about clearing the cache automatically, but I didn't want to bypass site admins' authority. nevertheless, I'll leave a warning telling them which settings force a cache cleanup.

cheers

ok, I did a re-check of all possible strings that should be passed through t().

some strings just have month names and years in them - but month names already go through t(), in _tardis_month_names().

as for the cache, I looked everywhere for instructions on how to clear the menu cache on form submit, but the best I could come up with is:

<?php

/**
 * Checks whether page title or address have changed, and flushes the cache.
 */
function tardis_page_settings_submit($form, &$form_state) {
  $tardis_page_address = $form_state['values']['tardis_page_address'];
  $tardis_page_title = $form_state['values']['tardis_page_title'];

  // Did the page address or title change? If so, we need to rebuild the menu.
  if ($tardis_page_address != variable_get('tardis_page_address') || $tardis_page_title != variable_get('tardis_page_title')) {
    if (cache_clear_all('menu', 'cache', TRUE)) {
        drupal_set_message(t('Caches cleared.'));
    }
  }
}

?>

which doesn't work. I'd appreciate help on this matter - otherwise I believe I should keep that message telling users to clear their cache, with a link.

(in fact I remember trying to get this to work right at the beginning, but no joy.)

btw how can I put line breaks on git comments? I've tried \n but it didn't create new lines as expected.

cheers

@cweagans,

oh ok! I'm still tiptoeing around Git, you know. trying not to bust anything on Drupal.org... hehehe.

fingers crossed here.

expect me after my review. I rather enjoyed the process; think I'll stick around.

cheers

wrong Git link. oops.
added another review.

added reviews.

woohoo! we did it!

[insert Freddie Mercury pose here]

thank you, everyone! :D

and thank you, cweagans. we all got our crazy Mondays... ;)