#272 | Screenshot 2023-03-03 at 17.22.23.png | 26.81 KB | robertoperuzzo |
#270 | interdiff-2648950_269_270.txt | 5.88 KB | joegraduate |
#270 | 2648950-270.patch | 63.42 KB | joegraduate |
|
#269 | 2648950-265_1.patch | 68.49 KB | leo liao |
|
#266 | 2648950-265.patch | 67.96 KB | romain.sickenberg |
|
#266 | interdiff_257_265.txt | 615 bytes | romain.sickenberg |
#262 | Exposed date.png | 14.21 KB | gease |
#257 | 2648950-257.patch | 67.87 KB | Rob230 |
|
#256 | After.png | 187.52 KB | matsbla |
#256 | Before.png | 263.68 KB | matsbla |
#253 | interdiff.txt | 11.11 KB | KapilV |
#253 | 2648950-253.patch | 63.27 KB | KapilV |
|
#249 | Screenshot 2021-04-29 at 5.44.39 PM.png | 115.34 KB | BhumikaVarshney |
#247 | 2648950-247-9.1.x.patch | 63.65 KB | acbramley |
|
#246 | 2648950-246.9_1_x.patch | 64.07 KB | douggreen |
|
#245 | 2648950-245.patch | 63.65 KB | Spokje |
|
#245 | raw_diff_241_245.txt | 1.93 KB | Spokje |
#242 | interdiff_239_241.txt | 789 bytes | Spokje |
#241 | interdiff_239_241.patch | 789 bytes | Spokje |
|
#241 | 2648950-241.patch | 63.82 KB | Spokje |
|
#239 | 2648950-239.patch | 63.81 KB | larowlan |
|
#237 | 2648950-236-interdiff.txt | 20.37 KB | Berdir |
#236 | 2648950-236.patch | 75.75 KB | Berdir |
|
#236 | 2648950-236.patch | 75.75 KB | Berdir |
|
#234 | interdiff_231-234.txt | 590 bytes | ravi.shankar |
#234 | 2648950-234_drupal8_9_x.patch | 70.14 KB | ravi.shankar |
|
#231 | interdiff-229-231.txt | 1.31 KB | akalata |
#231 | 2648950-231.8_9_x.patch | 61.02 KB | akalata |
|
#230 | 2648950-229.8_9_x.patch | 60.85 KB | akalata |
|
#224 | 2648950.219_224.interdiff.txt | 3.71 KB | dww |
#224 | 2648950-224.changes-for-2625136.DO-NOT-TEST.patch | 1.98 KB | dww |
|
#224 | 2648950-224.8_8_x.patch | 59.5 KB | dww |
|
#224 | 2648950-224.8_7_x.patch | 57.32 KB | dww |
|
#219 | 2648950.218_219.interdiff.txt | 5.25 KB | dww |
#219 | 2648950-219.changes-for-2625136.DO-NOT-TEST.patch | 1.98 KB | dww |
|
#219 | 2648950-219.8_7_x.patch | 57.05 KB | dww |
|
#219 | 2648950-219.8_8_x.patch | 59.23 KB | dww |
|
#218 | 2648950.217_218.interdiff.txt | 2.34 KB | dww |
#218 | 2648950-218.8_8_x.patch | 58.94 KB | dww |
|
#217 | 2648950-217.changes-for-2625136.DO-NOT-TEST.patch | 4.09 KB | dww |
|
#217 | 2648950.216_217.interdiff.txt | 4.61 KB | dww |
#217 | 2648950-217.newer-logic.8_8_x.patch | 58.98 KB | dww |
|
#217 | 2648950-217.newer-logic.8_7_x.patch | 56.76 KB | dww |
|
#216 | 2648950.216-prev-new.interdiff.txt | 3.34 KB | dww |
#216 | 2648950.210-216-new-logic.interdiff.txt | 7.66 KB | dww |
#216 | 2648950.210-216-previous-logic.interdiff.txt | 6.92 KB | dww |
#216 | 2648950-216.changes-for-2625136-new-logic.DO-NOT-TEST.patch | 1.25 KB | dww |
|
#216 | 2648950-216.changes-for-2625136-previous-logic.DO-NOT-TEST.patch | 4.23 KB | dww |
|
#216 | 2648950-216.fix-test-assertions-only.DO-NOT-TEST.patch | 2.44 KB | dww |
|
#216 | 2648950-216.new-logic-fix-test-assertions.8_8_x.patch | 58.55 KB | dww |
|
#216 | 2648950-216.new-logic.8_7_x.patch | 56.33 KB | dww |
|
#216 | 2648950-216.previous-logic.8_7_x.DO-NOT-TEST.patch | 56.51 KB | dww |
|
#210 | interdiff-2648950-193-210.txt | 3.24 KB | Spokje |
#210 | 2648950-210.patch | 54.98 KB | Spokje |
|
#208 | 2648950-208.patch | 22.69 KB | olivier.br |
|
#205 | 2648950-205.patch | 55.09 KB | cameron prince |
|
#193 | 2648950-193.patch | 54.95 KB | Berdir |
|
#182 | Screenshot from 2018-11-06 11-33-15.png | 23.24 KB | alexfarr |
#181 | 2648950-181.patch | 54.82 KB | jofitz |
|
#181 | interdiff-2648950-177-181.txt | 1.94 KB | jofitz |
#178 | Screenshot from 2018-10-11 04-16-34.png | 17.7 KB | matsbla |
#178 | Screenshot from 2018-10-11 04-12-03.png | 50.47 KB | matsbla |
#177 | interdiff-166-177.txt | 17.06 KB | sukanya.ramakrishnan |
#177 | interdiff-172-177.txt | 6.91 KB | sukanya.ramakrishnan |
#177 | 2648950-177.patch | 55.45 KB | sukanya.ramakrishnan |
|
#176 | Screen Shot 2018-10-05 at 11.44.08 AM.png | 34.91 KB | sukanya.ramakrishnan |
#172 | 2648950-172.patch | 48.54 KB | sukanya.ramakrishnan |
|
#172 | interdiff-166-172.txt | 18.96 KB | sukanya.ramakrishnan |
#166 | interdiff-2648950-155-166.txt | 12.16 KB | GoZ |
#166 | 2648950-166.patch | 37.65 KB | GoZ |
|
#161 | datetime-between-filters-label.png | 41.14 KB | GoZ |
#160 | datetime-between-filters.png | 33.06 KB | GoZ |
#156 | 2648950-155.patch | 25.08 KB | jhedstrom |
|
#156 | interdiff-2648950-150-155.txt | 1.51 KB | jhedstrom |
#154 | Screen Shot 2018-06-22 at 2.55.58 PM.png | 65.49 KB | jhedstrom |
#150 | interdiff-2648950-143-150.txt | 33.54 KB | stella |
#150 | 2648950-150-8.6.x.patch | 24.99 KB | stella |
|
#146 | interdiff-2648950-144-145.patch | 17.59 KB | stella |
|
#146 | 2648950-145-8.6.x.patch | 0 bytes | stella |
|
#144 | interdiff-2648950-142-143.txt | 1.15 KB | mmbk |
#144 | 2648950-143.patch | 16.71 KB | mmbk |
|
#142 | 2648950-142.patch | 16.52 KB | mmbk |
|
#141 | Selection_275.png | 83.42 KB | Berdir |
#141 | Selection_274.png | 9.39 KB | Berdir |
#140 | Screenshot from 2018-03-29 06-32-11.png | 4.4 KB | matsbla |
#139 | 2648950-78-ab.patch | 26.43 KB | alanburke |
|
#134 | use_form_element_of-2648950-134-8.4.x.patch | 24.99 KB | jibran |
|
#134 | use_form_element_of-2648950-134.patch | 24.96 KB | jibran |
|
#133 | use_form_element_of-2648950-131-8.5.x.patch | 25.6 KB | stella |
|
#132 | interdiff-2648950-118-131.txt | 2.05 KB | stella |
#132 | use_form_element_of-2648950-131.patch | 10.79 KB | stella |
|
#124 | date-exposed-inline-stairs.png | 84.74 KB | tacituseu |
#121 | use_form_element_of-2648950-120-8.3.2.patch | 25.59 KB | pasan.gamage |
|
#120 | use_form_element_of-2648950-120-8.3.patch | 25.59 KB | pasan.gamage |
|
#118 | interdiff-104-118.txt | 1.7 MB | gambry |
#118 | use_form_element_of-2648950-118.patch | 25.57 KB | gambry |
|
#108 | 2648950-108.patch | 17.04 KB | wtrv |
|
#107 | 2648950-71.patch | 17.04 KB | wtrv |
|
#104 | use_form_element_of-2648950-104.patch | 17.41 KB | gambry |
|
#104 | interdiff-103-104.txt | 2.14 KB | gambry |
#104 | use_form_element_of-2648950-104.patch | 17.41 KB | gambry |
|
#104 | interdiff-103-104.txt | 2.14 KB | gambry |
#103 | use_form_element_of-2648950-103.patch | 15.89 KB | gambry |
|
#103 | interdiff-100-103.txt | 1.26 KB | gambry |
#100 | 2648950-100.patch | 15.06 KB | jofitz |
|
#100 | interdiff-98-100.txt | 971 bytes | jofitz |
#98 | 2648950-98.patch | 15.06 KB | jofitz |
|
#96 | add_date_picker_to_date_field_filter_in_view-2648950-95.patch | 709 bytes | nassaz |
|
#85 | 2648950-85.patch | 15.07 KB | mpdonadio |
|
#80 | 2648950-79-8.2.x.patch | 25.11 KB | jefuri |
|
#78 | 2648950-78.patch | 26.39 KB | jefuri |
|
#75 | 2648950-73.patch | 17.06 KB | Berdir |
|
#71 | interdiff.txt | 2.12 KB | dawehner |
#71 | 2648950-71.patch | 17.08 KB | dawehner |
|
#65 | 2648950-65.patch | 15.92 KB | dawehner |
|
#55 | interdiff-2648950-52-55.txt | 537 bytes | tlyngej |
#55 | 2648950-55.patch | 16.41 KB | tlyngej |
|
#52 | 2648950-52.patch | 16.14 KB | tlyngej |
|
#50 | interdiff-42-48.txt | 3.07 KB | mpdonadio |
#48 | 2648950-48.patch | 16.85 KB | tregismoreira |
|
#48 | Screenshot at Aug 20 12-46-16.png | 24.7 KB | tregismoreira |
#42 | interdiff-39-42.txt | 8.41 KB | mpdonadio |
#42 | 2648950-42.patch | 15.63 KB | mpdonadio |
|
#39 | interdiff-38-39.txt | 1004 bytes | mpdonadio |
#39 | 2648950-39.patch | 11.07 KB | mpdonadio |
|
#38 | interdiff-32-38.txt | 9.62 KB | mpdonadio |
#38 | 2648950-38.patch | 10.35 KB | mpdonadio |
|
#34 | Screen Shot 2016-06-10 at 13.36.02.png | 17.27 KB | yannickoo |
#34 | Screen Shot 2016-06-10 at 13.36.19.png | 12.6 KB | yannickoo |
#32 | interdiff-27-32.txt | 2.02 KB | mpdonadio |
#32 | 2648950-32.patch | 5.07 KB | mpdonadio |
|
#27 | interdiff-27.txt | 8.57 KB | kgoel |
#27 | 2648950-27.patch | 4.97 KB | kgoel |
|
#22 | interdiff.txt | 3.28 KB | kgoel |
#22 | 2648950-22.patch | 5.41 KB | kgoel |
|
#18 | interdiff-13-18.txt | 1.81 KB | ztl8702 |
#18 | 2648950-18.patch | 2.13 KB | ztl8702 |
|
#15 | interdiff-04-13.txt | 1.46 KB | mpdonadio |
#13 | 2648950-13.patch | 2 KB | skyredwang |
|
#6 | Screen Shot 2016-02-01 at 01.19.15.png | 49.64 KB | yannickoo |
#4 | use_form_element_of-2648950-4.patch | 1.65 KB | mpdonadio |
|
Comments
Comment #2
yannickooComment #3
mpdonadioGood idea, but going to have to do this as part of 8.1, so adjusting.
Comment #4
mpdonadioFirst pass. Needs test to prevent regression. Need to think about time support when it is a date+time field? Also needs polyfill support ala #1835016: Polyfill date input type.
Comment #5
mpdonadioComment #6
yannickooReally cool!
I would really like the time support, sounds great :)
Comment #8
skyredwangI tested patch in #4 against fresh install of D8.1-beta1. I couldn't get the date widget to show up even after cache rebuild. Something changed?
Comment #9
mpdonadio#8 what browser? Chrome will use it builtin date input. Firefox uses the polyfill from the jQuery UI.
Comment #10
skyredwangMy bad, I didn't realize the widget was "only" available on exposed filter. How about we make the widget is also available even the filter is not exposed?
Comment #11
skyredwangAlso, if the operator is "between", then we will need two widgets on two input box.
Comment #12
13jupiters CreditAttribution: 13jupiters commentedOut of curiousity I spliced this back into a 8.0.5 install, and it worked well when the exposed date filter operator was a simple =, <, > etc. However, as pointed out in #11, the "between" operator requires two widgets, and this patch wiped out the second widget. (Not sure if anything has changed in 1.-beta or 2.-beta that would fix this.)
Comment #13
skyredwangI added a case handling for "between" operator.
Comment #14
gnugetCan you please provide an interdiff between #13 and #4?
Also this is going to need tests.
Comment #15
mpdonadioIssue already has the Needs tests tag. If everyone is OK w/ manual testing, then we can add some integration tests. But I wouldn't be opposed to expanding the issue to handle date inputs for all filter values, and not just exposed filters.
Comment #16
13jupiters CreditAttribution: 13jupiters commentedManually testing #13, it worked perfectly for exposed date filter with between operator - EXCEPT if the Filter identifier has been changed from the default value (which is the full field name of the date field being filtered). Can't recall if this was also the case with D7 date filtering...
If the Filter Identifier is changed during configuration of the exposed filter, e.g. from field_mydatefieldname_value to something user-friendly (like "date"), two pairs of min/max inputs appear, one pair using the widget, the other using plain text input.
Comment #17
ztl8702 CreditAttribution: ztl8702 as a volunteer commentedDo we also need to deal with the 'not between' operator? (Although this might not be used as frequently as 'between')
Comment #18
ztl8702 CreditAttribution: ztl8702 as a volunteer commentedChanged
realField
tooptions['expose']['identifier']
.Should fix the issue on #16.
Comment #19
ztl8702 CreditAttribution: ztl8702 as a volunteer commentedComment #20
13jupiters CreditAttribution: 13jupiters commentedTested #18 on 8.1.0 with good results - the field appears as type text-date with date selector, and observes the alias field name. Fixes issue #16
EDIT: to remove reference to an issue that turned out to be unrelated (cache causes trickiness with hook form alter, but this patch seems unrelated).
Comment #21
mpdonadioStill need test coverage.
Comment #22
kgoel CreditAttribution: kgoel commentedI have added test for core/modules/datetime/src/Plugin/views/filter/Date.php class. While I was working on writing test, I noticed error for missing schema for field_date_value.
I have created issue https://www.drupal.org/node/2721339 to fix ^
Working on writing test for core/modules/views/src/Plugin/views/filter/Date.php
Comment #24
gnugetThis method needs documentation.
Comment #25
jhedstromThis second implementation shouldn't be needed, as the
\Drupal\datetime\Plugin\views\filter\Date
class extends the views numeric Date class, where this method is also being added.This means the test can be moved to the views module I think.
Comment #26
mpdonadio@kgoel, if you are still working on this in addition to @jhedstrom's comment:
I think a ->assertFieldByXPath() would be better here. Something like
but I forget exactly what the name looks like here.
Can you still make forward progress on this, or are you stuck b/c #2721339: Missing schema for field_date_value?
Comment #27
kgoel CreditAttribution: kgoel commentedAddressed #25 and #26
Comment #28
jibranNice work! here is some feedback.
This is not a correct coding standard.
Can we please use short array syntax here?
This is not a lower camel case format.
Why can't we
or
here instead?
We don't need this.
We can use
$this->cssSelect()
and check type here.Can we use
$this->cssSelect()
here?Can we delete these comments?
Comment #29
mpdonadioThanks everyone!
#28-3, that is actually consistent with the rest of the file. I have no clue how or why it ended up like that.
#28-7, we should check both the ID (to make sure we have the proper element) and the type (to make sure it is actually a date input), so
?
Do we still need more coverage, or do we just need to polish this up?
Comment #30
jibranMaybe submit the filter values and assert the results as well just to make sure everything is working properly after the change.
Comment #31
mpdonadioGoing to try to wrap this one up in the next few days.
Comment #32
mpdonadio#28-1, OK
#28-2, OK, but rest of class uses array() syntax.
#28-3, Sadly, this matches the use in this class, so I left it as is. Read it applied. Fixing would be OOS, and one of the man date related followups / cleanups that are needed.
#28-4, Views still just gives you a render array, so we need to render the output. Added some comments.
#28-5, OK
#28-6, removed
#28-7, I made this more specific. I find the xpath easier to read than the css version when you check for both the name and the type.
#28-5, OK
Comment #33
mpdonadioComment #34
yannickooSo nice to see the progress here! I've uploaded two screenshots of the form elements:
Simple form element
Between operator
I would like to change the status to RTBC because the code and the result look fine and there is also a test in but it seems like we should support setting the time optionally otherwise this could be a regression because people are not able to enter times.
Comment #35
jibran#30 is still pending.
Comment #37
mpdonadioOK, couple things.
Patch in #32 now fails b/c of a schema problem. I am not sure which entry in the YAML is missing. I imported/saved/exported the view, and still have the problem. We may just need to recreate and export it (see below).
Pretty sure that #31 shows that we do have a problem with the time not being there. Need to look into that.
The test request in #30 means we need to change that test view to add a page display, and do a real integration test (so the values go through the form fields) rather than the "fake" one where we just grab and render the view.
Comment #38
mpdonadioShould address all outstanding feedback.
This is going to fail because of a notice. I see what is happening, but not why or how this patch is causing the problem.
Comment #39
mpdonadioActually, I think this is because the empty exposed form doesn't have a valid date/time in it, the exposed form isn't getting a valid datetime object. This silences the notice.
Comment #41
gnugetHi mpdonadio.
I gave a quick review to your last patch and I found some nits:
This needs a docblock.
This line must finish with a dot.
Missing dot.
Great work! thanks.
Comment #42
mpdonadioFB from #41 addressed.
Added coverage of the between path.
Little cleanup here and there.
This may need more more manual testing to see if anything got missed.
Comment #43
jhedstromI think the patch in #42 has addressed all the outstanding feedback. This is a great enhancement compared to the old input where a user must know the exact format to enter (and #2749483: Datetime views filter plugin should allow granularity to be configurable will make this even better).
Comment #44
jhedstromSetting back to NR since I discovered this bit of code:
is being changed in a slightly different way over in #2369119: Fatal error when trying to save a View with grouped filters using other than string values.
Comment #45
mpdonadioNice catch. I search for a while, but still missed that one. I am tempted to postpone this. I think the other issue is fixing the real root cause, and that my conclusion was incorrect.
Comment #46
mpdonadioI merged the element change + these tests with the patch in #2369119: Fatal error when trying to save a View with grouped filters using other than string values and I still get the "type" notice. I'm beginning to this these are two problems with the same blip of code in acceptExposedInput().
Comment #48
tregismoreira CreditAttribution: tregismoreira commentedHey guys, great work! @mpdonadio thanks for the patch ;)
I've improved a bit, adding an options to choose widget instead of set
datetime
as default.Comment #50
mpdonadio@tregismoreira thanks for helping out with the patch. Two things. When posting a new patch, it help to also post an interdiff so we can see what had changes from the previous patch. I attached one. This new option will require test coverage.
Also, what happens if it is a datetime field configured for date+time, and the user chooses date-only? That path needs to be explicitly tested.
Comment #51
NetNerdy CreditAttribution: NetNerdy commentedpatch #39 => test failed
i can select a date but don't show the content, the selected date isn't a correctly value (or so)
localhost Drupal 8.1.8, php 7
Comment #52
tlyngej CreditAttribution: tlyngej at Annertech commentedRe-rolled to match latest 8.2.x and to match patch from https://www.drupal.org/node/2369119#comment-11575307, mentioned in #44
It will fail as https://www.drupal.org/node/2369119 haven't been committed yet.
Comment #53
mpdonadioComment #55
tlyngej CreditAttribution: tlyngej at Annertech commentedAdded a check to see if an array key exists before trying to read the value of it.
Comment #56
daffie CreditAttribution: daffie commentedFor the testbot.
Comment #58
mpdonadioI think we need to postpone this on #2369119: Fatal error when trying to save a View with grouped filters using other than string values.
Also, the patch in #48 added a feature that needs explicit test coverage, and the questions from #50 need to discussed. Otherwise, we should go back to #42 as our starting point when the other patch lands.
Comment #59
gambryI would rather revert to #42 and discuss the needed of a widget switcher in a follow up (which personally I don't feel we do).
But the #42 by itself force the datetime widget (date + time input items) even if the field datetime_type is date-only. Why not checking the field datetime_type and setting the form
#type
accordingly?Comment #60
tlyngej CreditAttribution: tlyngej at Annertech commented@gambry
I think that we can set the default value to what ever the field storage is set to. But I think that we need to keep the option, for the user to choose whether they want it or not.
Comment #61
gambry@tlyngej the option seems useless for date-only field, am I right?
In this case the code needs to be updated in order to show the options only if the field date type is datetime.
Additionally as mentioned in #58 we need explicit test coverage and the answer to #50.
I personally see too much effort for a really small advantage. The only scenario is in fact a user choosing a datetime field type but exposing only the date part.
Comment #62
tlyngej CreditAttribution: tlyngej at Annertech commented@gambry
For fields, where the date is the only option, yes, giving the user the option to chose to display time as well would be pointless.
There is a use case for displaying only the date, on exposed Views filters, for a field that can contain both dates and times? But whether it would be "to much effort" to implement it, I don't know.
Comment #63
mpdonadioI think we are at the point whether we are discussing scope creep or not not this issue.
Adding the option, would probably mean changing the way they query work on date+time fields, which is not what this was originally about (just making this exposed filter use the HTML5 inputs). That is not to say it isn't a bad idea, but I think we should handle that as a followup once this get blocked, and we can get this in.
Regardless, we have to wait on #2369119: Fatal error when trying to save a View with grouped filters using other than string values to see if we will have to adjust this at all.
Comment #64
mpdonadioThis is postponed, but we need an IS update to match the solution we are going with. I still want to do #42 initially, and then handle improvements as a f/up.
Comment #65
dawehnerHere is a new version of the patch as I needed it for a client.
Comment #66
adriancidI made some test and it works
Comment #68
mpdonadioThis is still postponed on #2369119: Fatal error when trying to save a View with grouped filters using other than string values, as this patch has a similar, but incomplete fix for that.
Also, the new options introduced in #48 which are also in #65 do not have test coverage.
Comment #69
Yekaterina CreditAttribution: Yekaterina commented@dawehner, thanks, works on d.8.2.5.
Is there way to use the exposed filter only by year?
Comment #71
dawehnerI had some notices with this patch recently.
Comment #72
jlbellidoI've tested it and it works like a charm!
Thanks @Dawehner for your awesome work!
Comment #73
dkre CreditAttribution: dkre commented#71 Works but needs an array check in FilterPluginBase:
From the patch:
Addition:
Sorry I can't provide a patch.
Thanks @dawehner!
#69
I don't believe this sort of functionality will be available in core or available for a while. The best way to implement this would probably through the front-end.
Eg, present a select box to the user (2017, 2018 etc) which passes full dates to views (01012017T1200) or whatever the format is.
Comment #74
mpdonadioOK, #2369119: Fatal error when trying to save a View with grouped filters using other than string values is in, so we can unpostpone this. I suspect any patch here needs to be rerolled anyway.
The question is, simple approach in #42 or extra functionality that #65 provides.
I am leaning towards #42, with exploring #65 as a f/up (my fear is getting bogged down in edge cases with that).
Comment #75
BerdirHere's a reroll of #71 for now, haven't manually tested it.
Is the needs tests tag still needed? I see that there are tests.
Also not sure which part is the one that should no longer be needed :)
Comment #76
mpdonadioThis is essentially the hunk we are talking about. Not all of the patches have interdiffs, so a little hard to look back in time here).
The patch adds test coverage for the text=>date input change, but that was to ensure that the changes up to #42 was covered. The feature added after that in #48) doesn't have test coverage. In particular, does the correct thing happen when you choose date-only and apply it to something with date+time (like created/changed timestamps).
I haven't played with this patch applied to a live site in a while, though.
Comment #78
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedAdded the addition code from #73. Because this patch breaks other non-array values from the exposed filter.
Comment #79
mpdonadio#78, can you post an interdiff? The patch went from 17k to 26k.
Comment #80
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedAlso created a patch for the current 8.2.x branche of drupal core.
Comment #83
mpdonadio#78 and #80 have a lot of out of scope formatting changes.
Partial review of #75. Still need to manually review patch applied.
Needs to be updated to use short array syntax.
This logic is not correct for switching between date+time and date-only. The #type should be always be 'datetime' and then '#date_date_element' and '#date_time_element' need to be set appropriately. May also need to adjust the other options, too.
See the docs for \Drupal\Core\Datetime\Element\Datetime, and the DateTimeDefaultWidget for an example of how this form element gets used.
The date-only path also needs test coverage.
Comment #84
mpdonadioOK, I spent some time with this, and this is where we stand:
In #48+ where we add the widget type, we introduce some logic problems. A date on its own is ambiguous for timestamps, and date+time. It will work for date-only. So, for timestamps and date+time, we need to convert the queries to either just do date-portion comparison or >= 00:00:00 and <= 23:59:59 logic and the equivalents for all of the different filter options. This is going to be a mess, especially the test coverage for all of those cases.
In #42 we need to add the check to only apply the form element change if the 'type' === 'date', and update the tests to make sure the proper form element is used for 'date' and 'offset' (ie, 42 introduced a regression and should have failed).
I think we need to roll back to the approach in #42, fix that, and then consider the widget choice as a followup because of the complexity. We should also add some explicit test coverage for the datetime flavors (date+time and date-only) to make sure they don't regress. For datetime w/ type date-only we should still use the datetime form element, but in date-only mode.
I'll update the IS tomorrow with the approach and todo-list.
Comment #85
mpdonadioHere is a starting point. Essentially #42 w/o the hunk to acceptExposedInput(), the 'type' check, and some renaming to account for other changes. This is untested. The view may need to exported again so the schema matches the changes in #2369119: Fatal error when trying to save a View with grouped filters using other than string values.
Will update IS tomorrow.
Comment #87
LendudeThe View needs to be exported again because of #2459289: Boolean default values are not saved, that should clear up some fails.
Comment #88
flyke CreditAttribution: flyke commentedCould someone answer the question from @Yekaterina ?
Is there way to use the exposed filter only by year?
I would like to know this too. I have a content type 'blog item' with a date field and I would like to create an exposed filter to filter all blog items based on a selected year from a selectbox.
Comment #89
mpdonadio#88, that is not the scope of this issue. This issue is just to change the HTML element used to pick the date/time data for the filter/Date plugin.
Comment #90
dubcanada CreditAttribution: dubcanada commentedWe should also add time to the time when it is available. Not sure if their is a drupal core polyfill?
Comment #91
mpdonadio#90, we have an issue for that, #1838234: Add jQuery Timepicker for the Time element of the datetime field.
Comment #92
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commentedHi all !
Regardless of which patch I use, I can't get it working with entity basefield created via :
$fields['start_date'] = BaseFieldDefinition::create('datetime')
You can find an example of such a field in "Promotion" module of "Commerce 2.x".
When I expose a datetime field filter, even using patches from this issue, it is still a string field.
If I have
dsm(dsm($this->getPluginDefinition());)
in FilterPluginBase.php at buildExposedForm() method, it gives me:So datetime exposed filter is of time StringFilter and is thus not impacted by Date.php buildExposedForm() such as changed by patches here.
Comment #93
mpdonadio#92, this patch doesn't change the filter, just the form element that gets rendered out. I think you are getting hit by the basefields-can't-use-hook_field_views_data views issue, but I can't find that link right now.
Comment #94
Dom. CreditAttribution: Dom. as a volunteer and at ACINO commented@mpdonadio: thanks to point that out: I guess you are talking about this ?
#2489476: [PP-1] Base fields miss out on Views data from hook_field_views_data()
Comment #95
nassazI think this work actually, to change simple input by datepicker input, this is useful for users to choose the right format.
Comment #96
nassazComment #97
mpdonadio#94, yeah that is the issue I was thinking of.
#96, thanks for the patch. Our current work in progress is the patch in #85 which needs to be re-rolled. That particular patch contains additional nuances and test coverage in addition to just switching out the form element.
Comment #98
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll based on #85.
Comment #100
jofitz CreditAttribution: jofitz at ComputerMinds commentedResolve one of the test fails.
Comment #102
gambryThe reasons for the 2 fails are #2811887: Exposed date filter leads to a notice and #2369119: Fatal error when trying to save a View with grouped filters using other than string values as they both make use of created and I guess replacing the field with the date type widget makes things weird.
Comment #103
gambryThis patch resolve one of the test fails from #100, as now "created" exposed input is replaced with "created[date]" and "created[time]".
The other failure is due a deeper issue I'll explain on a separated comment/patch.
Comment #104
gambryTest on
core/modules/views/tests/src/Functional/Handler/FilterDateTest.php:262
seems to fail due the default value of the 'created' filter not been processed during visiting the view page.The value is in both the form_state input and the plugin
$this->value
array, but my understanding is as it isn't structured like the form element - is a simple string instead of the 'date' and 'time' sub-elements - the form/view builder skip it.I couldn't find a better solution than checking if the element form_state input is structured as the form element, and if not - and there is a default value - then the code splits the date and time parts and replace the form_state input string to an array.
@mpdonadio or @jhedstrom (or whoever can answer) Is that supposed to happen? Is the Date view filter - and
Date::buildExposedForm()
method - the right place where to fix this?Comment #105
mpdonadio#104, that's odd, I would have thought that just using '#default_value' would work.
Comment #106
gambry#105 it doesn't. Beside the documentation suggests to use a DrupalDateTime object in here, while the current value is a datetime string.
UPDATE: I'm not sure what happened with the files upload on #104 so I'm removing the duplicated interdiff/patch.
Comment #107
wtrv CreditAttribution: wtrv at Wunder commentedAdjusted the patch from #71 to support the short array syntax found in 8.3.2
Comment #108
wtrv CreditAttribution: wtrv at Wunder commentedResubmitted #107 with fixed patch name.
Comment #109
gambry@wtrv please provide interdiffs together with patches, in order to facilitate the review process.
However I don't quite understand the additional values brought by your patches. Patches from #43 to #84 contain additional features we won't cover on this issue, but in a follow up.
Current work is #103 (1 test fails) or #104 (needs investigation).
Comment #110
gambryUpdating the IS with guidelines from #84 and remaining tasks.
Comment #113
jackenmailRetested the #109(2648950-108.patch)
Comment #114
mpdonadioAgree w/ the updated IS and current scope. The patch in #104 is our current work in progress. No need to update or reroll others, and this issue need to be against 8.4.x as it is a feature request.
Comment #115
mpdonadioOK, about the #default_value:
So, I think that means our method is OK.
Comment #116
gambryThat's a relief.
However I'm not 100% happy with this redundant code:
Which appears 3 times. I tried my best to de-dupe, without much luck. I'll try again at least to de-dupe the conditional part, but suggestions are welcome.
While doing it I'll try to investigate more playing around with default values, in order to close "Investigate why the default value of the exposed filter is not picked up by the form (see #104)." remaining task.
The only outstanding task will then be "Hide time element for date-only type fields", what would be the best way to get this field info? The only idea I have is load the config.factory service and to pull the field storage configuration, but it looks like a bazooka shooting a fly. Thoughts?
Comment #117
wtrv CreditAttribution: wtrv at Wunder commented@gambry I understand current work is being done on #103 and #104. I merely provided an updated version of the patch provided by @dawhener so it would still work against 8.3.x.
There was a need for this on one of the projects I am working on and I thought it could be helpful for other people as a temporary solution.
I do however agree that all future efforts should target 8.4.x
Comment #118
gambryAttached the patch hiding the
time
form element component when the field is aDateTimeItem::DATETIME_TYPE_DATE
('date' only) type. Below few thoughts while I was working on it:Drupal\views\Plugin\views\filter\Date()
which handles any date filter and not only datetime ones. Applying the remaining task Hide time element for date-only type fields here would have added a wrong dependency to datetime module, as we don't have date-only type in core without datetime module AFAIK. That's why I moved this bit of logic insideDrupal\datetime\Plugin\views\filter\Date()
datetime pluginDrupal\Tests\datetime\Functional\Handler\FilterDateTest
code can be place in a *Base abstract class if you think future tests can benefit from it.Also:
Comment #119
gambryI forgot to mention this bit is for the
FieldAPIHandlerTrait
onDrupal\datetime\Plugin\views\filter\Date()
. This change has been already suggested on #2627512: Datetime Views plugins don't support timezones. One of the issues will get a conflict due these lines as soon as the other is committed.Comment #120
pasan.gamage CreditAttribution: pasan.gamage at PreviousNext commentedHere is the patch for 8.3.x
Comment #121
pasan.gamage CreditAttribution: pasan.gamage at PreviousNext commented8.3.2 patch
Comment #123
mpdonadio@pasan.gamage, thanks for the patches, but this issue has to go against 8.4.x because it is a new feature. It is unlikely that this will get a 8.3.x backport.
Comment #124
tacituseu CreditAttribution: tacituseu commentedTested #118 with fresh 8.4.x, the only problem I've encountered is lack of proper wrapper around the exposed datetime widgets producing 'stairs' on exposed form.
All other widgets are inside:
<div class="js-form-item form-item ...">...</div>
which makes them behave properly inside
form--inline
container.Not sure this is proper issue for this, might be an issue with datetime element itself, had no luck with finding existing one.
Comment #125
tacituseu CreditAttribution: tacituseu commentedDigging in a bit most core
FormElement
's have in::getInfo()
:'datetime'
instead does:and the
datetime-wrapper.html.twig
doesn't add any container to compensate.Comment #126
skyredwangBased on #125, I dug a little bit more, I found:
@FormElement("date")
use'#theme_wrappers' => ['form_element'],
While
@FormElement("datetime")
use'#theme_wrappers' => ['datetime_wrapper'],
If would be nice if there are some comments about why date and datetime use two different templates.
On the other hand, I tried to add a wrapper to /core/themes/classy/templates/form/datetime-wrapper.html.twig (there are 3 templates named datetime-wrapper.html.twig, admin theme uses the one from classy) like below, even though they lined up, but there is still some inconsistency in margins.
@mpdonadio has commits history on datetime-wrapper.html.twig, so I guess he would know more about the goal of the template, and where else this template is being used.
Comment #127
mpdonadio#126, we can't change the markup in those templates w/o maintainer approval (think it is still @lauriii).
I believe that template has been around since #1963476: datetime.module - Convert theme_ functions to Twig. I am not sure anyone really remembers why those are built the way they are.
That template is used for the DateTime form element, which is different from the Date form element. Date is a simple element; just a single input. DateTime is a more complex one; it can be date, time, or both, and has invisible elements in it for accessibility.
I think the best thing to do is use an approach similar to #1918994: Improve Datetime and Daterange Widget accessibility, but keep the datetime-wrapper.html.twig as is and update the render array around it to do the right thing (eg, wrap it in something else).
Comment #129
tacituseu CreditAttribution: tacituseu commentedIt also doesn't work when 'Expose operator' is checked, the textfields are still showing up (which ones depends on default operator selected).
Comment #130
tacituseu CreditAttribution: tacituseu commentedComment #131
stella CreditAttribution: stella at Annertech commentedOnce you enable a pager on a view with an exposed date filter, with the above patch applied, the pager links don't work as expected.
Say I have a view with a date field (exposed filter machine name is "start"), and I submit the exposed filters with a date set, then the url is like this:
http://www.example.com/events?start%5Bdate%5D=2017-05-01
However if I then click on the pager to get the next page of results, the url is like this:
http://www.example.com/events?start=2017-05-01%2012%3A54%3A47%20Europe/Dublin&page=1
As a result, no nodes are returned after you click on the pager, as the "start" exposed date filter value is in the wrong format.
This is with a date only field by the way, but the same thing happens with a date and time field.
Comment #132
stella CreditAttribution: stella at Annertech commentedI think I've figured the pager issue out. When reformatting the datetime into the needed format, we're using the default value set in the View config, rather than that entered in the exposed filters.
Comment #133
stella CreditAttribution: stella at Annertech commentedI'm not sure why the test is failing for 8.4.x. It appears to be failing on the line
$this->assertFieldByXPath('//input[@id="edit-created-date" and @type="date"]', '', 'Found date input element.');
But there is a field with that id and type attribute. It also has no value - though maybe we should be passing in NULL rather than '' for the value param?
Patch reroll for 8.5.x attached
Comment #134
jibranRerolled #118 for 8.5.x. I also created 8.4.x patch for my use. :)
Comment #135
jibranI just rerolled the patch and it is working fine for me.
The only remaining task is to create a followup to make that date field configurable from views UI other than that this is RTBC.
Comment #136
gambryHey @jibran,
#118 doesn't work "when 'Expose operator' is checked" according to #129, although haven't tried myself.
Also @stella has done some work on #131 to #133, what's the feedback against that work?
Comment #137
jibranRight! It was not needed for my case so I overlooked that I'm sorry about it.
Somehow I ignored #132 and #133 did have the interdiff so I persumed it was just a reroll of #118 that was stupid of me.
We need tests for 'Expose operator' so adding the tag and changing the status.
Comment #139
alanburke CreditAttribution: alanburke at Annertech commentedUpdated patch for 8.3.x
Comment #140
matsbla CreditAttribution: matsbla commentedI tested patch in #134 together with #2625136-63: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements and the exposed filter looks like this;
Comment #141
BerdirCan confirm that this works quite well functionally, but we do have a problem when you have multiple date filters, which I think is fairly common with date range fields, because the wrapper just has a h4 + the date time wrapper so you start to mix different elements on the same level and the standard margin/float logic that applies to div.form-item doesn't apply:
@mpdonadio mentioned #1918994: Improve Datetime and Daterange Widget accessibility and that we added specific wrappers based on where it is used, so we should possibly do the same here, to make sure those elements have a common wrapper and have consistent margins/float definitions?
Comment #142
mmbkNo content changes here, I just made #139 applying to 8.6.-dev.
The patch was working as expected, I hope the tests will not fail.
Comment #144
mmbkSo, I fixed on of the tests. When using the widget, the fieldnames are 'created[date]' and 'created[time]' instead of simply 'created.
With in the changed test, I used the assertSession()->fieldExists instead of the deprecated assertField()
Coding warnings are removed.
I don't understand the second failure, so this will still fail, nevertheless I start the tests, to see whether I've done the rest correctly.
Comment #146
stella CreditAttribution: stella at Annertech commentedMy changes in comments #131 to #133 were lost in subsequent patches, and other changes seem to have been introduced in a patch reroll in #139 that I'm not sure were right.
Here's an updated version of the patch, essentially a reroll of #133 but with the changes for the tests added in #144
Comment #148
stella CreditAttribution: stella at Annertech commentedComment #150
stella CreditAttribution: stella at Annertech commentedNot sure what happened there, managed to upload an empty patch file. Here's the correct file and interdiff.
Comment #151
mmbk@stella this last patch removes the ability to expose a date-only filter, which was included in #139 which ist imho quite important.
Comment #152
hannessIs this Module relevant for this issue?
Date Popup
Comment #153
jhedstromSetting to NW for issues described in #141 and also #151:
Comment #154
jhedstromIn these cases we also need to check for
not between
operators.In manual testing the patch does indeed apply the date element type to date-only fields, and, as noted above, #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements applied with this one does somewhat address the layout issue noted in #141.
However, I'm seeing an extra date element in the output:
This view has the 'authored on' filter which correctly works, but note the 'date and time' exposed between operator somehow adds that extra input. If I switch this to a non-between operator, it works as expected.
Comment #155
jhedstromThis patch adds support for the
not between
operators.The extra date input in the form actually comes from the addition of the patch in #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements.
Since the layout with this, but without that is broken, perhaps that one needs to go in first, then this issue can address whatever is causing the additional date input element to appear when between operators are used.
Comment #156
jhedstromComment #157
jhedstromThis is the only change needed here with #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements to get the between operators working:
Comment #158
mpdonadioAssigning to myself to remember to review in full.
Do we want explicit test coverage for 'not between'? Think this may be overkill, but that test would have caught the fact that we forgot about it :/
Comment #159
matthieuscarset CreditAttribution: matthieuscarset as a volunteer and commentedI have just tested this patch on a fresh install.
It works for the base Node's fields "created" and "changed" but it is not applied for a custom Datetime field.
Use case:
Expected result:
The exposed filter is a datetime form element.
Actual result:
The exposed filter is a textfield element.
Hopefully I have missed something here... because not having form element by default for Dates in Drupal 8 is a huge blocker.
Comment #160
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commented@matthieuscarset i don't think we should deal with datetime_range module on this issue.
As you said:
You are right, we should have form elements by default for dates in views filters, so let's fix this on this issue. Then we'll fix it for datetime_range module.
The patch works as expected considering #157 comment.
Here is a screenshot of between filter.
Comment #161
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedHere is what the filters will look like once #157 will be implemented
Comment #162
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedremove duplicate comment
Comment #163
jhedstromThis actually still needs a bit of work. With the old textfield style inputs, a date-only field could still take something like
2018-06-06
as an input. With this patch, date-only fields are forced to enter a time. We need to hide the time input for date-only fields, and also set the time to23:59:59
for the max input, and00:00:00
for the min input I think.Comment #164
jhedstromHmm, actually, this is already done (but I did see it still appearing under certain circumstances, so need to dig deeper).
Comment #165
jhedstromSorry for the noise. All the issues noted in #163 are working just fine in core. My issue was specific to combining this patch with a
search_api
view. Back to @mpdonadio for review.Comment #166
GoZ CreditAttribution: GoZ at Iosan, Barbe-Rousse commentedHere are tests for not_between as exposed by #158.
Done during DevDaysLisbon
Comment #168
jhedstromThanks @GoZ for the tests!
Comment #169
matsbla CreditAttribution: matsbla at Globalbility commentedThe patch in #166 works good for me, but it breaks the patch #76 here #2625136-76: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements making the div containers of the filters disappear. We should find out which issue this should be solved in, here or there? Both patches works good independent of each other.
Comment #170
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedFound an issue when the operator is exposed.
When we select between and not between operators , the input fields are not rendering properly in date format.
Comment #171
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedComment #172
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedAdding a patch that will work with exposed operators for an exposed datetime field on a view along with a test.
Please note: States is not working properly with datetime elements, this issue is being worked upon here https://www.drupal.org/node/2419131
So for this patch to work correctly for exposed operators on views, the states patch should also be applied.
Comment #174
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedTest failures due to wrong selector. Correcting selectors gets the test passed but not sure if a text placeholder is valid for a date field. This test might need changes.. (it is a text value without patch, but the patch makes the field date). Suggestions welcome !!
Comment #175
jhedstromIt doesn't look like html5 date elements support the 'placeholder' attribute, so the failing test can probably be updated to reflect that. However, we do need to determine why the patch from #166 didn't cause that test to fail, while the one from #172 does.
Comment #176
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commented@jhedstrom, The previous patch was having two elements repeated for the date fields, one with textfield and another with the datetime widget. which is why the test didnt fail last time....
Posting an image with exposed operators with the old patch !
This patch of mine fixes this issue !
Comment #177
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedUploading a patch with a "fix" for the failing tests,
Since placeholders are not valid for date fields anymore and the failing test was intended for a numeric value, I changed the filter to node id (only numeric field i cud find though it may not make real sense LOL).
Submitting interdiffs for both 166-177 and 172-177
Thanks,
Sukanya
Comment #178
matsbla CreditAttribution: matsbla at Globalbility commentedI've tried to set a default value for the exposed filter:
Screenshot from 2018-10-11 04-12-03.png
However, after the user is not any longer able to change the value in the datepicker to something else, even though it is an exposed filter:
Screenshot from 2018-10-11 04-16-34.png
Is the field "Value" suppose to be a "Default value"? It is about ambiguous.
Comment #179
jhedstromGood catch regarding the exposed operator @sukanya.ramakrishnan!
A few comments/questions and nits on #177.
This should be changed back to 'determine' :)
These indentation changes look un-intentional?
When breaking arrays up onto separate lines, the first item should also be on its own line.
I'm curious about these test changes. I think it would provide more complete coverage if we test a few different filter/operator combinations.
Comment #180
jhedstromAh, regarding my comment #179.4, that was just git being weird with the interdiff--reviewing the patch itself I see you did add a new test method for exposed operators.
Comment #181
jofitz CreditAttribution: jofitz commentedAddressed the comments in #179.
Comment #182
alexfarr CreditAttribution: alexfarr commentedHi all,
I wish to add my thoughts to this if I may.
I strongly feel that the use of the HTML5 date element should be optional, this is for two main reasons.
1: When this patch is applied to a view that already uses the "better exposed filters" date picker both the native and jquery date pickers are rendered. This may or may not be constituted as a breaking change.
2: The use case for the HTML5 date element is too narrow. There is a very valid case for the application to control the input date format, and not the browser. Example: an EU worker working on a US platform for a US company. They are required to input data in US format by business rules, this would not be possible via the HTML5 element.
Further to this, dates displayed on the site are controlled by the system and not the browser, having data view be different to data entry sounds like an antipattern.
Also if you have a roaming worker, they would be expected to enter date formats based on the physical local or machine rather than their account, this is highly likely to cause errors in data.
For these reasons we are unable to use the HTML5 date element, and why I would kindly request that this is optional.
Thanks for all the hard work on this issue.
Comment #183
alexfarr CreditAttribution: alexfarr commentedI am getting an error when using #181
I have an existing view that uses bef date picker, when trying to disable the datepicker option i click on the bef Settings link but nothing happens, in the console i can see the following error:
Comment #184
NickDickinsonWildeConfirmed to be working great on existing & new views.
re: #182
#1: I think that does need a Change Record, but is otherwise fine
#2: All dates should be normalized, and then formatted as needed for localization etc.
#138:
That is not a bug with this patch. Although, yes, it is exposed with BEF + this patch. Under some circumstances,
Drupal\Core\Form\FormState::getUserInput()
, returns null instead of an array as described by the the return annotations. Which means that should be fixed in a separate issue. I'll see if I can find one or if a new one needs to be created.Comment #185
NickDickinsonWildeDraft Change Record created.
Comment #187
larowlanFor #183 - either we need a separate issue or we need to resolve it here
Also #182 raises a valid point, should this be opt-in?
we should use the third argument here
same for these
Comment #188
anthonylindsay CreditAttribution: anthonylindsay at Annertech commentedI'm not sure this works with 8.6.9?
I got it to work on 8.6.3, but it's resulting in null values when submitted on 8.6.9.
Comment #189
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, I've not read all the 188 replies but would be possible to have a friendly widget to use as views fiter when the date field is exposed with the option
An offset from the current time such as "+1 day" or "-2 hours -30 minutes"
?Thanks :-)
Comment #190
pidru CreditAttribution: pidru commentedPerhaps you gonna try this
https://www.drupal.org/project/date_popup
Comment #191
FiNeX CreditAttribution: FiNeX as a volunteer commented@pidru: thanks! I will try it :-)
Comment #193
BerdirJust a reroll, conflicted on format_date() deprecations. Had to create the patch with -C90 as it did some weird stuff with copying from unrelated views and resulting in a larger and very different patch. This one is almost identical.
Comment #194
jhedstromMaking this optional will further complicate an already very complex set of logic, and make integration with fixes for some outstanding bugs such as #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements even more complex.
If we do make it optional, I'd say that moving forward HTML5 should be the default. If we opt-out existing sites via an update hook, that would simplify the conflicts with BEF described above.
Regarding the specific example in #182
Views should already be handling the conversion of what's entered in the browser to what the query actually needs. The formats passed by an HTML5 element are already munged on the front-end to the format specified by the input (which is the same format as user would currently enter with the plain textfield input.)
For actual data entry, date fields can already opt in or out of HTML5.
The other issues raised in #187 need to be addressed (third arg set to true for strict type checking in the
in_array
calls are the only patch changes, the other is a new issue created for #183)Comment #195
Riloto CreditAttribution: Riloto commented@pidru Thanks, its works for me!
Comment #196
MNSTR CreditAttribution: MNSTR commentedHello
I can't apply patch 181 on Drupal 8.7.1.
It fails. Anyone has got the issue?
Comment #197
rick_p CreditAttribution: rick_p as a volunteer commentedYes, I have the same issue...
Although, I'm beginning to wonder if I need the patch anymore because I still get a datepicker on my local after the 8.7.1 upgrade without the patch installed. I should mention that we use the use the bef jquery date picker.
Comment #198
volegerSee #193 , it's a reroll patch from #181
Just replace pathc url
https://www.drupal.org/files/issues/2018-11-02/2648950-181.patch
with the new onehttps://www.drupal.org/files/issues/2019-03-15/2648950-193.patch
.Comment #199
rick_p CreditAttribution: rick_p as a volunteer commentedI not sure if this will be true (happen) for others, but I tried #193, composer installed it OK but it broke date range for me. It would work to select the start date, but when you selected an end date it would not populate that field and also reset the start date field.
Comment #200
MNSTR CreditAttribution: MNSTR commentedThank you voleger, I thought #193 was for D8.8
Comment #201
volegerIt is. But 8.7.x and 8.8.x have no really big difference right now in the scope of the patch. That's why a patch is still good for 8.7.1
Comment #202
AaronMcHaleThanks for all the work on this.
Tested #193 on Drupal 8.7.2 and seems to work, although one nice to have would be the ability to control granularity, for example for me the "time" compoment is always shown, but in our case we only want the "date" component.
Thanks
-Aaron
Comment #203
imclean CreditAttribution: imclean at Digital Ink commented@AaronMcHale this is being worked on: #2868014: [PP-1] Views Date Filter Datetime Granularity Option
Comment #204
AaronMcHale@imclean thanks :)
Comment #205
cameron prince CreditAttribution: cameron prince at CivicActions commentedWith core 8.7.5, the 193 patch fails on tests/src/Functional/Handler/FilterDateTest.php. Here's a re-roll which fixes this.
Comment #206
dww#205 doesn't apply to cleanly to the end of the 8.8.x branch:
Also, please include an interdiff when posting new patches so we can easily see what's changed as part of a "reroll". Sounds like #205 is trying to do more than just a reroll, but is fixing a logic bug of some kind. Hard to tell what that fix is.
Thanks,
-Derek
p.s. Meanwhile, #193 still applies cleanly to the end of 8.8.x branch, although it also still needs work per #194 and others.
Comment #207
cameron prince CreditAttribution: cameron prince at CivicActions commentedI was mistaken with #205... The site I was working against is actually on 8.6.15. Please disregard.
Comment #208
olivier.br CreditAttribution: olivier.br commentedreroll of 193, adding a condition to fix this issue :
Error: Call to a member function get() on null in Drupal\datetime\Plugin\views\filter\Date->buildExposedForm() (line 190 of core/modules/datetime/src/Plugin/views/filter/Date.php).
Comment #209
SpokjeComment #210
SpokjePatch #193 with these comments from #187 addressed:
Not addressed, but also mentioned in #187:
Comment #211
SpokjeMeh, "new" (as in after #193 new) test in D8.8 doesn't play nice. Will return tomorrow to see if that's fixable by me.
Comment #212
david.qdoscc CreditAttribution: david.qdoscc commentedWould be great to see this committed soon. I had a nightmare trying to change views exposed date filters from mm/dd/yyyy to dd/mm/yyyy. Only way I could get it working was with a form_alter to change the field type from date to datetime.
Comment #213
SocialNicheGuru CreditAttribution: SocialNicheGuru commented#210 does not apply cleanly to Drupal 8.7.8.
Comment #215
dwwI spent a few hours with this issue on a flight this evening, trying to get all this actually working. What a mess! ;)
First of all, this is really broken and weird without #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements, so let’s just postpone this issue and make sure we can get that one in first. It’s *much* smaller, easier to review/test, so hopefully we can land that soon. Ideally before 8.8.0-beta1.
Next, there are a number of problems with the latest patch in here:
1. We need to stop letting ‘Regular expression’ be a valid operation for this filter once the exposed widget is a date picker. It simply doesn’t work, and makes no sense. Hopefully with actual dates to work with, no one would need a regexp in here. Seems simplest to remove it. Making it work would require keeping a text field value around for that 1 operation, updating #states and a bunch of form processing, etc. Ugh. Hopefully we can just kill it. I can't really fathom why someone would want to use a regexp to find dates, given the alternatives.
2. I wish this was out of scope (this is already a giant mess!), but I don’t think it is: Once these widgets are datetime elements instead of text fields, we can’t use placeholders. HTML5 date picker doesn’t allow a placeholder. So all of those options have to go, too.
3. These two points mean I fear we'll need an update to update everyone's views, update tests, etc. Ooof.
4. Hopefully at least *this* can be punted to a follow-up: it’d sort of be nice if the value in the the settings form was also a datetime element, not a text field. But that complicates life with the radio button to select offset vs. date stuff. :( TBD.
5. The logic in here needs a refresh once you apply #2625136. #157 was a start, but that only handled date-only fields and min/max. It was still broken for the main ‘value’ date element for everything *but* between / not between.
6. This comment is wrong, missing an extremely important “not”, making this logic more confusing than it already is:
7. If you expose operations in here, that only really works like you want via #states (conditionally hiding min/max unless the operator need it, hiding everything for empty/not empty, etc). But #states has never worked for datetime elements. :( See #2419131: [PP-1] #states attribute does not work on #type datetime for more. Sadly, our collective attempts to fix that ran into the ‘you must test with classy’ vs. ‘thou shalt not change classy/stable templates’ deadlock hell. :( I’ve re-rolled that for All The Branches(tm). Thankfully, we've found a way out of the deadlock hell, so that issue is no longer postponed. Additional support at #2419131 would be much appreciated!
So for now, I'm calling this [pp-2] and blocked on these:
It's 12:20am locally, my body thinks it's 3:20am, and I need sleep! :) I'll post patches and more tomorrow, so assigning this to myself for now.
Meanwhile, all hands on deck at these two blocker issues! #2625136 is very likely RTBC if someone wants to give it a final review, test, etc.
#2419131 is in the home stretch, and doesn't need that much work.
Thanks!
-Derek
Comment #216
dwwOkay, sorry this is so confusing, but this is a complicated issue, trying to change an extremely complicated part of core. Please bear with me on this comments and all the files. I hope it all makes sense.
First of all, the test failure in #210 is fantastic! The initial problem is that this new test added in 8.8.x was written for textfield values, not date + time elements. Cool. So we can fix those assertions (see 2648950-216.fix-test-assertions-only.DO-NOT-TEST.patch). But the test still fails. Why? Because it's catching a real bug in the previous logic here! Although we're trying to be clever about if min/max should be converted to a datetime, we get it wrong in the case that test is exercising: an exposed filter, with exposed operator, but the operators are limited to not include
between
andnot between
. I started trying to add the appropriate nestedif()
to handle this, but it was getting so long and complicated that I wanted to step back and try a different approach entirely.2648950-216.previous-logic.8_7_x.DO-NOT-TEST.patch is a patch (not worth having the testbot chew on) that fixes points 1, 2, and 6 from #215, plus some additional comment fixes and minor code style bug, sticking to the previous logic approach for when to convert elements to datetime. Interdiff relative to #210 is: 2648950.210-216-previous-logic.interdiff.txt
Then, my new approach boils down to this:
Interdiff between previous and new logic is here: 2648950.216-prev-new.interdiff.txt
If we're cool with that approach (and you fix the test assertions to look for date/time elements, not text), everything works. Yay!
So 2648950-216.new-logic.8_7_x.patch is the new logic (minus the fixes for the test assertions, since that test doesn't exist in 8.7.x).
2648950-216.new-logic-fix-test-assertions.8_8_x.patch is the new logic and fixes for the test assertions (should work on both 8.8.x and 8.9.x).
2648950-216.fix-test-assertions-only.DO-NOT-TEST.patch is the interdiff between them.
2648950.210-216-new-logic.interdiff.txt is the interdiff between the full 8.8.x patch and #210.
Finally, since I want this to work with #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements, I'm attaching patches that you can apply on top of either the previous logic or new logic patches that fixes the interaction with #2625136:
2648950-216.changes-for-2625136-new-logic.DO-NOT-TEST.patch : use with
2648950-216.new-logic.8_7_x.patch
or2648950-216.new-logic-fix-test-assertions.8_8_x.patch
vs.
2648950-216.changes-for-2625136-previous-logic.DO-NOT-TEST.patch : could be used with
2648950-216.previous-logic.8_7_x.DO-NOT-TEST.patch
, but is still broken in some cases.Phew! :)
TODO:
core/modules/datetime/src/Plugin/views/filter/Date.php
where we hide the time element for date-only fields. I didn't get that far, and wanted someone else to agree with the new logic approach before I got much further.However, if none of that bothers you too much, and you apply the latest patches from #2625136-103: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements and #2419131-129: [PP-1] #states attribute does not work on #type datetime, and can navigate this comment and figure out which patches to apply, you'll actually have a working exposed datetime filter. Huzzah! :) Good luck!!
Cheers,
-Derek
p.s. Hiding a bunch of older files, and many of the ones I'm attaching here, from the summary. Trying to make it easier to find what you might be looking for. ;)
p.p.s. Edited to fix filenames and add links to patches + interdiffs.
Comment #217
dwwThanks bot! 🙏 I was also doing some more local testing and noticed some flaws in my "simplified" new logic. I was being a bit careless. Here are new versions that should be all green again:
- More careful about what's in #type to make sure we're converting what we think we are. ;)
- Fixes the date-only cases by updating
core/modules/datetime/src/Plugin/views/filter/Date.php
2648950-217.newer-logic.8_7_x.patch (without the fixes to
core/modules/datetime/tests/src/Functional/DateFilterTest.php
).2648950-217.newer-logic.8_8_x.patch (8.8.x and 8.9.x, full patch)
2648950-217.changes-for-2625136.DO-NOT-TEST.patch - updated patch to work with #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements
plus interdiff vs. #216 (only dealing with "new" vs. "newer" logic, not the "previous" logic patches).
Comment #218
dwwHah, #217 fails on 8.8.x and above due to #3087692: Remove the core key from views configuration., which I helped with. Whoops! ;)
I still think this should be postponed on these:
But this is basically ready for a new round of reviews, testing, nitpicking, etc. And if anyone else wants to flesh out the test coverage even more, please do! :)
Almost there...
Comment #219
dwwThis has been bothering me:
I couldn't find a recursive
Element::children()
so I did my own via aprotected
method on the filter plugins.I believe this is a lot cleaner. Thoughts?
Note:
2648950.218_219.interdiff.txt
is also exactly the interdiff between2648950-217.newer-logic.8_7_x.patch
and2648950-219.8_7_x.patch
.Edit: Now, even with 2648950-219.changes-for-2625136.DO-NOT-TEST.patch applied, the above becomes just this:
Ahhhhh, sweet relief. ;)
Comment #220
jibranThis can be changed to
->getSetting('datetime_type')
Comment #221
jibranI don't think we should hardcode it. It should be configurable per expose filter.
Let's say I want to use this plugin for created, changed or any timestamp field. I can implement hook_views_data_alter and make those fields use this filter but this check stops me from using the date element. Thoughts?
Comment #222
dww@jibran: Thanks for the reviews!
Re: #220: Duly noted. I confess to not having gone through everything in here with a fine tooth comb, yet. Any other nits / improvements you (or others) can find, please post. I'll hopefully have a chance to re-roll and incorporate all the changes in the next day or two. (Although I'm in the SF Bay Area, the house I'll be sitting for the next week is currently without power, and large swaths of the state are on fire, so it's gonna be a bit hectic). :/
Re: #221: You misunderstand. It already allows you to use datetime for timestamp fields like node changed. All that's in core/modules/views/src/Plugin/views/filter/Date.php.
What you're looking at in core/modules/datetime/src/Plugin/views/filter/Date.php is the logic to hide the time element for actual datetime fields that are configured (field-storage-wise) to be date-only fields, not date/time fields. Make sense? Do we need better comments to this effect?
That said, it's still an open question as to whether we should let site/view builders decide if they still want a plain text field or a datepicker. That's basically the questions around if we should make this a whole separate filter plugin or not, if we want to add more configuration, or if we want to force the UI to always be a datetime element for any timestamp or datetime fields. Thoughts? ;)
Thanks again!
-Derek
Comment #223
jibranRE #220: Are we missing test coverage there?
Yeah, my bad I didn't apply the https://www.drupal.org/files/issues/2019-10-27/2648950-219.changes-for-2....
Yeah, I was wondering the same thing. On the one hand, I want to say yes because right now I'm battling these configurations on a client project in the codebase and without these configurations the expose filter seems incomplete.
On the other hand, it is too much information coming from UI. We can allow label changes as well.
Comment #224
dwwThis fixes #220.
Also, I started working on #2868014: [PP-1] Views Date Filter Datetime Granularity Option. I realized that's going to need
convertElementToDateOnly()
in some cases, but something slightly different in others (e.g. if granularity is minute or hour to set #date_increment on the datetime element, not just completely hide the time). So, to avoid adding this function here and then rename/repurpose it there, let's just add it here in such a way it's easy to use there. Ahh, the joys of being the only one writing all this code right now, I can effortlessly coordinate with myself. ;)Comment #225
dww#2419131: [PP-1] #states attribute does not work on #type datetime is now blocked on #3078334: Datetime and Datelist elements should render as fieldsets. So this is PostPoned on 3 issues (hence "[PP-3]" for all the folks who don't understand our cryptic system here in the core queue). Updating summary accordingly...
However, I'm leaving this at needs review, since I'd still love feedback from (other?) Views maintainers, committers, community, etc, on this key point:
Thanks,
-Derek
Comment #226
mandclu CreditAttribution: mandclu at Northern Commerce commented@dww thanks for all of your work on these issues.
Based on your most recent comment, I would suggest that the optimal past would be to fully adopt what we consider the optimal approach (which is sounds like is "remove the placeholders and regexp from the existing filter") and then write a hook_update_N implementation to convert over views configurations as much as possible.
Comment #228
larowlan#2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements is in
Comment #230
akalata CreditAttribution: akalata at Tag1 Consulting commentedWe've been hitting an edge case related to this change in one of our projects related to this patch.
Steps to reproduce:
Result:
I've attached an update for 8.9.x with the necessary changes added to `Datetime::valueCallback()`. I tried patching 9.2.x, and while the non-test changes apply cleanly I did not see a functionality change.
Comment #231
akalata CreditAttribution: akalata at Tag1 Consulting commentedComment #234
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed test of patch #231.
Comment #235
akalata CreditAttribution: akalata at Tag1 Consulting commentedComment #236
BerdirAttempt at a reroll for 9.1. The reason for it not working anymore because of the related issue that got fixed. Luckily, @dww already provided the fix for that in #224, so including that partial patch.
Also updated deprecated assert methods when I saw them.
interdiff is probably not quite right, mixed with merge conflict resolution.
Comment #237
BerdirComment #239
larowlanRe-roll of #236 with
-M 25%
, composer-patches was dying with the two copies of modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_placeholder_text,git apply
can handle it, butpatch
cannot.No changes at all other than the -M for those using composer patches.
Comment #241
SpokjeAttempt at an all green patch.
Comment #242
SpokjeComment #244
SpokjeComment #245
SpokjeReroll of #241 needed after #3159788: AssertLegacyTrait::assert(No)Text() in functional tests still have a message passed in was committed.
Comment #246
douggreen CreditAttribution: douggreen as a volunteer commentedAttached patch only for 9.1.x.
Comment #247
acbramley CreditAttribution: acbramley at PreviousNext commentedNeeded another reroll for 9.1.x due to a conflict in
core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
Comment #248
bkosborneFor anyone that needs both this functionality AND granularity functionality as described in #2868014: [PP-1] Views Date Filter Datetime Granularity Option, I created a patch in that issue that will apply after this patch has been applied. See comment #111.
Comment #249
BhumikaVarshney CreditAttribution: BhumikaVarshney as a volunteer and at OpenSense Labs commented#247 patch applies cleanly without any conflict.
Thanks
Comment #250
quietone CreditAttribution: quietone as a volunteer commented@BhumikaVarshney, there is no need to add a comment that a patch applied correctly. The fact that the testbot was able to run the tests is proof of that.
Comment #252
kim.pepperComment #253
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedRe-roll for 9.3.
Comment #254
ExTexan CreditAttribution: ExTexan commentedEdit: Disregard this. The datetime HTML input type seems to have been deprecated. We found a different solution for our needs.
I applied the patch from #234 to my D8.9.16 site, but saw no functionality changes.
I had already added from/to date filters to my view before applying the patch. After applying the patch, I cleared all caches, but the date filters were still text elements.
I edited/saved the view again. No changes.
I added a date filter again. No changes (still a text input element).
Am I missing something?
Comment #255
andypostFirefox 93 just released and properly handles
<input type="datetime-local">
Comment #256
matsbla CreditAttribution: matsbla at Globalbility commentedAfter I applied the patch in #253 the wrapper for the daterange filters are gone and the filters are placed cluttered.
Before patch:
After patch:
Comment #257
Rob230 CreditAttribution: Rob230 at CTI Digital for The Chartered Society of Physiotherapy commentedContrary to the comments above, patch #247 actually does not apply to Drupal 9.2. You can see the test run in October failed. It might originally have applied, but now one of the changes was already made in #3139404: Replace usages of AssertLegacyTrait::assertText, that is deprecated.
Re-rolled against 9.2 so that the patch applies.
Comment #259
joey-santiago CreditAttribution: joey-santiago commentedThe patch in #253 applies cleanly to 9.3.3 and my date filters in views now use browser's datepickers. Thank you very much, this is great!
Comment #260
nevergone CreditAttribution: nevergone at 2Toucans commentedPlease, needs review.
Comment #261
chucksimply CreditAttribution: chucksimply commentedPatch #253 works great for me on 9.3.3 also! Thanks for the work!
Comment #262
geaseThe last patch #253 breaks ajax functionality when the operator is also exposed (see the image).
Comment #264
romain.sickenbergHello,
Unfortunately the patch https://www.drupal.org/files/issues/2021-12-01/2648950-257.patch doesn't comply with Drupal 9.4 due to the test method
::setup
who's not compatible anymore with the parent.Comment #266
romain.sickenbergI re-rolled a patch and applied the fixes here with just the changes needed to follow the hierarchy.
Comment #267
ordermind CreditAttribution: ordermind commentedI just tested https://www.drupal.org/files/issues/2022-06-16/2648950-265_0.patch and the html structure seems wonky, the label is outside the wrapper div and the filter class is not on the outermost wrapper, which makes it more difficult to style. Look at https://www.drupal.org/project/date_popup, they're doing this aspect right.
Comment #269
leo liao CreditAttribution: leo liao commentedIn https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/view... the filter is set to date, which is correct. But this is later overridden in https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/view... ($attributes['type'] is int) and the filter is changed to numeric.
As a quick test I did this:
Comment #270
joegraduateThe attached patch addresses Yaml linting errors and removes a .php.orig file that was presumably added by mistake (looks like starting in #257).
Comment #271
nod_The issue summary says this is blocked on #3078334: Datetime and Datelist elements should render as fieldsets and #2419131: [PP-1] #states attribute does not work on #type datetime so updating the status for accuracy.
If those issues are not actually blocking and a patch can be reviewed please update the issue summary and set this back to needs review :)
Comment #272
robertoperuzzoI've changed the status to "Needs work" because this must be in the core. There is this contrib module https://www.drupal.org/project/date_popup that could help.
I tested the patch #270 and I get that in my view (it has two date fields in exposed filters)
So it works, but I don't need the time field in my case. So probably some other work on it is needed.
Comment #274
shadysamir CreditAttribution: shadysamir at Almanassa commentedEDIT: I had to uninstall Date Popup for this issue to disappear.
I applied #270 to an exposed filter on a timestamp field. I get this warning once I apply the filter:
followed by
both are trying to read
this->value['value']
when the array is:The filter is not applied (not added to the SQL statement)
Drupal 9.5.3
Comment #275
AnybodyJust a short heads-up, why this core issue is now even more important:
When trying to use date_popup or date_filter as workaround, but you're also using smart_date, it won't work! They're in conflict.
See #3370579: Integrate with date_popup. So if anyone is using these contrib modules as workaround, take care.
With this core issue fixed, that will be solved.
Comment #276
joseph.olstadOnce this issue is resolved it will unblock also:
#3210945-28: Remove dependency on jquery_ui_datepicker
Comment #277
joseph.olstadComment #278
basvredelingPatch #270 no longer applies to 10.1.x