Over in #1783746: Add support for "between" operator a date filter is being overhauled to do more cool stuff. Here is the issue post for adding a date argument handler. Patch will be posted in the follow-up.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sammys’s picture

FileSize
4.11 KB

Here is the patch. It's not intended to be perfect at this point. It's a foundation for the implementation and it's operational in the use case I have. It's far too late in the night for me to make it perfect.

sammys’s picture

Status: Active » Needs review

Flipping to review status.

sammys’s picture

Title: Add views argument for dates to match what will be committed for the filter » Add views argument for dates

Title change.

Status: Needs review » Needs work

The last submitted patch, 2091499_1.patch, failed testing.

sammys’s picture

Status: Needs work » Needs review
FileSize
2.62 KB

Here's an updated patch with a more stable implementation.

sammys’s picture

Argument now correctly supports multiple dates (comma-separated) with ranges using a semicolon between values. This code also adds support for string dates in addition to timestamps.

sammys’s picture

Status: Needs review » Needs work

I think date filtering must work like this:

  • A single date is a single date: (field = INPUT)
  • Multiple dates to one configured filter will be (field = INPUT1 OR field = INPUT2) and, if NOT is enabled, (field <> INPUT1 AND field <> INPUT2)
  • A single date range will result in (field >= BEGIN AND field <= END) and, if NOT is enabled, (field < BEGIN OR field > END).
  • Multiple date ranges will result in ((field >= BEGIN1 AND field <= END1) OR (field >= BEGIN2 AND field <= END2)) and, if NOT is enabled, ((field < BEGIN1 OR field > END1) AND (field < BEGIN2 OR field > END2))

The NOT case for multiple date ranges doesn't work correctly

UPDATE: altered the specification to avoid using NOT in the query itself

sammys’s picture

Status: Needs work » Needs review
FileSize
2.95 KB

Patch attached has the fix for the last posted problem plus support for spaces separating input argument values.

drunken monkey’s picture

Title: Add views argument for dates » Add Views contextual filter handler for dates
Component: Framework » Views integration
Status: Needs review » Needs work

Thanks a lot for the patch, it's really a nice suggestion for additional functionality!

