Posted by cpliakas on November 8, 2012 at 12:06am
26 followers
| Project: | Date Facets |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | fixed |
Issue Summary
I most likely won't be implementing this but would love to give guidance to contributors.
Comments
#1
subscribe
#2
As an update, I am making a conscious decision to explicitly not take the lead on coding the Search API integration. Similar to Facet API, the goal is to integrate new developers and viewpoints, and if I commit to doing this that won't happen. So, code is gold for this issue.
Chris
#3
There is some code already in #1598702: Facet support for date fields. I haven't looked at it but it could be a start.
#4
Here's a first attempt at providing support for search api, feedback is most welcome.
#5
tried your patch in #4 with search_api
i get
Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1941 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_format() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1951 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1941 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_format() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1951 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1941 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_format() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1951 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1941 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_format() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1951 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1941 von C:\xampp\htdocs\yodrup\includes\common.inc).
Warning: date_format() expects parameter 1 to be DateTime, boolean given in format_date() (Zeile 1951 von C:\xampp\htdocs\yodrup\includes\common.inc).
block looks like in the attachement
tried it on date field from the date module and on node creation date
looking forward
#6
getting same error.
configuration should be right.
Query type: date_range
all sortings for the facets are unchecked.
the date module ist activated. User can not set their own timezone.
In my understanding of the error above, it says that the module makes queries with dates without any timezone.
think you need only implement something like "get the timezone of the user and if no timezone is availabe use the default timezone."
#7
c31ck,
Thanks for the initiative, and great work with the first patch. Marking as "needs work" based on the comments in #5 and #6.
Regarding the timezones, they are very tricky especially with Solr. See #1667866: Content with created date of 1st of a month appears in previous months facet for more details.
Thanks for the contributions and reviews!
Chris
#8
It looks like because the field it indexes is set as a Date. It tries to handle the label as if it were a date.. The label of the facet is text though ('Past week etc') and thus it will result in an error.
#9
Found the problem in the label value callback. Changed the callback for date ranges to a custom one.
Added it to the patch.
#10
#11
Patch #9 work fine for me.
Thx.
#12
Patch #9 works also for me.
Is it normal that doens't appear the number of nodes available for facet's element?
#13
+++ date_facets.module (working copy)@@ -28,13 +29,40 @@
+function _date_facets_api_facet_create_label(array $values, array $options) {
+ $map = array();
+
+ //Loop through all the values to create a map. It does nothing yet but more functionality might be added.
+ if($values) {
+ foreach($values as $key => $value) {
+ $map[$key] = $value; ¶
+ }
+ }
+
+ return $map;
+}
bunch of tabs here :)
+++ date_facets.module (working copy)@@ -70,5 +98,7 @@
+ drupal_alter('date_facets_get_ranges', $build);
Please add some comments here
+++ lib/Drupal/SearchApi/Facetapi/QueryType/DateRangeQueryType.php (working copy)@@ -0,0 +1,82 @@
+
+ $options = array(
+ 'past_hour' => "[$past_hour TO $now]",
+ 'past_24_hours' => "[$past_24_hours TO $now]",
+ 'past_week' => "[$past_week TO $now]",
+ 'past_month' => "[$past_month TO $now]",
+ 'past_year' => "[$past_year TO $now]",
+ );
Not sure if the original module does some validation, but I'd recommend you to do so. This is plain query injection, also for solr. See the apachesolr module for a validation function for a query filter ( public static function validFilterValue($filter) {)
#14
@arrubiu
Yes, this is normal. It is a bit tricky to get the date counts depending on you backend and even version of Solr, so in order to provide the most basic functionality straight filters were used. It would be a great feature request to implement counts, but it is trickier than it may seem to do it in a way that works across backends.
Thanks,
Chris
#15
Patch #9 work fine for me.
great think. thx
#16
I'm not sure why, but I had to add such code at the beginning of the date_facets.module file in order it worked for me
<?phprequire_once(dirname(__FILE__) . '/date_facets.facetapi.inc');
?>
I think that's not correct. But the system didn't see hook_facetapi_widgets() hook otherwise.
#17
After applying the patch got this error:
"Notice: Undefined index: field type in date_facets_associate_widget() (line 34 of /sites/all/modules/date_facets/date_facets.module)."
i also get this error in the "Configure facet display" UI:
"Warning: call_user_func() expects parameter 1 to be a valid callback, class 'Drupal_SearchApi_Facetapi_QueryType_DateRangeQueryType' not found in FacetapiAdapter->loadQueryTypePlugins() (line 346 of /sites/all/modules/facetapi/plugins/facetapi/adapter.inc)."
Any clues?
#18
did you try my comment above?
#19
The errors have stopped after upgrading to the latest versions of all modules.
However I get 'The widget does not support the date query type' still on date fields.
One thing I noticed when testing out Apache Solr is that I get the bottom left query type select box - but don't get that on Search API.
Maybe if we can work out how to display the query type box we can change it to date_range and get it working...
@Soul88 yes I did try but no luck...
#20
I've applied the patch in #9. There were a couple of hunk errors, not sure why, and so I fixed by hand. I've also made some minor improvements based on #13.
Regardless, I don't see any changes when visiting a facet display page. I also don't get a "query type" dropdown on any facet, anywhere – should I be seeing this option?
#21
I've installed #9 without any problems and it seems to work fine.
I do have the "query type" dropdown :-)
Maybe you have to change the Display Widget to Date Query to see it?
#22
I was able to count the number by date range. I had to change a lot of code, and now I can not make a patch because I added to git all the changes. If this functionality is important for some developers, I can prepare a patch.
Past hour was excluded (not need for me).
#23
I think it definitely would be interesting to have a code or a patch
#24
Patch integrate date_facets with search_api
Facet will looks like this:
#25
It's looking good, some minor comments
+++ b/date_facets.infoundefined@@ -6,3 +6,4 @@ core = 7.x
\ No newline at end of file
missing new line
+++ b/date_facets.moduleundefined@@ -28,13 +29,39 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $search
+ //This widget needs a different way to set labels so we add our own callback
comment too long
+++ b/date_facets.moduleundefined@@ -28,13 +29,39 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $search
+ * Value callback for labels with the date range type
missing dots at the end.
+++ b/date_facets.moduleundefined@@ -28,13 +29,39 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $search
+ * Just added a basic functionality. Might need to be expanded for more functionality
wrap comments at 80 chars.
+++ b/date_facets.moduleundefined@@ -28,13 +29,39 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $search
+ * @return multitype:unknown
return value is an array.
+++ b/date_facets.moduleundefined@@ -28,13 +29,39 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $search
+ //Loop through all the values to create a map. It does nothing yet but more functionality might be added.
missing space after //
+++ b/date_facets.moduleundefined@@ -28,13 +29,39 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $search
+ if ($values) {
since $values is an array, no need to check
and it think you can just return $values
+++ b/lib/Drupal/SearchApi/Facetapi/QueryType/DateRangeQueryType.phpundefined@@ -0,0 +1,151 @@
+ * Unlike normal facets, we provide a static list of options.
shouldn't we need a way to configure this, what if I want to show another month or all years separately?
+++ b/lib/Drupal/SearchApi/Facetapi/QueryType/DateRangeQueryType.phpundefined@@ -0,0 +1,151 @@
+ $options = self::getFacetItems();
spacing issue
+++ b/lib/Drupal/SearchApi/Facetapi/QueryType/DateRangeQueryType.phpundefined@@ -0,0 +1,151 @@
+ ¶
trailing white space
#26
Patch was updated.
I tried to make this logic a little more universal. But if you need some complex changes, then I think that you should create own class that extends from Drupal_SearchApi_Facetapi_QueryType_DateRangeQueryType
If this patch will be applyed, please add me as author of commit. It's very important for me :)
#27
Great work, adding the alters is a good idea, and you're right, for specific cases creating a custom class is also an option.
This looks good to me!
+++ b/lib/Drupal/SearchApi/Facetapi/QueryType/DateRangeQueryType.phpundefined@@ -0,0 +1,143 @@
+ ¶
trailing white space
#28
Updated.
#29
Let's see if we can get this committed, so we can move forward.
#30
I am in favor of committing this, the one thing that I have concerns with is that I do not use Search API regularly nor will I probably use Search API in the near future. Therefore I don't want to have a patch that gets no love in the future as that would be frustrating for developers.
We have a couple of options. One is to maintain a separate module. That way it could innovate at it's own pace by people who regularly use Search API and are better equipped to test and resolve issues. The second options is to adopt a co-maintainer who is responsible for maintaining the Search API specific portions of the module, which I am in favor of.
Would love peeps to weigh in on these options and propose others if any come to mind.
Thanks for all the hard work and contributions. Great job!
Chris
#31
I think that a new separate module is not very good. I'm ready to be co-maintainer and resolve issues related to search api.
#32
Excellent! Thanks for offering and I will most definitely take you up on that.
Overall the patch looks good, but I do have a few suggestion / concerns which is why I am marking this as needs work.
@@ -14,7 +14,7 @@ function date_facets_facetapi_widgets() {'handler' => array(
'label' => t('Date range'),
'class' => 'Drupal_Apachesolr_Facetapi_Widget_DateRangeWidget',
- 'query types' => array('date_range'),
+ 'query types' => array('date_range', 'date'),
),
),
);
Why do we need to include "date" query type here? The range query should be a different query than the regular date query, so I am not sure if the widget should apply to date queries.
@@ -28,47 +29,93 @@ function date_facets_facetapi_facet_info_alter(array &$facet_info, array $searchfunction date_facets_associate_widget(array &$facet_info) {
foreach ($facet_info as $name => $info) {
$query_types = array_flip($info['query types']);
- if (isset($query_types['date'])) {
+ // The check on field_type is specific for search api.
+ // @todo check if there is a beter way to do this.
+ if (isset($query_types['date']) || $info['field type'] == 'list<date>') {
$facet_info[$name]['query types'][] = 'date_range';
+ // This widget needs a different way to set labels
+ // so we add our own callback.
+ $facet_info[$name]['map options']['value callback'] = '_date_facets_api_facet_create_label';
}
}
}
Minor nit-pick, but could we change the "value callback" option to be named "label callback"? To me the function creates the label from the value, so the option name should reflect the label. An analogy is that hook_menu() has a "title callback" that returns a title from other variables passed to it, so it reflects what is being returned as opposed to what is passed to it.
+/**
* Returns render arrays for all date ranges.
*
* @return array
* An associative array of date ranges.
- *
- * @todo Implement an alter hook?
*/
function date_facets_get_ranges() {
$build = array();
+ if ($diff < 3600) {
+ $build['past_hour']['#count'] += 1;
+ }
+ if ($diff < 86400) {
+ $build['past_24_hours']['#count'] += 1;
+ }
+ if ($diff < 604800) {
+ $build['past_week']['#count'] += 1;
+ }
+ if ($value['filter'] > $last_month_timestamp) {
+ $build['past_month']['#count'] += 1;
+ }
+ $build['past_year']['#count'] += 1;
+
$build['past_hour'] = array(
Where do we get
$difffrom? To me it looks like it is not initialized which will likely throw errors and not work as intended.+ drupal_alter('date_facets_get_ranges', $build);+
return $build;
}
There is actually another issue at #1836818: Implement a hook to alter the facet values desinged to tackle this. I'd rather add this hook in a follow-up commit since an date_facets.api.php file will need to be created to thoroughly document the hook so that developers know it exists and how to use it.
@@ -50,7 +50,16 @@ class Drupal_Apachesolr_Facetapi_Widget_DateRangeWidget extends FacetapiWidgetLi
}
}
-
+ // Order items as in ranges.
+ // @Todo maybe this functionality should be configurable.
+ $new_element = array();
+ $ranges = date_facets_get_ranges();
+ foreach ($ranges as $key => $range) {
+ if (!empty($element[$key])) {
+ $new_element[$key] = $element[$key];
+ }
+ }
+ $element = $new_element;
// Render the links.
parent::execute();
}
Does this override the sort options that are in the facet display configuration page? I don't want to confuse users by exposing a sort option just to have it overridden behind the scenes. Maybe this isn't the case, but I want to explore it further.
Again, thanks for all the hard work on this issue. Great job.
Chris
#33
Yes, seems you right. I remove 'date' from array.
Unfortunately I can not change the key, because the search_api_facetapi module, uses key 'value callback'.
Sorry, it's wrong copy-paste :(
Ok, I will do it in next commit.
Yes, you right, it's wrong ordering of facet items.
#34
new patch
#35
#36
@Patch #34
Syntax error on line:
$build[$key] = ['#count'] += 1;#37
Unfortunate mistake. Fixed.
#38
Previous patch didn't apply for me and had some errors.
Here is a fixed patch that worked for me.
#39
It's not full patch for search api integration
#40
Thanks @eugene.ilyin,
forgot to include new files in patch.
Here the full patch.
#41
maybe problem was in
; Information added by drupal.org packaging script on 2012-11-09
version = "7.x-1.0-beta1"
#42
Maybe.
@eugene.ilyin, I left that out too, because it was causing Notices.
+ $build['past_year']['#count'] += 1;Few lines below it's overridden with NULL.
#43
Yes, it's wrong code. Removed.
#44
Any news? Can you commit my patch please?
#45
Hi eugene.ilyin.
I just got back from an internet-less vacation. Please bear with me as I wrk through my backlog. It looks like the hooks are still in there. If we can remove them and work together at #1836818: Implement a hook to alter the facet values we can RTBC and commit.
Thanks,
Chris
#46
Just wondering if there are any specifications that I am missing for this. I have this set up with a Date type field that uses Repeating dates. It's returning incorrect counts for the facets and when I click on a facet - no records are being returned.
Thanks!
Elizabeth
#47
Hello.
Please check that settings of your facet according to http://i.imgur.com/PlxiBoz.png
If it not helps, try to debug in function build of Drupal_SearchApi_Facetapi_QueryType_DateRangeQueryType
#48
@cpliakas
>I just got back from an internet-less vacation. Please bear with me as I wrk through my backlog. It looks like the hooks are still in there. If we can remove them and work together at #1836818: Implement a hook to alter the facet values we can RTBC and commit.
You mean that I should remove drupal_alter in date_facets_get_ranges() and add drupal_alter in function build of Drupal_SearchApi_Facetapi_QueryType_DateRangeQueryType?
#49
eugene.ilyin,
Yes. I really like that hooks are being fired and agree they should be there, but I really want to discuss the naming of them and iron out the documentation in an api.php file so that developers know that they exist and how to use them. To me they aren't a critical part of getting this patch through, so I would rather commit this without the hooks and add them in later after vetting their name and writing good docs than delay this any further. I am willing to discuss if you feel otherwise.
Thanks!
Chris
#50
Hello.
Ok, lets try to commit that without alter hooks and add them later.
I attached new patch
#51
Looks good to me! Will add you as a maintainer and will post a list of conventions that you should follow so we can work together efficiently. Then you can commit your hard work! Stay tuned.
#52
@eugene.ilyin,
You now have access to write to VCS.
As mentioned in the post above, there are a couple of conventions that I would appreciate you following:
Other than that, glad to have your involvement and I am excited to have someone give some love to Search API in this project. Feel free to commit the patch in #50 at your leisure.
Chris
#53
Thanks - my settings were correct but I figured out what I was doing by debugging - I had changed the filter settings.
Now I just have to figure out how to aggregate by node for events that repeat. Thanks!
#54
Patch #50 was applyed .
Please add info on page with information of module, that search api is supported.
@cpliakas, thanks. I'm glad to help.
#55
Noted on project page.
#56
Follow-up bug from this issue at #1986494: Undefined index "field type" on line 34 of date_facets.module.
#57
Follow-up bug from this issue at #1988354: Rename SearchApi/FacetApi folder to match convention used elsewhere..