Problem/Motivation

When adding a datetime exposed filter I cannot simply select a date - I have to manually enter a date which is very bad UX.

Proposed resolution

Use a form element with '#type' => 'datetime' so it's easy for users to select the date.

Change Record

Remaining tasks

Blocked on:

  • Investigate why the default value of the exposed filter is not picked up by the form (see #104). see #115
  • Hide time element for date-only type fields
  • Create a follow-up for the possibility for site builder to decide the widget type from the views management UI (see #48)
  • I think the new logic approach in #216 should also be happening in 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.
  • We clearly could use even more test coverage of all the possible combinations.
  • We should decide if we really want to remove the placeholders and regexp from the existing filter and potentially break existing views that are using them (#215), or just add a whole new filter plugin with a different name. Then a) site builders can decide which interface they prefer and b) we don't have to worry about upgrade paths.

User interface changes

Exposed date filters will have input[type=date], and will leverage HTML5 handling if supported by the browser, or fall back to the polyfill if it doesn't.
For datetime w/ type date-only we should still use the datetime form element, but in date-only mode ($element['#date_time_element'] = 'none'.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#272 Screenshot 2023-03-03 at 17.22.23.png26.81 KBrobertoperuzzo
#270 interdiff-2648950_269_270.txt5.88 KBjoegraduate
#270 2648950-270.patch63.42 KBjoegraduate
#269 2648950-265_1.patch68.49 KBleo liao
#266 2648950-265.patch67.96 KBromain.sickenberg
#266 interdiff_257_265.txt615 bytesromain.sickenberg
#262 Exposed date.png14.21 KBgease
#257 2648950-257.patch67.87 KBRob230
#256 After.png187.52 KBmatsbla
#256 Before.png263.68 KBmatsbla
#253 interdiff.txt11.11 KBKapilV
#253 2648950-253.patch63.27 KBKapilV
#249 Screenshot 2021-04-29 at 5.44.39 PM.png115.34 KBBhumikaVarshney
#247 2648950-247-9.1.x.patch63.65 KBacbramley
#246 2648950-246.9_1_x.patch64.07 KBdouggreen
#245 2648950-245.patch63.65 KBSpokje
#245 raw_diff_241_245.txt1.93 KBSpokje
#242 interdiff_239_241.txt789 bytesSpokje
#241 interdiff_239_241.patch789 bytesSpokje
#241 2648950-241.patch63.82 KBSpokje
#239 2648950-239.patch63.81 KBlarowlan
#237 2648950-236-interdiff.txt20.37 KBBerdir
#236 2648950-236.patch75.75 KBBerdir
#236 2648950-236.patch75.75 KBBerdir
#234 interdiff_231-234.txt590 bytesravi.shankar
#234 2648950-234_drupal8_9_x.patch70.14 KBravi.shankar
#231 interdiff-229-231.txt1.31 KBakalata
#231 2648950-231.8_9_x.patch61.02 KBakalata
#230 2648950-229.8_9_x.patch60.85 KBakalata
#224 2648950.219_224.interdiff.txt3.71 KBdww
#224 2648950-224.changes-for-2625136.DO-NOT-TEST.patch1.98 KBdww
#224 2648950-224.8_8_x.patch59.5 KBdww
#224 2648950-224.8_7_x.patch57.32 KBdww
#219 2648950.218_219.interdiff.txt5.25 KBdww
#219 2648950-219.changes-for-2625136.DO-NOT-TEST.patch1.98 KBdww
#219 2648950-219.8_7_x.patch57.05 KBdww
#219 2648950-219.8_8_x.patch59.23 KBdww
#218 2648950.217_218.interdiff.txt2.34 KBdww
#218 2648950-218.8_8_x.patch58.94 KBdww
#217 2648950-217.changes-for-2625136.DO-NOT-TEST.patch4.09 KBdww
#217 2648950.216_217.interdiff.txt4.61 KBdww
#217 2648950-217.newer-logic.8_8_x.patch58.98 KBdww
#217 2648950-217.newer-logic.8_7_x.patch56.76 KBdww
#216 2648950.216-prev-new.interdiff.txt3.34 KBdww
#216 2648950.210-216-new-logic.interdiff.txt7.66 KBdww
#216 2648950.210-216-previous-logic.interdiff.txt6.92 KBdww
#216 2648950-216.changes-for-2625136-new-logic.DO-NOT-TEST.patch1.25 KBdww
#216 2648950-216.changes-for-2625136-previous-logic.DO-NOT-TEST.patch4.23 KBdww
#216 2648950-216.fix-test-assertions-only.DO-NOT-TEST.patch2.44 KBdww
#216 2648950-216.new-logic-fix-test-assertions.8_8_x.patch58.55 KBdww
#216 2648950-216.new-logic.8_7_x.patch56.33 KBdww
#216 2648950-216.previous-logic.8_7_x.DO-NOT-TEST.patch56.51 KBdww
#210 interdiff-2648950-193-210.txt3.24 KBSpokje
#210 2648950-210.patch54.98 KBSpokje
#208 2648950-208.patch22.69 KBolivier.br
#205 2648950-205.patch55.09 KBcameron prince
#193 2648950-193.patch54.95 KBBerdir
#182 Screenshot from 2018-11-06 11-33-15.png23.24 KBalexfarr
#181 2648950-181.patch54.82 KBjofitz
#181 interdiff-2648950-177-181.txt1.94 KBjofitz
#178 Screenshot from 2018-10-11 04-16-34.png17.7 KBmatsbla
#178 Screenshot from 2018-10-11 04-12-03.png50.47 KBmatsbla
#177 interdiff-166-177.txt17.06 KBsukanya.ramakrishnan
#177 interdiff-172-177.txt6.91 KBsukanya.ramakrishnan
#177 2648950-177.patch55.45 KBsukanya.ramakrishnan
#176 Screen Shot 2018-10-05 at 11.44.08 AM.png34.91 KBsukanya.ramakrishnan
#172 2648950-172.patch48.54 KBsukanya.ramakrishnan
#172 interdiff-166-172.txt18.96 KBsukanya.ramakrishnan
#166 interdiff-2648950-155-166.txt12.16 KBGoZ
#166 2648950-166.patch37.65 KBGoZ
#161 datetime-between-filters-label.png41.14 KBGoZ
#160 datetime-between-filters.png33.06 KBGoZ
#156 2648950-155.patch25.08 KBjhedstrom
#156 interdiff-2648950-150-155.txt1.51 KBjhedstrom
#154 Screen Shot 2018-06-22 at 2.55.58 PM.png65.49 KBjhedstrom
#150 interdiff-2648950-143-150.txt33.54 KBstella
#150 2648950-150-8.6.x.patch24.99 KBstella
#146 interdiff-2648950-144-145.patch17.59 KBstella
#146 2648950-145-8.6.x.patch0 bytesstella
#144 interdiff-2648950-142-143.txt1.15 KBmmbk
#144 2648950-143.patch16.71 KBmmbk
#142 2648950-142.patch16.52 KBmmbk
#141 Selection_275.png83.42 KBBerdir
#141 Selection_274.png9.39 KBBerdir
#140 Screenshot from 2018-03-29 06-32-11.png4.4 KBmatsbla
#139 2648950-78-ab.patch26.43 KBalanburke
#134 use_form_element_of-2648950-134-8.4.x.patch24.99 KBjibran
#134 use_form_element_of-2648950-134.patch24.96 KBjibran
#133 use_form_element_of-2648950-131-8.5.x.patch25.6 KBstella
#132 interdiff-2648950-118-131.txt2.05 KBstella
#132 use_form_element_of-2648950-131.patch10.79 KBstella
#124 date-exposed-inline-stairs.png84.74 KBtacituseu
#121 use_form_element_of-2648950-120-8.3.2.patch25.59 KBpasan.gamage
#120 use_form_element_of-2648950-120-8.3.patch25.59 KBpasan.gamage
#118 interdiff-104-118.txt1.7 MBgambry
#118 use_form_element_of-2648950-118.patch25.57 KBgambry
#108 2648950-108.patch17.04 KBwtrv
#107 2648950-71.patch17.04 KBwtrv
#104 use_form_element_of-2648950-104.patch17.41 KBgambry
#104 interdiff-103-104.txt2.14 KBgambry
#104 use_form_element_of-2648950-104.patch17.41 KBgambry
#104 interdiff-103-104.txt2.14 KBgambry
#103 use_form_element_of-2648950-103.patch15.89 KBgambry
#103 interdiff-100-103.txt1.26 KBgambry
#100 2648950-100.patch15.06 KBjofitz
#100 interdiff-98-100.txt971 bytesjofitz
#98 2648950-98.patch15.06 KBjofitz
#96 add_date_picker_to_date_field_filter_in_view-2648950-95.patch709 bytesnassaz
#85 2648950-85.patch15.07 KBmpdonadio
#80 2648950-79-8.2.x.patch25.11 KBjefuri
#78 2648950-78.patch26.39 KBjefuri
#75 2648950-73.patch17.06 KBBerdir
#71 interdiff.txt2.12 KBdawehner
#71 2648950-71.patch17.08 KBdawehner
#65 2648950-65.patch15.92 KBdawehner
#55 interdiff-2648950-52-55.txt537 bytestlyngej
#55 2648950-55.patch16.41 KBtlyngej
#52 2648950-52.patch16.14 KBtlyngej
#50 interdiff-42-48.txt3.07 KBmpdonadio
#48 2648950-48.patch16.85 KBtregismoreira
#48 Screenshot at Aug 20 12-46-16.png24.7 KBtregismoreira
#42 interdiff-39-42.txt8.41 KBmpdonadio
#42 2648950-42.patch15.63 KBmpdonadio
#39 interdiff-38-39.txt1004 bytesmpdonadio
#39 2648950-39.patch11.07 KBmpdonadio
#38 interdiff-32-38.txt9.62 KBmpdonadio
#38 2648950-38.patch10.35 KBmpdonadio
#34 Screen Shot 2016-06-10 at 13.36.02.png17.27 KByannickoo
#34 Screen Shot 2016-06-10 at 13.36.19.png12.6 KByannickoo
#32 interdiff-27-32.txt2.02 KBmpdonadio
#32 2648950-32.patch5.07 KBmpdonadio
#27 interdiff-27.txt8.57 KBkgoel
#27 2648950-27.patch4.97 KBkgoel
#22 interdiff.txt3.28 KBkgoel
#22 2648950-22.patch5.41 KBkgoel
#18 interdiff-13-18.txt1.81 KBztl8702
#18 2648950-18.patch2.13 KBztl8702
#15 interdiff-04-13.txt1.46 KBmpdonadio
#13 2648950-13.patch2 KBskyredwang
#6 Screen Shot 2016-02-01 at 01.19.15.png49.64 KByannickoo
#4 use_form_element_of-2648950-4.patch1.65 KBmpdonadio

Issue fork drupal-2648950

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yannickoo created an issue. See original summary.

yannickoo’s picture

Title: Use date field instead of textfield when selecting date » Use form element of type date instead textfield when selecting a date in an exposed filter
mpdonadio’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +VDC

Good idea, but going to have to do this as part of 8.1, so adjusting.

mpdonadio’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.65 KB

First 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.

mpdonadio’s picture

yannickoo’s picture

Issue summary: View changes
FileSize
49.64 KB

Really cool!

I would really like the time support, sounds great :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

skyredwang’s picture

Status: Needs review » Needs work

I 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?

mpdonadio’s picture

#8 what browser? Chrome will use it builtin date input. Firefox uses the polyfill from the jQuery UI.

skyredwang’s picture

My 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?

skyredwang’s picture

Also, if the operator is "between", then we will need two widgets on two input box.

13jupiters’s picture

Out 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.)

skyredwang’s picture

I added a case handling for "between" operator.

gnuget’s picture

Status: Needs review » Needs work

Can you please provide an interdiff between #13 and #4?

Also this is going to need tests.

mpdonadio’s picture

FileSize
1.46 KB

Issue 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.

13jupiters’s picture

Manually 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.

ztl8702’s picture

Do we also need to deal with the 'not between' operator? (Although this might not be used as frequently as 'between')

ztl8702’s picture

Changed realField to options['expose']['identifier'].
Should fix the issue on #16.

ztl8702’s picture

Status: Needs work » Needs review
13jupiters’s picture

Tested #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).

mpdonadio’s picture

Status: Needs review » Needs work

Still need test coverage.

kgoel’s picture

Status: Needs work » Needs review
FileSize
5.41 KB
3.28 KB

I 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.

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for views.view.test_filter_datetime with the following errors: views.view.test_filter_datetime:display.default.display_options.filters.field_date_value.value missing schema in Drupal\Core\Config\Testing\ConfigSchemaChecker->onConfigSave() (line 93 of drupal/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php).

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

Status: Needs review » Needs work

The last submitted patch, 22: 2648950-22.patch, failed testing.

gnuget’s picture

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -107,4 +108,19 @@ public function testDateOffsets() {
+  public function testExposedForm() {

This method needs documentation.

jhedstrom’s picture

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -123,4 +124,19 @@ protected function opSimple($field) {
+  /**
+   * Override parent method to change input type.
+   */
+  public function buildExposedForm(&$form, FormStateInterface $form_state) {
+    parent::buildExposedForm($form, $form_state);
+
+    $field_identifier = $this->options['expose']['identifier'];
+    if ($this->operator == 'between') {
+      $form[$field_identifier]['min']['#type'] = 'date';
+      $form[$field_identifier]['max']['#type'] = 'date';
+    } else {
+      $form[$field_identifier]['#type'] = 'date';
+    }
+  }
+

This 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.

mpdonadio’s picture

@kgoel, if you are still working on this in addition to @jhedstrom's comment:

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -107,4 +108,19 @@ public function testDateOffsets() {
+    $this->assertTrue(strpos((string) $output, '<input type="date"') !== FALSE);
+  }

I think a ->assertFieldByXPath() would be better here. Something like

$this->assertFieldByXPath('//input[@type="date" and @name="field_date_value"]', 'Found date input element.');

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?

kgoel’s picture

Status: Needs work » Needs review
FileSize
4.97 KB
8.57 KB

Addressed #25 and #26

jibran’s picture

Issue summary: View changes
Status: Needs review » Needs work

Nice work! here is some feedback.

  1. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
    @@ -183,4 +183,19 @@ protected function opSimple($field) {
    +    } else {
    

    This is not a correct coding standard.

  2. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -16,7 +17,7 @@ class FilterDateTest extends HandlerTestBase {
    +  public static $testViews = array('test_filter_date_between', 'test_filter_date_between_exposed');
    
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +    $element = array(
    

    Can we please use short array syntax here?

  3. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +  protected function _testFilterDate() {
    

    This is not a lower camel case format.

  4. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +      '#type' => 'view',
    +      '#name' => 'test_filter_date_between_exposed',
    

    Why can't we

        $view = Views::getView('blah');
        $this->executeView($view);

    or

        $view = Views::getView('blah');
        $view->executeDisplay('default'); 

    here instead?

  5. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +    $this->verbose($output);
    

    We don't need this.

  6. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +    // $this->assertTrue(strpos((string) $output, '<input type="date"') !== FALSE);
    

    We can use $this->cssSelect() and check type here.

  7. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +    $this->assertFieldByXPath('//input[@id="edit-created"]', NULL, 'Found date input element.');
    

    Can we use $this->cssSelect() here?

  8. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,19 @@ protected function _testUiValidation() {
    +    //input[@id="edit-created"]
    

    Can we delete these comments?

mpdonadio’s picture

Thanks 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

$this->assertTrue($this->cssSelect('input[type="date"]#edit-created'), 0, 'Found date input element.');

?

Do we still need more coverage, or do we just need to polish this up?

jibran’s picture

Maybe submit the filter values and assert the results as well just to make sure everything is working properly after the change.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Going to try to wrap this one up in the next few days.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
5.07 KB
2.02 KB

#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

mpdonadio’s picture

Issue summary: View changes
yannickoo’s picture

So 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.

jibran’s picture

Status: Needs review » Needs work

#30 is still pending.

The last submitted patch, 32: 2648950-32.patch, failed testing.

mpdonadio’s picture

OK, 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.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
10.35 KB
9.62 KB

Should 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.

mpdonadio’s picture

Actually, 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.

The last submitted patch, 38: 2648950-38.patch, failed testing.

gnuget’s picture

Status: Needs review » Needs work

Hi mpdonadio.

I gave a quick review to your last patch and I found some nits:

  1. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,43 @@ protected function _testUiValidation() {
    +  protected function _testFilterDate() {
    +    $this->drupalLogin($this->drupalCreateUser(array('access content')));
    

    This needs a docblock.

  2. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,43 @@ protected function _testUiValidation() {
    +    // Apply the filter
    

    This line must finish with a dot.

  3. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
    @@ -153,4 +155,43 @@ protected function _testUiValidation() {
    +    // Verify the node list
    

    Missing dot.

Great work! thanks.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
15.63 KB
8.41 KB

FB 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.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I 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).

jhedstrom’s picture

Status: Reviewed & tested by the community » Needs review

Setting back to NR since I discovered this bit of code:

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -131,7 +131,13 @@ public function acceptExposedInput($input) {
-    $type = $this->value['type'];
+    if (array_key_exists('type', $this->value)) {
+      $type = $this->value['type'];
+      $restore_type = TRUE;
+    }
+    else {
+      $restore_type = FALSE;
+    }

@@ -155,7 +161,12 @@ public function acceptExposedInput($input) {
-    $this->value['type'] = $type;
+    if ($restore_type) {
+      $this->value['type'] = $type;
+    }
+    else {
+      unset($this->value['type']);
+    }

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.

mpdonadio’s picture

Nice 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.

mpdonadio’s picture

I 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().

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tregismoreira’s picture

Hey 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.

choose widget

Status: Needs review » Needs work

The last submitted patch, 48: 2648950-48.patch, failed testing.

mpdonadio’s picture

FileSize
3.07 KB

@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.

NetNerdy’s picture

Issue tags: -VDC, -SprintWeekend2016, -SprintWeekendDCNJ +views date filter

patch #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

tlyngej’s picture

Re-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.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -views date filter +VDC, +SprintWeekend2016, +SprintWeekendDCNJ

Status: Needs review » Needs work

The last submitted patch, 52: 2648950-52.patch, failed testing.

tlyngej’s picture

Added a check to see if an array key exists before trying to read the value of it.

Status: Needs review » Needs work

The last submitted patch, 55: 2648950-55.patch, failed testing.

mpdonadio’s picture

I 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.

gambry’s picture

I 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?

tlyngej’s picture

@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.

gambry’s picture

@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.

tlyngej’s picture

@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.

mpdonadio’s picture

I 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.

mpdonadio’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

This 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.

dawehner’s picture

Here is a new version of the patch as I needed it for a client.

adriancid’s picture

Status: Postponed » Reviewed & tested by the community

I made some test and it works

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: 2648950-65.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Postponed

This 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.

Yekaterina’s picture

@dawehner, thanks, works on d.8.2.5.

Is there way to use the exposed filter only by year?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

FileSize
17.08 KB
2.12 KB

I had some notices with this patch recently.

jlbellido’s picture

I've tested it and it works like a charm!
Thanks @Dawehner for your awesome work!

dkre’s picture

#71 Works but needs an array check in FilterPluginBase:

From the patch:

diff --git a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
index 5c2a8d8..4d0e201 100644
--- a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1404,7 +1404,7 @@ public function acceptExposedInput($input) {
         }
       }
       if (isset($value)) {
-        $this->value = $value;
+        $this->value = array_merge($this->value, $value);
         if (empty($this->alwaysMultiple) && empty($this->options['expose']['multiple']) && !is_array($value)) {
           $this->value = array($value);
         }

Addition:

if (isset($value)) {
   
 -      $this->value = array_merge($this->value, $value);
+      	if(!is_array($value)){
+    		   $this->value = $value;
+      	}else{
+     	   $this->value = array_merge($this->value, $value);
+      	}
       if (empty($this->alwaysMultiple) && empty($this->options['expose']['multiple']) && !is_array($value)) {
   $this->value = array($value);
}

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.

mpdonadio’s picture

Status: Postponed » Needs work

OK, #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).

Berdir’s picture

Status: Needs work » Needs review
FileSize
17.06 KB

Here'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 :)

mpdonadio’s picture

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -36,6 +37,21 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
+      $form['value']['widget'] = array(
+        '#type' => 'radios',
+        '#title' => $this->t('Widget'),
+        '#options' => array(
+          'datetime' => $this->t('Date and time'),
+          'date' => $this->t('Date only'),
+        ),
+        '#default_value' => !empty($this->value['widget']) ? $this->value['widget'] : 'datetime',
+        '#states' => array(
+          'visible' => array(
+            ':input[name="options[value][type]"]' => array('value' => 'date'),
+          ),
+        ),
+      );

This 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.

Status: Needs review » Needs work

The last submitted patch, 75: 2648950-73.patch, failed testing.

jefuri’s picture

Status: Needs work » Needs review
FileSize
26.39 KB

Added the addition code from #73. Because this patch breaks other non-array values from the exposed filter.

mpdonadio’s picture

#78, can you post an interdiff? The patch went from 17k to 26k.

jefuri’s picture

Also created a patch for the current 8.2.x branche of drupal core.

The last submitted patch, 78: 2648950-78.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 80: 2648950-79-8.2.x.patch, failed testing.

mpdonadio’s picture

#78 and #80 have a lot of out of scope formatting changes.

Partial review of #75. Still need to manually review patch applied.

  1. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
    @@ -36,6 +37,21 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
    +      $form['value']['widget'] = array(
    +        '#type' => 'radios',
    +        '#title' => $this->t('Widget'),
    +        '#options' => array(
    +          'datetime' => $this->t('Date and time'),
    +          'date' => $this->t('Date only'),
    +        ),
    +        '#default_value' => !empty($this->value['widget']) ? $this->value['widget'] : 'datetime',
    +        '#states' => array(
    +          'visible' => array(
    +            ':input[name="options[value][type]"]' => array('value' => 'date'),
    +          ),
    +        ),
    +      );
         }
    

    Needs to be updated to use short array syntax.

  2. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
    @@ -185,4 +201,23 @@ protected function opSimple($field) {
    +      if ($this->operator == 'between') {
    +        $form[$field_identifier]['min']['#type'] = $this->value['widget'];
    +        $form[$field_identifier]['max']['#type'] = $this->value['widget'];
    +      }
    +      else {
    +        $form[$field_identifier]['#type'] = $this->value['widget'];
    +      }
    +    }
    

    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.

mpdonadio’s picture

OK, 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.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
15.07 KB

Here 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.

Status: Needs review » Needs work

The last submitted patch, 85: 2648950-85.patch, failed testing.

Lendude’s picture

The View needs to be exported again because of #2459289: Boolean default values are not saved, that should clear up some fails.

flyke’s picture

Could 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.

mpdonadio’s picture

#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.

dubcanada’s picture

We should also add time to the time when it is available. Not sure if their is a drupal core polyfill?

mpdonadio’s picture

Dom.’s picture

Hi 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:

(
    [plugin_type] => filter
    [id] => string
    [class] => Drupal\views\Plugin\views\filter\StringFilter
    [provider] => views
)

So datetime exposed filter is of time StringFilter and is thus not impacted by Date.php buildExposedForm() such as changed by patches here.

mpdonadio’s picture

#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.

Dom.’s picture

nassaz’s picture

I think this work actually, to change simple input by datepicker input, this is useful for users to choose the right format.

nassaz’s picture

mpdonadio’s picture

#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.

jofitz’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
15.06 KB

Re-roll based on #85.

Status: Needs review » Needs work

The last submitted patch, 98: 2648950-98.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
971 bytes
15.06 KB

Resolve one of the test fails.

Status: Needs review » Needs work

The last submitted patch, 100: 2648950-100.patch, failed testing.

gambry’s picture

Assigned: Unassigned » gambry
Issue tags: +SpringSprintLondon2017

The 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.

gambry’s picture

Assigned: gambry » Unassigned
FileSize
1.26 KB
15.89 KB

This 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.

gambry’s picture

Test 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?

mpdonadio’s picture

#104, that's odd, I would have thought that just using '#default_value' would work.

gambry’s picture

I would have thought that just using '#default_value' would work

#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.

wtrv’s picture

Adjusted the patch from #71 to support the short array syntax found in 8.3.2

wtrv’s picture

Resubmitted #107 with fixed patch name.

gambry’s picture

@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).

gambry’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updating the IS with guidelines from #84 and remaining tasks.

The last submitted patch, 107: 2648950-71.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 108: 2648950-108.patch, failed testing.

jackenmail’s picture

Retested the #109(2648950-108.patch)

mpdonadio’s picture

Agree 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.

mpdonadio’s picture

OK, about the #default_value:

<dawehner> tell mpdonadio Exposed forms are weird, because they are GET forms and as such basically triggered all the time

So, I think that means our method is OK.

gambry’s picture

So, I think that means our method is OK.

That's a relief.

However I'm not 100% happy with this redundant code:

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -185,4 +186,54 @@ protected function opSimple($field) {
+        // Check the element input matches the form structure.
+        $input = $form_state->getUserInput();
+        if (isset($input[$field_identifier]) &&  !is_array($input[$field_identifier]) && $value = $this->value['value']) {
+          $date = new DrupalDateTime($value);
+          $input[$field_identifier] = [
+            'date' => $date->format('Y-m-d'),
+            'time' => $date->format('H:i:s'),
+          ];
+        }
+        $form_state->setUserInput($input);

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?

wtrv’s picture

@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

gambry’s picture

Attached the patch hiding the time form element component when the field is a DateTimeItem::DATETIME_TYPE_DATE ('date' only) type. Below few thoughts while I was working on it:

  1. So far we've been working on 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 inside Drupal\datetime\Plugin\views\filter\Date() datetime plugin
  2. For the reason above datetime_range 'allday' type exposed element is not covered by this patch. We need to add a follow-up when #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields is in (or add the check in there when this one is in).
  3. Additional test coverage simply tests the right form element are displayed, it doesn't submit the forms nor validate results. Should we do it?
  4. Most of the new test Drupal\Tests\datetime\Functional\Handler\FilterDateTest code can be place in a *Base abstract class if you think future tests can benefit from it.
  5. Noticed a lot of our assert method are deprecated. Shall we change them while this is still in progress?

Also:

  • Removing Needs tests tag as we have strong coverage now.
  • Update the IS removing the Remaining tasks
gambry’s picture

+++ b/core/modules/datetime/datetime.views.inc
@@ -39,6 +39,8 @@ function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
+          'entity_type' => $field_storage->getTargetEntityTypeId(),
+          'field_name' => $field_storage->getName(),

I forgot to mention this bit is for the FieldAPIHandlerTrait on Drupal\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.

pasan.gamage’s picture

Here is the patch for 8.3.x

pasan.gamage’s picture

8.3.2 patch

The last submitted patch, 120: use_form_element_of-2648950-120-8.3.patch, failed testing.

mpdonadio’s picture

@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.

tacituseu’s picture

Status: Needs review » Needs work
FileSize
84.74 KB

Tested #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.

date-exposed-inline-stairs.png

tacituseu’s picture

Digging in a bit most core FormElement's have in ::getInfo():

'#theme_wrappers' => ['form_element']

'datetime' instead does:

'#theme_wrappers' => ['datetime_wrapper']

and the datetime-wrapper.html.twig doesn't add any container to compensate.

skyredwang’s picture

Based 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.

<div class="form-item">
    {% if title %}
      <h4{{ title_attributes.addClass(title_classes) }}>{{ title }}</h4>
    {% endif %}
    {{ content }}
    {% if errors %}
      <div class="form-item--error-message">
        <strong>{{ errors }}</strong>
      </div>
    {% endif %}
    {% if description %}
      <div{{ description_attributes.addClass('description') }}>
        {{ description }}
      </div>
    {% endif %}
</div>

@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.

mpdonadio’s picture

#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).

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tacituseu’s picture

It also doesn't work when 'Expose operator' is checked, the textfields are still showing up (which ones depends on default operator selected).

tacituseu’s picture

stella’s picture

Once 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.

stella’s picture

I 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.

stella’s picture

I'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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

gambry’s picture

Status: Reviewed & tested by the community » Needs review

Hey @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?

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

doesn't work "when 'Expose operator' is checked"

Right! It was not needed for my case so I overlooked that I'm sorry about it.

Also @stella has done some work on #131 to #133, what's the feedback against that work?

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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alanburke’s picture

Updated patch for 8.3.x

matsbla’s picture

Berdir’s picture

FileSize
9.39 KB
83.42 KB

Can 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?

mmbk’s picture

Status: Needs work » Needs review
FileSize
16.52 KB

No 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.

Status: Needs review » Needs work

The last submitted patch, 142: 2648950-142.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mmbk’s picture

Status: Needs work » Needs review
FileSize
16.71 KB
1.15 KB

So, 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.

Status: Needs review » Needs work

The last submitted patch, 144: 2648950-143.patch, failed testing. View results

stella’s picture

My 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

The last submitted patch, 146: 2648950-145-8.6.x.patch, failed testing. View results

stella’s picture

Status: Needs review » Needs work

The last submitted patch, 146: interdiff-2648950-144-145.patch, failed testing. View results

stella’s picture

Status: Needs work » Needs review
FileSize
24.99 KB
33.54 KB

Not sure what happened there, managed to upload an empty patch file. Here's the correct file and interdiff.

mmbk’s picture

@stella this last patch removes the ability to expose a date-only filter, which was included in #139 which ist imho quite important.

hanness’s picture

Is this Module relevant for this issue?

Date Popup

Adds the native HTML 5 date popup widget to all date fields in views filters.

jhedstrom’s picture

Status: Needs review » Needs work

Setting to NW for issues described in #141 and also #151:

@stella this last patch removes the ability to expose a date-only filter, which was included in #139 which ist imho quite important.

jhedstrom’s picture

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -179,4 +180,24 @@ protected function getOffset($time, $timezone) {
+      if ($this->operator === 'between') {

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -188,4 +189,54 @@ protected function opSimple($field) {
+      if ($this->operator === 'between') {

In 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.

jhedstrom’s picture

Status: Needs work » Needs review

This 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.

jhedstrom’s picture

FileSize
1.51 KB
25.08 KB
jhedstrom’s picture

This 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:

diff --git a/core/modules/datetime/src/Plugin/views/filter/Date.php b/core/modules/datetime/src/Plugin/views/filter/Date.php
index 0ef7a0cf7f..da258ab036 100644
--- a/core/modules/datetime/src/Plugin/views/filter/Date.php
+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -191,8 +191,8 @@ public function buildExposedForm(&$form, FormStateInterface $form_state) {
       $field_identifier = $this->options['expose']['identifier'];

       if (in_array($this->operator, ['between', 'not between'])) {
-        $form[$field_identifier]['min']['#date_time_element'] = 'none';
-        $form[$field_identifier]['max']['#date_time_element'] = 'none';
+        $form[$field_identifier . '_wrapper'][$field_identifier]['min']['#date_time_element'] = 'none';
+        $form[$field_identifier . '_wrapper'][$field_identifier]['max']['#date_time_element'] = 'none';
       }
       else {
         $form[$field_identifier]['#date_time_element'] = 'none';
diff --git a/core/modules/views/src/Plugin/views/filter/Date.php b/core/modules/views/src/Plugin/views/filter/Date.php
index d9037ed3ba..a166b3d752 100644
--- a/core/modules/views/src/Plugin/views/filter/Date.php
+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -201,8 +201,8 @@ public function buildExposedForm(&$form, FormStateInterface $form_state) {
       $field_identifier = $this->options['expose']['identifier'];

       if (in_array($this->operator, ['between', 'not between'])) {
-        $form[$field_identifier]['min']['#type'] = 'datetime';
-        $form[$field_identifier]['max']['#type'] = 'datetime';
+        $form[$field_identifier . '_wrapper'][$field_identifier]['min']['#type'] = 'datetime';
+        $form[$field_identifier . '_wrapper'][$field_identifier]['max']['#type'] = 'datetime';

         // Check the element input matches the form structure.
         $input = $form_state->getUserInput();
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Assigning 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 :/

matthieuscarset’s picture

I 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:

  • Create an new node type named Event
  • Add a new field of type Datetime Range (ex: field_dates)
  • Create a View to list all Event nodes
  • Add an exposed filter for the field "field_dates"

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.

GoZ’s picture

Issue summary: View changes
FileSize
33.06 KB

@matthieuscarset i don't think we should deal with datetime_range module on this issue.

As you said:

Hopefully I have missed something here... because not having form element by default for Dates in Drupal 8 is a huge blocker.

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.

GoZ’s picture

Issue summary: View changes
FileSize
41.14 KB

Here is what the filters will look like once #157 will be implemented

GoZ’s picture

remove duplicate comment

jhedstrom’s picture

Assigned: mpdonadio » jhedstrom
Status: Needs review » Needs work

This 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 to 23:59:59 for the max input, and 00:00:00 for the min input I think.

jhedstrom’s picture

With this patch, date-only fields are forced to enter a time

Hmm, actually, this is already done (but I did see it still appearing under certain circumstances, so need to dig deeper).

jhedstrom’s picture

Assigned: jhedstrom » mpdonadio

Sorry 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.

GoZ’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysLisbon
FileSize
37.65 KB
12.16 KB

Here are tests for not_between as exposed by #158.

Done during DevDaysLisbon

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Issue tags: -Needs tests

Thanks @GoZ for the tests!

matsbla’s picture

The 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.

sukanya.ramakrishnan’s picture

Found 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.

sukanya.ramakrishnan’s picture

Status: Needs review » Needs work
sukanya.ramakrishnan’s picture

Status: Needs work » Needs review
FileSize
18.96 KB
48.54 KB

Adding 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.

Status: Needs review » Needs work

The last submitted patch, 172: 2648950-172.patch, failed testing. View results

sukanya.ramakrishnan’s picture

Test 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 !!

jhedstrom’s picture

It 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.

sukanya.ramakrishnan’s picture

@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 !

sukanya.ramakrishnan’s picture

Status: Needs work » Needs review
FileSize
55.45 KB
6.91 KB
17.06 KB

Uploading 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

matsbla’s picture

I'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.

jhedstrom’s picture

Good catch regarding the exposed operator @sukanya.ramakrishnan!

A few comments/questions and nits on #177.

  1. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -51,7 +51,7 @@ class Date extends NumericDate implements ContainerFactoryPluginInterface {
    -   * The request stack used to determine current time.
    +   * The request stack used to determin current time.
    

    This should be changed back to 'determine' :)

  2. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_placeholder_text.yml
    @@ -2,9 +2,9 @@ langcode: en
    -    - core.entity_view_mode.node.teaser
    +  - core.entity_view_mode.node.teaser
    ...
    -    - node
    +  - node
    

    These indentation changes look un-intentional?

  3. +++ b/core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
    @@ -22,7 +22,10 @@ class FilterDateTest extends ViewTestBase {
    +  public static $testViews = ['test_filter_date_between',
    +    'test_filter_date_between_exposed',
    +    'test_filter_date_exposed_operators',
    

    When breaking arrays up onto separate lines, the first item should also be on its own line.

  4. +++ b/core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
    @@ -408,16 +412,21 @@ protected function _testExposedFilterTimestampUI() {
    -    // Test the exposed "not between" filter.
    -    $this->drupalGet('test-filter-date-not-between-exposed');
    +    // Test the exposed "=" filter.
    +    $this->drupalGet('test-filter-date-exposed-operators');
     
    ...
    -    $this->assertFieldByXPath('//input[@id="edit-created-min-date" and @type="date"]', '', 'Found min date input element.');
    -    $this->assertFieldByXPath('//input[@id="edit-created-min-time" and @type="time"]', '', 'Found min time input element.');
    -    $this->assertFieldByXPath('//input[@id="edit-created-max-date" and @type="date"]', '', 'Found max date input element.');
    -    $this->assertFieldByXPath('//input[@id="edit-created-max-time" and @type="time"]', '', 'Found max time input element.');
    +    $this->assertFieldByXPath('//input[@id="edit-created-value-date" and @type="date"]', '', 'Found date input element.');
    +    $this->assertFieldByXPath('//input[@id="edit-created-value-time" and @type="time"]', '', 'Found time input element.');
    

    I'm curious about these test changes. I think it would provide more complete coverage if we test a few different filter/operator combinations.

jhedstrom’s picture

Ah, 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.

jofitz’s picture

Assigned: mpdonadio » Unassigned
FileSize
1.94 KB
54.82 KB

Addressed the comments in #179.

alexfarr’s picture

Hi 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. This will "break" existing sites that use the bef jquery date picker
  2. The HTML5 date element is a floored concept, more on this i a moment.

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.
duplicate datepickers

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.

alexfarr’s picture

I 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:

TypeError: Argument 1 passed to Drupal\Core\Form\FormState::setUserInput() must be of the type array, null given, called in /var/www/project/web/core/modules/views/src/Plugin/views/filter/Date.php on line 253 in file /var/www/project/web/core/lib/Drupal/Core/Form/FormState.php on line 971
Stack trace:
  1. TypeError-&gt;() /var/www/project/web/core/lib/Drupal/Core/Form/FormState.php:971
  2. Drupal\Core\Form\FormState-&gt;setUserInput() /var/www/project/web/core/modules/views/src/Plugin/views/filter/Date.php:253
  3. Drupal\views\Plugin\views\filter\Date-&gt;buildExposedForm() /var/www/project/web/core/modules/datetime/src/Plugin/views/filter/Date.php:187
  4. Drupal\datetime\Plugin\views\filter\Date-&gt;buildExposedForm() /var/www/project/web/modules/contrib/better_exposed_filters/src/Plugin/views/exposed_form/BetterExposedFilters.php:529
  5. Drupal\better_exposed_filters\Plugin\views\exposed_form\BetterExposedFilters-&gt;buildOptionsForm() /var/www/project/web/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:1804
  6. Drupal\views\Plugin\views\display\DisplayPluginBase-&gt;buildOptionsForm() /var/www/project/web/core/modules/views/src/Plugin/views/display/PathPluginBase.php:435
  7. Drupal\views\Plugin\views\display\PathPluginBase-&gt;buildOptionsForm() /var/www/project/web/core/modules/views/src/Plugin/views/display/Page.php:239
  8. Drupal\views\Plugin\views\display\Page-&gt;buildOptionsForm() /var/www/project/web/core/modules/views_ui/src/Form/Ajax/Display.php:74
  9. Drupal\views_ui\Form\Ajax\Display-&gt;buildForm() /var/www/project/web/core/lib/Drupal/Core/Form/FormBuilder.php:518
 10. call_user_func_array() /var/www/project/web/core/lib/Drupal/Core/Form/FormBuilder.php:518
 11. Drupal\Core\Form\FormBuilder-&gt;retrieveForm() /var/www/project/web/core/lib/Drupal/Core/Form/FormBuilder.php:275
 12. Drupal\Core\Form\FormBuilder-&gt;buildForm() /var/www/project/web/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php:214
 13. Drupal\views_ui\Form\Ajax\ViewsFormBase-&gt;Drupal\views_ui\Form\Ajax\{closure}() /var/www/project/web/core/lib/Drupal/Core/Render/Renderer.php:582
 14. Drupal\Core\Render\Renderer-&gt;executeInRenderContext() /var/www/project/web/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php:216
 15. Drupal\views_ui\Form\Ajax\ViewsFormBase-&gt;ajaxFormWrapper() /var/www/project/web/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php:125
 16. Drupal\views_ui\Form\Ajax\ViewsFormBase-&gt;getForm() /var/www/project/web/core/modules/views_ui/src/Form/Ajax/Display.php:46
 17. Drupal\views_ui\Form\Ajax\Display-&gt;getForm() /var/www/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123
 18. call_user_func_array() /var/www/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123
 19. Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() /var/www/project/web/core/lib/Drupal/Core/Render/Renderer.php:582
 20. Drupal\Core\Render\Renderer-&gt;executeInRenderContext() /var/www/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:124
 21. Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext() /var/www/project/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:97
 22. Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() /var/www/project/vendor/symfony/http-kernel/HttpKernel.php:151
 23. Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw() /var/www/project/vendor/symfony/http-kernel/HttpKernel.php:68
 24. Symfony\Component\HttpKernel\HttpKernel-&gt;handle() /var/www/project/web/core/lib/Drupal/Core/StackMiddleware/Session.php:57
 25. Drupal\Core\StackMiddleware\Session-&gt;handle() /var/www/project/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php:47
 26. Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle() /var/www/project/web/core/modules/page_cache/src/StackMiddleware/PageCache.php:99
 27. Drupal\page_cache\StackMiddleware\PageCache-&gt;pass() /var/www/project/web/core/modules/page_cache/src/StackMiddleware/PageCache.php:78
 28. Drupal\page_cache\StackMiddleware\PageCache-&gt;handle() /var/www/project/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:47
 29. Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle() /var/www/project/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php:52
 30. Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle() /var/www/project/web/modules/contrib/whoops/src/StackMiddleware/WhoopsMiddleware.php:43
 31. Drupal\whoops\StackMiddleware\WhoopsMiddleware-&gt;handle() /var/www/project/vendor/stack/builder/src/Stack/StackedHttpKernel.php:23
 32. Stack\StackedHttpKernel-&gt;handle() /var/www/project/web/core/lib/Drupal/Core/DrupalKernel.php:665
 33. Drupal\Core\DrupalKernel-&gt;handle() /var/www/project/web/index.php:19
NickDickinsonWilde’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed 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.

NickDickinsonWilde’s picture

Issue summary: View changes

Draft Change Record created.

The last submitted patch, 166: 2648950-166.patch, failed testing. View results

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

For #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?

  1. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -179,4 +180,38 @@ protected function getOffset($time, $timezone) {
    +      if (in_array($this->operator, ['between', 'not between']) || $this->options['expose']['use_operator']) {
    ...
    +      if ((!in_array($this->operator, ['between', 'not between']))) {
    

    we should use the third argument here

  2. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
    @@ -188,4 +189,70 @@ protected function opSimple($field) {
    +      if (in_array($this->operator, ['between', 'not between']) || $this->options['expose']['use_operator']) {
    ...
    +      if ((!in_array($this->operator, ['between', 'not between']))) {
    ...
    +      if (in_array($this->operator, ['between', 'not between'])) {
    

    same for these

anthonylindsay’s picture

I'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.

FiNeX’s picture

Hi, 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 :-)

pidru’s picture

FiNeX’s picture

@pidru: thanks! I will try it :-)

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Just 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.

jhedstrom’s picture

Status: Needs review » Needs work

Making 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

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.

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)

Riloto’s picture

@pidru Thanks, its works for me!

MNSTR’s picture

Hello

I can't apply patch 181 on Drupal 8.7.1.
It fails. Anyone has got the issue?

rick_p’s picture

Yes, I have the same issue...

https://www.drupal.org/files/issues/2018-11-02/2648950-181.patch (Use form element of type date instead textfield when selecting a date in an exposed filter)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2018-11-02/2648950-181.patch

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.

voleger’s picture

See #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 one https://www.drupal.org/files/issues/2019-03-15/2648950-193.patch.

rick_p’s picture

I 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.

MNSTR’s picture

Thank you voleger, I thought #193 was for D8.8

voleger’s picture

It 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

AaronMcHale’s picture

Thanks 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

imclean’s picture

AaronMcHale’s picture

@imclean thanks :)

cameron prince’s picture

With core 8.7.5, the 193 patch fails on tests/src/Functional/Handler/FilterDateTest.php. Here's a re-roll which fixes this.

dww’s picture

#205 doesn't apply to cleanly to the end of the 8.8.x branch:

% git apply 2648950-205.patch
fatal: corrupt patch at line 1719
% patch -p1 < 2648950-205.patch
patching file core/modules/datetime/src/Plugin/views/filter/Date.php
patching file core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_filter_datetime_exposed.yml
patching file core/modules/datetime/tests/src/Functional/Handler/FilterDateTest.php
patching file core/modules/views/src/Plugin/views/filter/Date.php
patching file core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_date_between_exposed.yml
patching file core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_date_exposed_operators.yml
patching file core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_date_not_between_exposed.yml
patching file core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_placeholder_text.yml
patching file core/modules/views/tests/src/Functional/Handler/FilterDateTest.php
Hunk #3 FAILED at 266.
1 out of 4 hunks FAILED -- saving rejects to file core/modules/views/tests/src/Functional/Handler/FilterDateTest.php.rej
patching file core/modules/views/tests/src/Functional/Handler/FilterPlaceholderTextTest.php
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 39 with fuzz 1.

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.

cameron prince’s picture

I was mistaken with #205... The site I was working against is actually on 8.6.15. Please disregard.

olivier.br’s picture

reroll 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).

Spokje’s picture

Spokje’s picture

Patch #193 with these comments from #187 addressed:

[snipped code fragment about third arg set to true for strict type checking in the in_array calls]
we should use the third argument here
[snipped code fragment about third arg set to true for strict type checking in the in_array calls]
same for these

Not addressed, but also mentioned in #187:

For #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?

Spokje’s picture

Meh, "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.

david.qdoscc’s picture

Would 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.

SocialNicheGuru’s picture

#210 does not apply cleanly to Drupal 8.7.8.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

dww’s picture

Title: Use form element of type date instead textfield when selecting a date in an exposed filter » [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter
Assigned: Unassigned » dww
Issue summary: View changes
Status: Needs work » Postponed
Issue tags: +Needs issue summary update

I 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:

        // If the operator is exposed, the date field is inside the
        // identifier array directly.

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

dww’s picture

Assigned: dww » Unassigned
Issue summary: View changes
Issue tags: +Needs tests
Related issues: +#2419131: [PP-1] #states attribute does not work on #type datetime
FileSize
56.51 KB
56.33 KB
58.55 KB
2.44 KB
4.23 KB
1.25 KB
6.92 KB
7.66 KB
3.34 KB

Okay, 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 and not between. I started trying to add the appropriate nested if() 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:

      // What elements are visible and where they live in the form structure is
      // really complicated. Cases to consider:
      // - Operation is fixed and requires no elements (empty / not empty).
      // - Operation is fixed and requires a single element (=, !=, >=, etc).
      // - Operation is fixed and requires 2 elements (between / not between).
      // - Operation is exposed but limited to one or more of the above.
      // - Operation is exposed and unlimited (we have value, max and min).
      // Instead of trying to code for all of this separately, we see what form
      // elements we have and where they live, and set them to datetime.
      // @see \Drupal\views\Plugin\views\filter\NumericFilter::valueForm()
      $field_identifier = $this->options['expose']['identifier'];
      $value_keys = ['value', 'max', 'min'];
      foreach ($value_keys as $value_key) {
        if (isset($form[$field_identifier]['value'][$value_key]['#type'])) {
          $form[$field_identifier]['value'][$value_key]['#type'] = 'datetime';
        }
        if (isset($form[$field_identifier][$value_key]['#type'])) {
          $form[$field_identifier][$value_key]['#type'] = 'datetime';
        }
      }

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 or 2648950-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:

  1. I think the new logic approach should also be happening in 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.
  2. We clearly could use even more test coverage of all the possible combinations.
  3. We should decide if we really want to remove the placeholders and regexp from the existing filter and potentially break existing views that are using them, or just add a whole new filter plugin with a different name. Then a) site builders can decide which interface they prefer and b) we don't have to worry about upgrade paths. I'm strongly leaning this way, but again, would like some buy-in from other datetime* maintainers before I/we spend the effort.

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.

dww’s picture

Thanks 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).

dww’s picture

Hah, #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...

dww’s picture

This has been bothering me:

      $field_identifier = $this->options['expose']['identifier'];
      $field_wrapper = $field_identifier . '_wrapper';
      $value_keys = ['value', 'max', 'min'];
      foreach ($value_keys as $value_key) {
        if (isset($form[$field_wrapper][$field_identifier]['value'][$value_key]['#type']) && $form[$field_wrapper][$field_identifier]['value'][$value_key]['#type'] === 'textfield') {
          $form[$field_wrapper][$field_identifier]['value'][$value_key]['#type'] = 'datetime';
        }
        if (isset($form[$field_wrapper][$field_identifier][$value_key]['#type']) && $form[$field_wrapper][$field_identifier][$value_key]['#type'] === 'textfield') {
          $form[$field_wrapper][$field_identifier][$value_key]['#type'] = 'datetime';
        }
        if (isset($form[$field_identifier]['value'][$value_key]['#type']) && $form[$field_identifier]['value'][$value_key]['#type'] === 'textfield') {
          $form[$field_identifier]['value'][$value_key]['#type'] = 'datetime';
        }
        if (isset($form[$field_identifier][$value_key]['#type']) && $form[$field_identifier][$value_key]['#type'] === 'textfield') {
          $form[$field_identifier][$value_key]['#type'] = 'datetime';
        }
      }
      if (isset($form[$field_wrapper][$field_identifier]['#type']) && $form[$field_wrapper][$field_identifier]['#type'] === 'textfield') {
        $form[$field_wrapper][$field_identifier]['#type'] = 'datetime';
      }
      if (isset($form[$field_identifier]['#type']) && $form[$field_identifier]['#type'] === 'textfield') {
        $form[$field_identifier]['#type'] = 'datetime';
      }

I couldn't find a recursive Element::children() so I did my own via a protected method on the filter plugins.

I believe this is a lot cleaner. Thoughts?

Note: 2648950.218_219.interdiff.txt is also exactly the interdiff between 2648950-217.newer-logic.8_7_x.patch and 2648950-219.8_7_x.patch.

Edit: Now, even with 2648950-219.changes-for-2625136.DO-NOT-TEST.patch applied, the above becomes just this:

      $field_identifier = $this->options['expose']['identifier'];
      // The form elements might be encased in a wrapper, so check that first.
      $field_wrapper = $field_identifier . '_wrapper';
      if (isset($form[$field_wrapper])) {
        $this->convertTextElementToDatetime($form[$field_wrapper]);
      }
      elseif (isset($form[$field_identifier])) {
        $this->convertTextElementToDatetime($form[$field_identifier]);
      }

Ahhhhh, sweet relief. ;)

jibran’s picture

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -179,4 +181,47 @@ protected function getOffset($time, $timezone) {
+    if ($this->fieldStorageDefinition->get('settings')['datetime_type'] === DateTimeItem::DATETIME_TYPE_DATE) {

This can be changed to ->getSetting('datetime_type')

jibran’s picture

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -179,4 +181,47 @@ protected function getOffset($time, $timezone) {
+    // Hide the date_time_element for date only fields.
+    if ($this->fieldStorageDefinition->get('settings')['datetime_type'] === DateTimeItem::DATETIME_TYPE_DATE) {

I 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?

dww’s picture

@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

jibran’s picture

RE #220: Are we missing test coverage there?

You misunderstand. It already allows you to use datetime for timestamp fields like node changed.

Yeah, my bad I didn't apply the https://www.drupal.org/files/issues/2019-10-27/2648950-219.changes-for-2....

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.

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.

dww’s picture

This 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. ;)

dww’s picture

Title: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter » [PP-3] Use form element of type date instead textfield when selecting a date in an exposed filter
Issue summary: View changes
Related issues: +#3078334: Datetime and Datelist elements should render as fieldsets

#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:

  • We should decide if we really want to remove the placeholders and regexp from the existing filter and potentially break existing views that are using them (#215), or just add a whole new filter plugin with a different name. Then a) site builders can decide which interface they prefer and b) we don't have to worry about upgrade paths.

Thanks,
-Derek

mandclu’s picture

@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.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

larowlan’s picture

Title: [PP-3] Use form element of type date instead textfield when selecting a date in an exposed filter » [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

akalata’s picture

We've been hitting an edge case related to this change in one of our projects related to this patch.

Steps to reproduce:

  1. Install Drupal w/ standard profile.
  2. Add a Date field to a node.
  3. Create a view for these nodes:
    • Create a page display
    • Use Ajax
    • Add the date field as an exposed filter
    • The exposed filter must also have the operation exposed
    • Set the filter to "remember the last selection"
  4. Navigate to the view, and run the filter
  5. Navigate to a different page
  6. Clear the cache (this step wasn't needed in our project but is when reproducing here)
  7. Navigate back to the view

Result:

Error: Cannot use object of type Drupal\Core\Datetime\DrupalDateTime as array in Drupal\Core\Datetime\Element\Datetime::valueCallback() (line 77 of core/lib/Drupal/Core/Datetime/Element/Datetime.php).
Drupal\Core\Datetime\Element\Datetime::valueCallback(Array, Object, Object)
call_user_func_array(Array, Array) (Line: 1267)
Drupal\Core\Form\FormBuilder->handleInputElement('views_exposed_form', Array, Object) (Line: 1003)
Drupal\Core\Form\FormBuilder->doBuildForm('views_exposed_form', Array, Object) (Line: 1073)
...

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.

akalata’s picture

The last submitted patch, 230: 2648950-229.8_9_x.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 231: 2648950-231.8_9_x.patch, failed testing. View results

ravi.shankar’s picture

Fixed failed test of patch #231.

akalata’s picture

Status: Needs work » Needs review
Berdir’s picture

Attempt 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.

Berdir’s picture

FileSize
20.37 KB

The last submitted patch, 236: 2648950-236.patch, failed testing. View results

larowlan’s picture

Re-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, but patch cannot.

No changes at all other than the -M for those using composer patches.

Status: Needs review » Needs work

The last submitted patch, 239: 2648950-239.patch, failed testing. View results

Spokje’s picture

Spokje’s picture

FileSize
789 bytes

attek25 made their first commit to this issue’s fork.

Spokje’s picture

Status: Needs work » Needs review
Spokje’s picture

douggreen’s picture

FileSize
64.07 KB

Attached patch only for 9.1.x.

acbramley’s picture

Needed another reroll for 9.1.x due to a conflict in core/modules/views/tests/src/Functional/Handler/FilterDateTest.php

bkosborne’s picture

For 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.

BhumikaVarshney’s picture

#247 patch applies cleanly without any conflict.
Thanks

quietone’s picture

@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.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kim.pepper’s picture

Issue tags: +#pnx-sprint
KapilV’s picture

ExTexan’s picture

Edit: 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?

andypost’s picture

matsbla’s picture

Status: Needs review » Needs work
FileSize
263.68 KB
187.52 KB

After I applied the patch in #253 the wrapper for the daterange filters are gone and the filters are placed cluttered.

Before patch:
Daterange filter has wrapper

After patch:
Wrapper gone after patch applied

Rob230’s picture

Contrary 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.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joey-santiago’s picture

The 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!

nevergone’s picture

Status: Needs work » Needs review

Please, needs review.

chucksimply’s picture

Patch #253 works great for me on 9.3.3 also! Thanks for the work!

gease’s picture

Status: Needs review » Needs work
FileSize
14.21 KB

The last patch #253 breaks ajax functionality when the operator is also exposed (see the image).

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

romain.sickenberg’s picture

Hello,

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.

romain.sickenberg’s picture

I re-rolled a patch and applied the fixes here with just the changes needed to follow the hierarchy.

ordermind’s picture

I 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.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

leo liao’s picture

In 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:

diff --git a/core/modules/views/views.views.inc b/core/modules/views/views.views.inc
index de79773321..9c7fe8f6e6 100644
--- a/core/modules/views/views.views.inc
+++ b/core/modules/views/views.views.inc
@@ -605,6 +605,9 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
         if ($field_storage->getType() == 'boolean') {
           $filter = 'boolean';
         }
+        elseif ($field_storage->getType() == 'timestamp') {
+          $filter = 'date';
+        }
         break;
 
       case 'blob':
joegraduate’s picture

nod_’s picture

Status: Needs review » Postponed

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 :)

robertoperuzzo’s picture

Status: Postponed » Needs work
FileSize
26.81 KB

I'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)
date filters

So it works, but I don't need the time field in my case. So probably some other work on it is needed.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

shadysamir’s picture

EDIT: 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:

Warning: Undefined array key "value" in Drupal\views\Plugin\views\filter\NumericFilter->acceptExposedInput() (line 426 of /home/shadysamir/work/almanassa/almanassa/web/core/modules/views/src/Plugin/views/filter/NumericFilter.php).

followed by

Warning: Undefined array key "value" in Drupal\views\Plugin\views\filter\Date->acceptExposedInput() (line 161 of /home/shadysamir/work/almanassa/almanassa/web/core/modules/views/src/Plugin/views/filter/Date.php).

both are trying to read this->value['value'] when the array is:

date: "2023-05-01"
time: "00:00:00"
type: "date"

The filter is not applied (not added to the SQL statement)

Drupal 9.5.3

Anybody’s picture

Just 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.

joseph.olstad’s picture

Once this issue is resolved it will unblock also:
#3210945-28: Remove dependency on jquery_ui_datepicker

joseph.olstad’s picture

basvredeling’s picture

Patch #270 no longer applies to 10.1.x