However, it still needs some work. Here are a few things I've noticed:

  1. +++ b/contrib/search_api_views/includes/handler_argument_date.inc
    @@ -0,0 +1,99 @@
    +<?php
    +
    +
    

    Please add an @file documentation comment here, and also keep to the documentation standards in the rest of the code.

  2. +++ b/contrib/search_api_views/includes/handler_argument_date.inc
    @@ -0,0 +1,99 @@
    +    if (!empty($this->argument)) {
    +      $dates = preg_split('/[, ]/', $this->argument);
    

    You should use $this->value here, since you've already set it up above. That should already contain the split input. It also should only be split if the break_phrase option is set (which fillValue() already does).

  3. +++ b/contrib/search_api_views/includes/handler_argument_date.inc
    @@ -0,0 +1,99 @@
    +    $date = new DateTime($value, date_default_timezone_object());
    +    $date->setTime(0, 0, 0);
    +    return $date->format('U');
    

    Please use strtotime() instead, if possible, as this is used at all other places in this and related modules.

    Also, why do you set the time to midnight? You don't do that if timestamps are passed, there'll be use cases for other times and users could already set the time in the date string, if they want this behavior.

    Though, admittedly, the last is a bit difficult if you also split on spaces – so maybe you should just split on commas? In that case, of course, the description for the break_phrase option would have to be changed, too.

  4. +++ b/contrib/search_api_views/includes/handler_argument_date.inc
    @@ -0,0 +1,99 @@
    +  /**
    +   * Get the title this argument will assign the view, given the argument.
    +   */
    +  public function title() {
    

    This and the rest of the class body seem to be copied from SearchApiViewsHandlerArgumentTaxonomyTerm, though I don't know why. Especially title() doesn't make any sense here – it should be overridden to display the dates properly, though.

    Please use SearchApiViewsHandlerArgument as the base for your class, as that's also the class you are extending (correctly, I think).

    Instead of copying the code, though, especially for the query() method, it might be better to refactor SearchApiViewsHandlerArgument::query() a bit to use helper methods for the individual parts of the methods, so subclasses could more easily override only the bits that are actually different. This would make the code a lot cleaner, and probably also help other argument handlers.

Also, I noticed you don't actually use the handler anywhere – you should include that part of the code from #1 in your later patches as well.

sammys’s picture

Thanks so much for taking time out to review this.

It most certainly does need work and this patch was more a snapshot of where I'm at rather than a "please commit this" type of thing. There are probably a few things needing discussion and then subsequent development.

I'll comment on a point-by-point basis.

  1. I may need to defer updating myself to the latest and greatest documentation standards that have been formalised way after I started playing with Drupal. Perhaps you can add code docs? You are more aware of the documentation standards.
  2. Thanks for telling me about break_phrase. I didn't look into break_phrase. Must have had a brain fart. I'll change the patch to use it (and all my other patches for search_api).
  3. The reason I used DateTime instead of strtotime was because I didn't want to audit core code to ensure the correct timezone will be used when converting the string to a timestamp. I didn't want to spend time on it when on a tight deadline for a client. For their site, PHP is configured to use UTC and the site America/Los_Angeles. After a quick grep through core code it looks like it should be ok for me to rely on it for the patch. I'll update it accordingly.
  4. You are right that I copied the term argument code to use as a starting point. Just didn't care about the title stuff for my client's use case. I'll update it.

I'm doing an update to merge the "between" operator thing with changes in 1.8 and I'll see if I can factor in the fixes to this at the same time.

Thanks again for looking at it.

EDIT: I didn't mention why I set the time. The strtotime function will use the current time of day when making a timestamp from "2013-10-10". That will create the wrong timestamp. The argument, unlike the filter, must support dates as a string. In the exposed use case with a text field, a user will most likely not enter the time. The code will need to check for time input but it should gracefully handle the scenario when no time has been input.

sammys’s picture

Status: Needs work » Needs review
FileSize
6.31 KB

Here is a reworked patch using the break_phrase cool stuff. Unfortunately, we need to expand the allowed values to allow ';' for date ranges along with string dates containing '-' and ':'. I've duplicated views_break_phrase into the class in order to do this properly.

Items 2, 3 and 4 are taken care of by this patch. I'm doing this work for free right now and I have a stack of other paid things I need to do so perhaps you can point out where the documentation can be improved or amend the patch yourself.

Status: Needs review » Needs work

The last submitted patch, 2091499_11-search_api-date-argument.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

Oops, damn, apparently I accidentally already committed your last patch! Good thing it didn't include the search_api_views.views.inc changes …
Anyways, here is a re-roll according to latest dev, with the documentation comments and some other style "issues" fixed.

I also now tested the patch, and it seems to work fine, everything behaves as it should.

EDIT: I didn't mention why I set the time. The strtotime function will use the current time of day when making a timestamp from "2013-10-10".

No, it doesn't. At least not in my version (5.5) – are you sure, and which one are you using?
It does use the current time for strings like "+2days", but I think that makes sense. And, as said, people can still just pass the time if they really want to. Also, I didn't understand the part with filter vs. argument – if it's exposed, it's a filter, not an argument.
I now removed all that magic, since it also makes the code considerably cleaner. Please provide a good argument if you really need it in there.

drunken monkey’s picture

Status: Needs review » Fixed

Committed.
Thanks again for your work!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

sammys’s picture

Issue summary: View changes
Status: Closed (fixed) » Active

Thanks for cleaning up and committing the patch! Apologies I didn't get back to you sooner.

I was aiming for exposure of the argument in panels. Code I supplied had been written for panels integration and it was working. I'll leave this Active for now and post an amendment patch in here once it's ready.

sammys’s picture

Status: Active » Closed (fixed)

I'm gonna set this back to closed in lieu of more focussed issues for bugs.