We already created a String query type. We should also create one for dates.

Attached is a patch as a first step.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marthinal created an issue. See original summary.

borisson_’s picture

Issue tags: +Needs tests

So I know that this is still WIP, but I have some smaller things that can be fixed. Overall this is looking like it's going in the right direction.

  1. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +/**
    + * A string query type for core.
    

    /s/string/date/

  2. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +    $tables_joined = array();
    

    /s/array()/[]/

  3. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +    // Obtain the start and end dates.
    +    if (preg_match(FACETAPI_REGEX_DATE_RANGE, $active_item, $matches)) {
    

    /s/FACETAPI/FACETS/

  4. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +    // Makes sure there was at least one match.
    +    /*if (!$this->adapter->hasMatches) {
    +      return array();
    +    }*/
    

    Is there a reason this is commented out?

  5. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +
    +
    +    foreach ($query_info['joins'] as $table_alias => $join_info) {
    

    An additional newline here should be removed.

  6. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +
    +
    +      foreach ($result as $record) {
    

    An additional newline here should be removed.

  7. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +          $gap = FACETAPI_DATE_DAY;//facetapi_get_timestamp_gap(min($timestamps), max($timestamps));
    

    /s/FACETAPI/FACETS/

  8. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +        } else {
    

    The else { should go on a new line.

  9. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +          $gap = FACETAPI_DATE_HOUR;
    

    /s/FACETAPI/FACETS/

  10. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +      // Treat each date as the range start and next date as the range end.
    

    Good comment! ++

  11. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    @@ -0,0 +1,204 @@
    +        if (NULL !== $previous) {
    

    this should either be an !is_null($previous), or change the condition around, the drupal coding standards don't allow Yoda if.

  12. +++ b/core_search_facets/src/Plugin/facets/query_type/CoreNodeSearchDate.php
    index 0000000..2d735ed
    --- /dev/null
    
    --- /dev/null
    +++ b/facets.date.inc
    

    This can be converted to a service, so we can unit test as well.

  13. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    +define('FACETAPI_DATE_YEAR', 'YEAR');
    ...
    +define('FACETAPI_DATE_MONTH', 'MONTH');
    ...
    +define('FACETAPI_DATE_DAY', 'DAY');
    ...
    +define('FACETAPI_DATE_HOUR', 'HOUR');
    ...
    +define('FACETAPI_DATE_MINUTE', 'MINUTE');
    ...
    +define('FACETAPI_DATE_SECOND', 'SECOND');
    ...
    +define('FACETAPI_DATE_ISO8601', 'Y-m-d\TH:i:s\Z');
    ...
    +define('FACETAPI_REGEX_RANGE', '/^[\[\{](\S+) TO (\S+)[\]\}]$/');
    ...
    +define('FACETAPI_REGEX_DATE', '/^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})Z$/');
    ...
    +define('FACETAPI_REGEX_DATE_RANGE', '/^\[(' . trim(FACETAPI_REGEX_DATE, '/^$') . ') TO (' . trim(FACETAPI_REGEX_DATE, '/^$') . ')\]$/');
    ...
    +function facetapi_isodate($timestamp, $gap = FACETAPI_DATE_SECOND) {
    ...
    +function facetapi_get_next_date_gap($gap, $min_gap = FACETAPI_DATE_SECOND) {
    ...
    +function facetapi_get_timestamp_gap($start_time, $end_time, $min_gap = NULL) {
    ...
    +function facetapi_get_date_gap($start_date, $end_date, $min_gap = NULL) {
    ...
    +function facetapi_format_timestamp($timestamp, $gap = FACETAPI_DATE_YEAR) {
    ...
    +function facetapi_format_date($date, $gap = FACETAPI_DATE_YEAR, $callback = 'facetapi_format_timestamp') {
    ...
    +function facetapi_get_next_date_increment($date, $gap) {
    ...
    +function facetapi_gap_compare($gap1, $gap2) {
    

    /s/FACETAPI/FACETS/

  14. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    +/**
    + * Helper function to convert dates from Unix timestamps into ISO 8601 format.
    

    We can change this to Converts dates from unix timestamps to ISO 8601 format.

  15. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    + * @param $timestamp
    ...
    + * @param $gap
    

    Need type hints.

  16. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    + * Return a date gap one increment smaller than the one passed.
    

    /s/Return/Returns/

  17. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    +  switch (TRUE) {
    

    I really dislike a switch (TRUE) { statement, I guess a bunch of if statements isn't much better though.

  18. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    +  if (null === $min_gap || facetapi_gap_compare($gap, $min_gap) >= 0) {
    

    see earlier comment about is_null

  19. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    +/**
    + * Converts ISO date strings to Unix timestamps, passes values to the
    + * facetapi_get_timestamp_gap() function to calculate the gap.
    + *
    

    The first line of the comment should be a single line.

  20. +++ b/facets.date.inc
    @@ -0,0 +1,361 @@
    + * @param $start_date
    ...
    + * @param $end_date
    

    let's add typehints here as well.

marthinal’s picture

Status: Needs review » Needs work

@borisson_ Thanks for reviewing!

Ok so needs work

borisson_’s picture

Status: Needs work » Needs review
FileSize
30.56 KB
25.36 KB

I resolved most of the remarks I had from #2. I also created a new service for the .date.inc. I added some basic unit tests for that service as well. This needs tests and more work though.

marthinal’s picture

FileSize
27.92 KB

@borisson_ thanks for you last patch.Looks great.

Working a little bit more on it.

1- I've changed the service name to "DateHandler". is it ok for you?

2- I think the date range regex is not working.

3- Missing unit tests for ::formatTimestamp and ::formatDate. I need to review the best way to test it because we need to use services from these methods (we need to replace format_date() with a service I think) .

borisson_’s picture

@marthinal, can you attach an interdiff? I'll work on tests for the remaining methods and replace the format_date calls.

marthinal’s picture

FileSize
39.46 KB

Oops sorry.

borisson_’s picture

FileSize
9.96 KB

The new service name is fine, let's keep it for now.
I haven't looked at the date regex yet, if I find more time I'll take a look at that.
I added a unit test for formatTimestamp.

I also swapped out the format_date calls to direct calls to the service.

borisson_’s picture

forgot the patch.

marthinal’s picture

FileSize
30.21 KB

Needs work but the links seem to work again.

marthinal’s picture

FileSize
1.32 KB
marthinal’s picture

@borisson_

Working on the query_type finally I needed to build the list of links from the LinksWidget.

So, basically I decided to add the children into the results directly. If a results has children then add it to the item list.

It was different for D7 but I'm trying to simplify and avoid to add parent and children per result.

I'm not sure if this is the best way or maybe I'm missing something... Anyway this is a possible solution for the moment and of course a WIP.

I can change the result later from the query_type in the case that we want to build the item list in a different way.

marthinal’s picture

FileSize
13.78 KB

Oops

Attached interdiff.

This is a related issue #2612078: Facet children

Novitsh’s picture

Related issues: +#2612078: Facet children

Updating metadata.

borisson_’s picture

This needed a reroll. I also fixed some code style things, I'll try to create an integration test.

borisson_’s picture

FileSize
15.16 KB
30.52 KB

Initial pass at that test, I'll work more on it later.

Status: Needs review » Needs work

The last submitted patch, 16: create_a_date_query-2672024-16.patch, failed testing.

The last submitted patch, 16: create_a_date_query-2672024-16.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
8.62 KB
38.99 KB

Status: Needs review » Needs work

The last submitted patch, 19: create_a_date_query-2672024-19.patch, failed testing.

The last submitted patch, 19: create_a_date_query-2672024-19.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
38.99 KB
borisson_’s picture

FileSize
2.45 KB
41.01 KB

More code in the test.

Status: Needs review » Needs work

The last submitted patch, 23: create_a_date_query-2672024-23.patch, failed testing.

The last submitted patch, 23: create_a_date_query-2672024-23.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
7.64 KB
40.91 KB

A couple of CS fixes, still trying to figure out why the failures are happening.

Status: Needs review » Needs work

The last submitted patch, 26: create_a_date_query-2672024-26.patch, failed testing.

The last submitted patch, 26: create_a_date_query-2672024-26.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.56 KB
41.34 KB

A couple more code style fixes, tests will still fail.

Status: Needs review » Needs work

The last submitted patch, 29: create_a_date_query-2672024-29.patch, failed testing.

The last submitted patch, 29: create_a_date_query-2672024-29.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
41.11 KB

This should be a green test. Does this need extra coverage?

marthinal’s picture

I've detected that date facet results for multilingual sites are not correct. I've fixed the problem in my last patch + refactoring Integration tests.

Actually the facets results should be correct but we need to fix this problem for the search results as well.

Basically the problem is that we have the same nid for different nodes with different langcode-created date. I'll try to fix this problem.

@borisson_ if you agree I think we can fix the current issue and open a follow up for other issues that probably are out of the scope of this issue.

marthinal’s picture

And I think we need to verify the facet results for multilingual sites. So enabling language module to avoid problems in the future

  • borisson_ committed eefee65 on 8.x-1.x authored by marthinal
    Issue #2672024 by borisson_, marthinal: Create a date query type for...
borisson_’s picture

Status: Needs review » Fixed

I agree with the above, I've committed this issue. It'd be great if you can open up some follow-ups for remaining tasks.

marthinal’s picture

Created a follow-up. In his case probably should be fixed on core(Node search query).

Status: Fixed » Closed (fixed)

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