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.
We already created a String query type. We should also create one for dates.
Attached is a patch as a first step.
Comment | File | Size | Author |
---|---|---|---|
#33 | 2672024-33.patch | 49.55 KB | marthinal |
date_core_facets.png | 106.73 KB | marthinal |
Comments
Comment #2
borisson_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.
/s/string/date/
/s/array()/[]/
/s/FACETAPI/FACETS/
Is there a reason this is commented out?
An additional newline here should be removed.
An additional newline here should be removed.
/s/FACETAPI/FACETS/
The
else {
should go on a new line./s/FACETAPI/FACETS/
Good comment! ++
this should either be an
!is_null($previous)
, or change the condition around, the drupal coding standards don't allow Yoda if.This can be converted to a service, so we can unit test as well.
/s/FACETAPI/FACETS/
We can change this to
Converts dates from unix timestamps to ISO 8601 format.
Need type hints.
/s/Return/Returns/
I really dislike a
switch (TRUE) {
statement, I guess a bunch of if statements isn't much better though.see earlier comment about
is_null
The first line of the comment should be a single line.
let's add typehints here as well.
Comment #3
marthinal CreditAttribution: marthinal commented@borisson_ Thanks for reviewing!
Ok so needs work
Comment #4
borisson_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.Comment #5
marthinal CreditAttribution: marthinal commented@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) .
Comment #6
borisson_@marthinal, can you attach an interdiff? I'll work on tests for the remaining methods and replace the format_date calls.
Comment #7
marthinal CreditAttribution: marthinal commentedOops sorry.
Comment #8
borisson_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.Comment #9
borisson_forgot the patch.
Comment #10
marthinal CreditAttribution: marthinal commentedNeeds work but the links seem to work again.
Comment #11
marthinal CreditAttribution: marthinal commentedComment #12
marthinal CreditAttribution: marthinal commented@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.
Comment #13
marthinal CreditAttribution: marthinal commentedOops
Attached interdiff.
This is a related issue #2612078: Facet children
Comment #14
Novitsh CreditAttribution: Novitsh at Colruyt Group Services commentedUpdating metadata.
Comment #15
borisson_This needed a reroll. I also fixed some code style things, I'll try to create an integration test.
Comment #16
borisson_Initial pass at that test, I'll work more on it later.
Comment #19
borisson_Comment #22
borisson_Comment #23
borisson_More code in the test.
Comment #26
borisson_A couple of CS fixes, still trying to figure out why the failures are happening.
Comment #29
borisson_A couple more code style fixes, tests will still fail.
Comment #32
borisson_This should be a green test. Does this need extra coverage?
Comment #33
marthinal CreditAttribution: marthinal commentedI'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.
Comment #34
marthinal CreditAttribution: marthinal commentedAnd I think we need to verify the facet results for multilingual sites. So enabling language module to avoid problems in the future
Comment #36
borisson_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.
Comment #37
marthinal CreditAttribution: marthinal commentedCreated a follow-up. In his case probably should be fixed on core(Node search query).