Needs review
Project:
Apache Solr Search
Version:
6.x-3.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
25 Mar 2013 at 16:26 UTC
Updated:
31 Jul 2015 at 14:36 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
zerolab commentedand the patch
Comment #2
zerolab commentedComment #3
SamChez commentedHey zerolab, thanks for the contribution. I ported it to 7.x-1.x-dev. In the ported version, I changed the default granularity to be day instead of minute, because day might be used a bit for frequently than minute.
Also, since this is for D7, should I be posting this ported patch here or as a new issue with the version tag 7.x-1.x-dev?
Comment #5
jongagne commentedMy coworker of mine was logged into my computer and so I posted Patch #3 on his account. It failed because it was being tested against the 6.x-3.0-rc1 branch instead of the 7.0-1.x-dev branch. I'm not familiar with standard practices with porting, but I'll create a new issue for the port under the appropriate branch. If anyone is familiar with different standard practices for this process, please advise. Thanks.
Comment #6
jongagne commentedComment #7
David_Rothstein commentedThis mostly seems to work but here's a reroll fixing a few things:
This can't be right since $granularity is a string. I actually just removed this whole section from the patch because it seemed to me that this issue is more about limiting the options that are displayed inside the facet rather than limiting the query.
I don't think this can be an option in the UI since there's code elsewhere that prevents it? So I removed it.
I also tried to simplify the code a bit using facetapi_get_next_date_gap() but then realized there's an edge case where my simplification fails: If, for example, you configure the facet to have granularity of "year" but the actual date range of all your site's data is within a single month, it will not work and you'll get the full drilldown again. So we could go back to the other method, I suppose, though in practice I think it takes effort to get into this situation since it only happens if the actual range of your data is way smaller than you expected it to be, in which case maybe the system knows better than you :)
The other thing I was wondering about this patch is whether the admin form is really defined in the right place in the code? It looked funny but I didn't know a better place offhand.
Comment #8
David_Rothstein commentedActually, there's an even simpler way to write the code, and it avoids the edge case I mentioned above also. We can simply use strtotime() to compare the gap and granularity for us.
Comment #9
zerolab commentedHi jongagne, David_Rothstein,
thank you for the patch tweaks, they look great.
I had the same thought about the admin forms, but could not find any simpler or easier way to do it. Also could not find a more suitable place either.
Cheers,
Dan
Comment #10
nick_vhChanged SECOND, that was a string to the FACETAPI_DATE_SECOND constant and tested the patch. Works fine but has some issues with date facets (which is the problem of date facets).
Committing this and needs backport
Comment #11
kevin.dutra commentedHere's an equivalent D6 version to the patch in #8.