Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | 2091499-13--views_date_argument_handler.patch | 8.76 KB | drunken monkey |
#11 | 2091499_11-search_api-date-argument.patch | 6.31 KB | sammys |
#8 | 2091499_8-search_api-date-argument.patch | 2.95 KB | sammys |
#6 | 2091499_6-search_api-date-argument.patch | 2.9 KB | sammys |
#5 | 2091499_5-search_api-date-argument.patch | 2.62 KB | sammys |
Comments
Comment #1
sammys CreditAttribution: sammys commentedHere 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.
Comment #2
sammys CreditAttribution: sammys commentedFlipping to review status.
Comment #3
sammys CreditAttribution: sammys commentedTitle change.
Comment #5
sammys CreditAttribution: sammys commentedHere's an updated patch with a more stable implementation.
Comment #6
sammys CreditAttribution: sammys commentedArgument 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.
Comment #7
sammys CreditAttribution: sammys commentedI think date filtering must work like this:
The NOT case for multiple date ranges doesn't work correctly
UPDATE: altered the specification to avoid using NOT in the query itself
Comment #8
sammys CreditAttribution: sammys commentedPatch attached has the fix for the last posted problem plus support for spaces separating input argument values.
Comment #9
drunken monkeyThanks 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:
Please add an
@file
documentation comment here, and also keep to the documentation standards in the rest of the code.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 thebreak_phrase
option is set (whichfillValue()
already does).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.This and the rest of the class body seem to be copied from
SearchApiViewsHandlerArgumentTaxonomyTerm
, though I don't know why. Especiallytitle()
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 refactorSearchApiViewsHandlerArgument::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.
Comment #10
sammys CreditAttribution: sammys commentedThanks 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.
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.
Comment #11
sammys CreditAttribution: sammys commentedHere 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.
Comment #13
drunken monkeyOops, 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.
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.
Comment #14
drunken monkeyCommitted.
Thanks again for your work!
Comment #16
sammys CreditAttribution: sammys commentedThanks 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.
Comment #17
sammys CreditAttribution: sammys commentedI'm gonna set this back to closed in lieu of more focussed issues for bugs.