UI fatal caused by views argument handlers no longer being able to provide their own default argument handling

Oh right!

I guess we need a test to ensure that at least the date argument handler works as expected. An thrown exception isn't that nice.

.

Took a look at the fix and the code and I'd like to propose to take this in another direction.

Instead of just skipping past miss configuration, why not just provide the date default_argument_handler?

I can see a case for only providing the 'current date' default argument handler for date fields, but no such restraints are in place for the other default argument handlers, so why make an exception for date? (it's not like providing the current user id to a date field makes sense, but it's possible at the moment).

So I would propose to just get date in line with the other default argument handlers. With some changes to the current setup:
- lose support for "Current node's creation time"
- lose support for "Current node's update time"
no idea why those should have support in core, and if we do want them in core, let's give them proper default argument handlers and tests and such (but i'd say just toss them out)

This probably needs more work but let me know if this would be a good direction to take this.

@Lendude
It is great to see your engagement in views!

Right, you understood why we had this odd code. In an additional issue we could think about a type for argument plugins similar as we have
for display plugins, to limit it.

PS: We have a test.

+++ b/core/modules/views/src/Plugin/views/argument/Date.php
@@ -88,38 +86,11 @@ public static function create(ContainerInterface $container, array $configuratio
+   * Set the deafult date argument to the given argFormat.

Let's fix the 'deafult' typo here :)

@dawehner, learning a ton by diving into views, great fun! Thanks for all the reviewing and great feedback.

fixed typo, added a description to the 'date format' field with a link to php.net

This patch only had cosmetic changes since @dawehner RTBC'd in #7.

Langcode settings needed to go. Rerolled.

Patch still applies, and as #9 notes, this only has cosmetic changes and rerolls since RTBC in #7.

I added a beta phase evaluation to the issue summary.

I also bumped this to major since a fatal error through the UI seems major.

Rerolled, needed to account for #2429447: Use data table as views base table, if available., so base table of the test view changed to node_field_data.

Thanks for adding the beta evaluation.

Back to RTBC assuming bot goes green.

Updated issue summary to reflect the new proposed solution.

So we're removing functionality here that people might be using - are we sure that we want to do that?

Adding default argument handlers for "Current node's creation time" and "Current node's update time" to core doesn't make a lot of sense to me, these are not things you want to be showing for every view (even those not showing nodes at all), and are they really 'core' functionality? (not in my mind, these sounds like fringe cases, and if nodes get dedicated handlers, why not users and other entities?).

So to justify keeping the functionality, I'd say we'd need what @dawehner suggested in #7, some way to limit the default argument handlers shown. But until that exists, I'd feel that getting the 'Current date' working is more important then supporting "Current node's creation time" and "Current node's update time" in core.

So, what to do?
- Toss "Current node's creation time" and "Current node's update time" from core and move them to some contrib module
- Support "Current node's creation time" and "Current node's update time" in core as default argument handlers (probably in the node module?).
- Develop some way to limit the default argument handlers shown and keep supporting "Current node's creation time" and "Current node's update time" in core
- Develop some way to limit the default argument handlers shown and toss supporting "Current node's creation time" and "Current node's update time" from core anyway

I'd be for option 4, but curious what others think.

I found this bug when working on a project. #15 fixed problem for me as I was using "date" default argument.

However, I understand concerns @alexpott had in #18 and agree that we should keep existing features in. Based on that I'd suggest keep "Current node's update time" and "Current node's creation time" and add limit functionality in a follow-up. That would be option 3 in #19 I believe.

Straight reroll

Kickin' the testbot into gear. This still needs work, but lets see if it passes.

This patch adds the handlers for "Current node's update time" and "Current node's creation time". Tested manually but need some real tests, but don't feel like doing that right now.

lets see what this does.

Clean up of #23, "Current node's update time" and "Current node's creation time" handlers are blatant copies of the "current node id" handler. Was looking at using the tests for node->id handler as a base for the two date handlers but alas:

  /**
   * @todo Test node default argument.
   */
  //function testArgumentDefaultNode() {}

So that is gonna take a little more effort then copy/paste.

We have some test coverage for this in #2567773: Can't select "current date" as default option in a date contextual filter, I'm going to move that here.

Ah seems like test coverage for the "date" argument default handler is already ok in this patch.
I'll add some more for "node_changed" and "node_created" and create a follow up for the todo mentioned in #24

How are the argument defaults that fetch something from a current node (like node_created, node_changed and node) actually supposed to work?
What's the current node? When I add the node id in the path it's being seen as the argument value, so my argument_default never gets called.

Even worse, when I use the node_created argument, I'm getting an error saying
Exception: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 165 of /var/www/html/core/lib/Drupal/Component/Datetime/DateTimePlus.php).
That's should probably be a different issue though.

I added the view I'm testing with in case my explanation doesn't make much sense. Maybe I'm just seeing this all wrong.

My manual test case was adding a block display, and showing that block next to a node and showing all nodes that were created on the same date as the node you are watching.

The whole node_created and node_changed feel like such fringe use cases, that's why I argued in #5 to just toss them from core. But @alexpott in #18 seemed to mean that we shouldn't be removing functionality from core anymore (and I see his point).

Yea I figured a block display was the only use case. That makes it pretty annoying to test. Will take a stab at that later today.

Re-roll: After updating to RC1 with this patch and running DB update we got the following error from system.install system_update_8004:

Failed: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "date" plugin does not exist. in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 57 of /var/www/zeitraumexit/src/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

Solution: Since the class CacheablePluginInterface no longer exists it has been replaced with CacheableDependencyInterface.

thanks for the reroll! Let's kick the testbot into gear.

I get this on a 'Created week' with 'provide default value' as 'Current date'. Guess that's the same.

What about Comment, Term and User with 'Current date' as I get the same error there on http://drupal.d8/admin/structure/views/ajax/handler/comment/default/argu...

1. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
   @@ -0,0 +1,35 @@
   +  public function getArgument() {
   +    if (($node = $this->routeMatch->getParameter('node')) && $node instanceof NodeInterface) {
   +      return date($this->options['date_format'], $node->getChangedTime());
   +    }
   +    return FALSE;
   +  }
   +
   

   This should probably use the date.formatter service instead of the native date function? Also elsewhere in the patch.

2. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
   @@ -0,0 +1,83 @@
   +use Drupal\views\Plugin\CacheablePluginInterface;
   +use Drupal\views\Plugin\views\argument_default\Date;
   +use Drupal\node\NodeInterface;
   +use Symfony\Component\DependencyInjection\ContainerInterface;
   +use Drupal\Core\Routing\RouteMatchInterface;
   +
   

   Bad ordering. Should be alphabetical.

3. +++ b/core/modules/views/src/Plugin/views/argument_default/Date.php
   @@ -0,0 +1,74 @@
   +  public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    parent::buildOptionsForm($form, $form_state);
   +    $form['date_format'] = array(
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Date format'),
   +      '#default_value' => $this->options['date_format'],
   +      '#description' => $this->t('Provide the date format you would like to use, see <a href="http://php.net/manual/en/function.date.php" target="_blank">the PHP docs</a> for date formats.'),
   +    );
   +  }
   +
   

   This doesn't make a lot of sense. The argument plugins that use this will define the format that they need, which you can get with the getFormula() method.

4. +++ b/core/modules/views/src/Plugin/views/argument_default/Date.php
   @@ -0,0 +1,74 @@
   +  public function isCacheable() {
   +    return TRUE;
   +  }
   

   Relying on the request date means this isn't properly cacheable.

5. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
   @@ -0,0 +1,35 @@
   +class NodeChanged extends NodeCreated implements CacheablePluginInterface {
   +
   

   This is weird inheritance, and I think you could say it violates SOLID.

6. +++ b/core/modules/views/src/Plugin/views/argument/Date.php
   @@ -86,38 +84,11 @@ public static function create(ContainerInterface $container, array $configuratio
   -    $form['default_argument_type']['#options'] += array('date' => $this->t('Current date'));
   -    $form['default_argument_type']['#options'] += array('node_created' => $this->t("Current node's creation time"));
   -    $form['default_argument_type']['#options'] += array('node_changed' => $this->t("Current node's update time"));
   -  }
   

   Technically, this would introduce a BC break if these handlers existed, and would require a upgrade path. I also think this would be a regression from Drupal 7.

This somewhat of a partial review as I don't think the approach is quite right.

I hope #34 didn't come off as being negative. The patch is good, but at this point the changes to argument/Date.php are a bit radical.

I'm going to work on my objections :)

Still needs test coverage for the node create and changed plugins. This is going to fail. Not sure how/why the NullArgument handler is being used here? Manual testing seems fine.

See what is gong on now. Working on this.

I think this will come up green, but it still needs some test coverage.

- the current date through the date argument plugins works when I do manual testing, but this should have proper test coverage
- the node creation/updated defaults need tests through both paths

I also looked at the exported config a lot, and I think that removing the defaultArgumentForm() is OK and not a BC break nor a regression from Drupal 7. So, we have kinda looped back back to #30, but w/o the settings form + some stuff that I identified. I attached that interdiff for reference.

Feedback appreciated.

No time right now for a proper code review, but on first glance it looks a lot better then it was. I agree that removing the defaultArgumentForm is not a regression from D7, this is how it works in D7 too as far as I can tell. So this looks like the right path to take for minimal/no migration impact.

I feel an update of the issue summary is needed to make clear that we are not really removing functionality (the ability to provide own default handlers), just getting date default argument handlers in line with the plugin system. But can't think of the proper wording right now....

Back to 'needs work' for the tests.

tests for the 'current node X time' default argument handlers.

The new tests look good. I'm a little confused by my own comment, though, from a few months ago:

the current date through the date argument plugins works when I do manual testing, but this should have proper test coverage

Do you think the testing is ArgumentDefaultTest::testArgumentDefaultDate() is adequate? Also

1. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
   @@ -43,7 +43,7 @@ protected function setUp() {
   -  public function testArgumentDefaultPlugin() {
   
   @@ -80,7 +80,7 @@ public function testArgumentDefaultPlugin() {
   -  function testArgumentDefaultNoOptions() {
   +  function ptestArgumentDefaultNoOptions() {
   

2. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
   @@ -104,7 +104,7 @@ function testArgumentDefaultNoOptions() {
   -  function testArgumentDefaultFixed() {
   +  function ptestArgumentDefaultFixed() {
   

3. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
   @@ -43,7 +43,7 @@ protected function setUp() {
   -  public function testArgumentDefaultPlugin() {
   +  public function ptestArgumentDefaultPlugin() {
   

4. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
   @@ -80,7 +80,7 @@ public function testArgumentDefaultPlugin() {
   -  function testArgumentDefaultNoOptions() {
   +  function ptestArgumentDefaultNoOptions() {
   

5. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
   @@ -104,7 +104,7 @@ function testArgumentDefaultNoOptions() {
   -  function testArgumentDefaultFixed() {
   +  function ptestArgumentDefaultFixed() {
        $random = $this->randomMachineName();
   

These tests need to be added back in, and these changes aren't in the interdiff?

@mpdonadio thanks for looking at it!

#42.1-5 sorry, found that I left those in, changed it, rerolled the interdiff and then promptly uploaded the old version of the patch....

The test coverage in ArgumentDefaultTest::testArgumentDefaultDate() matches that of the other default arguments handlers, so I think it should be fine. But since ArgumentDefaultTest is a webtest already anyway extending the tests to do a drupalGet en see if the actual filtering works should be fairly trivial should we feel the need.

duh, .patch interdiff

Re-roll.

+++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
@@ -0,0 +1,113 @@
+ */
+class NodeChanged extends ArgumentDefaultPluginBase implements CacheableDependencyInterface {

IMHO we should implement this not just for nodes but for every entity with EntityChangedInterface

@dawehner you are pointing to the 'Current node changed time' default argument handler, so do you mean make a default argument handler for 'Current comment changed time' and 'Current user changed time'? I don't really see a use case for the node version (we just added those from a BC standpoint), let alone for the other entities. That would just clutter up the default argument selector more.

Or do you mean something else?

Re-rolled the patch from #47 for Drupal 8.1.x

Go testbot, go.

manual reroll to 8.2.x

Oops!

EDIT: The patches in #54 and here were a manual reroll of the patch in #52 as it would no longer apply to 8.1.x, including back when it was tested. (Applied against various commits around June 21, 2016 and failed in two different hunks.)

Patched against 8.1.x-dev (commit a734cfae, "back to -dev" after release of Drupal 8.1.9)
Patched against 8.2.x-dev (commit 0e65dfa4)

EDIT #2: Nevermind, #52 patched with fuzz; I guess I grabbed the wrong one in testing... Ugh...

Assuming the additional tests I queued come up green, I think this is a good reroll. I have this question on a few issues, and still haven't gotten a definitive answer: we have a schema addition, so do we need an empty hook_update_N() to force a rebuild so it gets read in?

I don't know enough to say whether or not hook_update_N() applies. However, I can try to provide additional information based on whether things work without using drush si -y. (I assume that's the threshold as to whether hook_update_N() is necessary.)

I just installed 8.2.x @ 04953c5a and tried to reproduce the original issue. I was presented with the ability to create default arguments listed in the proposed resolution, but actually saving them wouldn't work. I could save with "fixed value" but I could not save (e.g. the Views contextual argument overlay window would not disappear after clicking "Apply") with current date, current node's creation time, or current node's update time.

Then I applied the patch:

[php55@Web ~/local.dev/public_html]$ patch -p1 < views_argument_handlers-2325899-56.patch
patching file core/modules/node/config/schema/node.views.schema.yml
patching file core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
patching file core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
patching file core/modules/node/src/Tests/Views/DateArgumentDefaultTest.php
patching file core/modules/views/config/schema/views.argument_default.schema.yml
patching file core/modules/views/src/Plugin/views/argument/Date.php
patching file core/modules/views/src/Plugin/views/argument_default/Date.php
patching file core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
patching file core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_default_date.yml

And the options for current date, or node creation/update time disappeared.

Afterward, I reversed the patch and the current date and node creation/update options reappeared.

[php55@Web ~/local.dev/public_html]$ patch -p1 -R < views_argument_handlers-2325899-56.patch
patching file core/modules/node/config/schema/node.views.schema.yml
patching file core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
patching file core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
patching file core/modules/node/src/Tests/Views/DateArgumentDefaultTest.php
patching file core/modules/views/config/schema/views.argument_default.schema.yml
patching file core/modules/views/src/Plugin/views/argument/Date.php
patching file core/modules/views/src/Plugin/views/argument_default/Date.php
patching file core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
patching file core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_default_date.yml

I highly doubt this answers the question of "do we need hook_update_N()" but might highlight another issue. Hmm... (I will do more research and attempt to figure out what's going on.

I reinstalled 8.2.x @ 04953c5a with the patch applied prior to installation. (e.g. patch was applied and then drush8 si -y standard --account-name='admin' --account-pass='admin' and everything seems to work. Again, I don't know enough about hook_update_N(), but it appears as though it's needed because the patch applied after Drupal has been installed has had unintentional effects. Installing Drupal with the patch previously applied gave a somewhat different order of default arguments:

EDIT: I was able to save everything and have the Views overlay window disappear when adding a contextual argument.

EDIT 2: Do the previous two comments answer the question of whether or not hook_update_N() is necessary? I hope this is enough information to determine whether or not the update is necessary.

Is there anything else I can do to test this issue and provide more information? Is it just writing an update_hook(), or as @mpdonadio put it, "an empty update_hook()"? (I will try, but will probably fail like 100 times before it works; I R TEH ID10T. Nobody wants 10,000 notifications of failure, let alone a dumbass who can't seem to get core/scripts/run-tests.sh to work correctly without a million failures.)

I'm stupid, but I try... *smirk*

Thank you for the screenshots and manual testing, howards! One thing that really helps the committers is to be able to see these manual test results in the issue summary.

The next step would be to ask in IRC (#drupal-contribute) regarding this issue and see if anyone there is able to answer @mpdonadio's question. Perhaps @dawehner could shed some light?

Updating issue summary with manual test results.

@timplunkett, @alexpott, @dawehner, @xjm and I discussed this in a triage session at DrupalCon Dublin. A fatal in the UI is major priority normally so we are keeping that status.

Is there anything I can do to help the process?

Based on the manual tests, it sounds like we do need the empty update hook to force a cache clear to read the schema additions. Here is a quick patch. Manual testing process would be

- Install 8.2.x (has to be 8.2.x HEAD, update hook won't work otherwise)
- Apply patch
- Run and apply updates from update.php
- Test to see if everything works as expected

I did this quickly, but someone should recheck this methodically.

I reinstalled from HEAD (56188d7f), added a date field to article and created a number of nodes with arbitrary dates (authored on and field_date) from yesterday, today, and tomorrow (local time). Afterward, I added a view and tried to add contextual argument, much to the same previous result. (Views contextual arguments would save/disappear with "fixed value" but not with current date, current node's creation time, or current node's update time.) I applied the patch and checked saving arguments again; similar disappearance as previously demonstrated.

Then, I ran /update.php to run the empty views_update hook and again tried saving with expected results. I was able to save contextual arguments without issue. I was also able to test the Views output.

For the tests, I set up multiple views with Block displays to see which nodes were returned. I set up nine blocks: one for each combination of "authored on", "changed", and an arbitrary field_date against each of "node created time, node changed time, and current date." I used block placement to display all of the blocks on all pages and started browsing through the previously created nodes to see which appeared in which blocks. The nodes appropriately displayed in each of the blocks according to which node was viewed. (e.g. nodes created on Sunday appeared with other nodes created on Sunday, nodes with field_date set to Tuesday appropriately appeared with other nodes' field_dates set to Tuesday. etc.)

Everything seemed to work as expected. It all appears to work well with the empty hook_update() for having the schema reload. I will provide screenshots of methodical testing if necessary, but would take a bit of explanation as to which node is displayed and its expected Views results.

Setting RTBC. Thanks for everything!

+++ b/core/modules/views/src/Plugin/views/argument/Date.php
@@ -34,87 +30,12 @@ class Date extends Formula implements ContainerFactoryPluginInterface {
-  public function defaultArgumentForm(&$form, FormStateInterface $form_state) {
...
-  public function getDefaultArgument($raw = FALSE) {

All of these methods are on the parent class therefore there is no API change here. Nice.

We're adding at least three new views plugins - do we have test coverage for everything?

+++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -125,6 +126,25 @@ public function testArgumentDefaultFixed() {
+ function testArgumentDefaultDate() {

tests the current date

+++ b/core/modules/node/src/Tests/Views/DateArgumentDefaultTest.php
@@ -0,0 +1,93 @@
+ * Contains \Drupal\node\Tests\Views\DateArgumentDefaultTest.

tests both the current node plugins

+++ b/core/modules/views/config/schema/views.argument_default.schema.yml
@@ -1,5 +1,9 @@
+views.argument_default.date:
+  type: boolean
+  label: 'Current date'

we seem to be missing the two 'current node' schema here though.

+++ b/core/modules/node/config/schema/node.views.schema.yml
@@ -73,6 +73,14 @@ views.argument_default.node:
+views.argument_default.node_created:
+  type: sequence
+  label: 'Current node created time'
+
+views.argument_default.node_changed:
+  type: sequence
...
+

oops, just overlooked them!

+++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
@@ -0,0 +1,113 @@
+
+      // The Date argument handlers provide their own format strings, otherwise
+      // use a default.
+      if ($argument instanceof \Drupal\views\Plugin\views\argument\Date) {
+        /** @var \Drupal\views\Plugin\views\argument\Date $argument */
+        $format = $argument->getArgFormat();
+      }
+      else {
+        $format = 'Y-m-d';
+      }

+++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
@@ -0,0 +1,113 @@
+      // The Date argument handlers provide their own format strings, otherwise
+      // use a default.
+      if ($argument instanceof \Drupal\views\Plugin\views\argument\Date) {
+        /** @var \Drupal\views\Plugin\views\argument\Date $argument */
+        $format = $argument->getArgFormat();
+      }
+      else {
+        $format = 'Y-m-d';
+      }

+++ b/core/modules/views/src/Plugin/views/argument_default/Date.php
@@ -0,0 +1,109 @@
+    // The Date argument handlers provide their own format strings, otherwise
+    // use a default.
+    if ($argument instanceof \Drupal\views\Plugin\views\argument\Date) {
+      /** @var \Drupal\views\Plugin\views\argument\Date $argument */
+      $format = $argument->getArgFormat();
+    }
+    else {
+      $format = 'Y-m-d';
+    }

Is this logic tested for each plugin?

This commit…

https://www.drupal.org/commitlog/commit/2/6663917e5c3e2d614ae98777d80dae...

…uses the same update version in `views.install` .

Looks like the patch needs to be updated.

Thanks,

•• matt

This no longer applies, and for conflicts outside the function below.

+++ b/core/modules/views/views.install
@@ -406,5 +406,12 @@ function views_update_8200() {
 /**
+ * Clear cache to ensure schema additions are read.
+ */
+function views_update_8201() {
+  // Empty update to cause a cache rebuild so that schema additions are read.
+}
+
+/**
  * @} End of "addtogroup updates-8.2.0".

This should be changed to an empty hook_post_update_NAME(), since we are trying to avoid empty hook_update_N() in core just to trigger rebuilds.

Re-roll.

Corrected update hook number.

Still needs to be rerolled. From https://www.drupal.org/pift-ci-job/586765 / https://dispatcher.drupalci.org/job/drupal_patches/389/

13:38:36 No syntax errors detected in /var/www/html/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
13:38:36 PHP Fatal error:  Cannot redeclare views_update_8201() (previously declared in /var/www/html/core/modules/views/views.install:413) in /var/www/html/core/modules/views/views.install on line 420
13:38:36 Errors parsing /var/www/html/core/modules/views/views.install

See the comment in #76. Instead of using a hook_update_N() in views.install, this should be changed to a post-update hook in views.post_update.php

Crosspost with #79, but the comment still applies. This should be a post update hook and not and update_N.

Already done.

+++ b/core/modules/views/views.install
@@ -413,5 +413,12 @@ function views_update_8201() {
 /**
+ * Clear cache to ensure schema additions are read.
+ */
+function views_update_8202() {
+  // Empty update to cause a cache rebuild so that schema additions are read.
+}
+
+/**

See the comment near the end of #76. Instead of using a hook_update_N() in views.install, this should be changed to a post-update hook in views.post_update.php. We are really trying to avoid empty hook_update_N just to trigger rebuilds; instead we need to do them in post-update hooks.

Sorry, my mistake, didn't read it properly. Thanks for your patience.

I have applied the patch, and it seems that the 'current node created time' uses Y-M-D, whereas I would think it should use timestamp, or a time including seconds.

First step to addressing @alexpotts feedback in #73, also some CS cleanup.

I have tested the "Current date" contextual filter successfully. Thanks

Rerolled after:

• #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase
• #2369119: Fatal error when trying to save a View with grouped filters using other than string values
• #2860096: Remove api doc groups for updates eg. updates-8.2.x-to-8.3.x

Patch only applies to 8.4.x

patch applies to 8.3.x with fuzz. Here as .txt

argument/Date implements ContainerFactoryPluginInterface

+++ b/core/modules/views/src/Plugin/views/argument/Date.php
@@ -2,11 +2,7 @@
@@ -34,87 +30,12 @@ class Date extends Formula implements ContainerFactoryPluginInterface {

and then we remove the create method, which ContainerFactoryPluginInterface requires

+++ b/core/modules/views/src/Plugin/views/argument/Date.php
@@ -34,87 +30,12 @@ class Date extends Formula implements ContainerFactoryPluginInterface {
-  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
-    return new static(
-      $configuration,
-      $plugin_id,
-      $plugin_definition,
-      $container->get('current_route_match')
-    );
-  }

I'm confused why this is working; I don't see where the create method is coming from now. I am also curious whether this is a BC break. Asking because this has potentially gotten entangled with #2627512: Datetime Views plugins don't support timezones, which was extending this class, and the two patches don't work with each other right now.

Not sure which patch has to do what.

?

The very base plugin class also implements ContainerFactoryPluginInterface and the base form of create() so it doesn't actually need to be added here.

Assuming the other patch goes in first, I think the fix here will be easy.

Rerolled to 8.2.7

#94, 8.2.7 is no longer supported, and 8.3.1 is coming out later today. This issue is currently against 8.4.x.

#88 is the patch that need to be reviewed. Just triggered retests.

#88 no longer applies cleanly in 8.4.x. Changing to Needs Work.

Re-rolled for 8.3.3 (in case anybody, like me, needs the patch on that version)

Above patch was solving issue partially. While executing update 8300 of block_content, I was facing issue "Date" plugin not found. Combined #99 patch and #2567773: Can't select "current date" as default option in a date contextual filter resolved my issue.

Thanks

I can't get the patch from #99 to work. Applied just fine, but the `Current date` default argument isn't available as an option for a regular date field. I tried to change the `default_argument_type` in the exported config file and imported the new config, but then I got the same error again: `The "date" plugin does not exist`.

I ran update.php, and also cleared the Views cache manually, but there's no difference.

8.4.x is the target and #98 is still the one to review, at the very least add -do-not-test.patch to those 'helpful' patches to not disturb the issue.

Applied patch in #106 with drupal-composer, ran update.php and it fixed the issue.

Applied patch in #106 with drupal-composer, ran update.php and it fixed the issue.

+1

+++ b/core/modules/views/src/Plugin/views/argument_default/Date.php
@@ -0,0 +1,105 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getCacheMaxAge() {
+    return 0;
+  }

I think this not getting cached is important enough to warrant a test. Just hit a page that uses this and making sure it's marked uncachable, something like that?

@vaplas This is issue is certainly not fixed. Yesterday I tried to do a contextual filter on the current date (for a date field) and it simply won't work.

I came across this issue in 8.5 and am able to reproduce this on a fresh install of 8.6.x-dev. The patch in #111 did successfully apply and solved the issue. However, some feedback.

I don't think that using a 0 cache age is the correct approach here. The cache age can be determined using the seconds until the date change. This can be done using strtotime('tomorrow') - time(). As users can set individual timezones this should then have the cache context for the current user, and use their timezone to calculate the max cache age.

Also reading through the comments, I noticed #48. I think this is a very valuable thing to consider as well since it would provide wider support for the functionality.

Marking this as needs works since it should either get tests added or have a proper cache context implemented.

The "Date" plugin not found error still exists at least on 8.5.7, reproducable by selecting "Current date" as the argument default for a date contextual filter eg "Content: node.field_event_start_date (year)" where field_event_start_date is a date field.

Confirmed that #111 fixes it.

The "Date" plugin not found error still exists at least on 8.6.3, reprodicable by selecting "Current date" as the argument default for a date contextual filter eg "Content: node.field_event_start_date (year)" where field_event_start_date is a date field.

Confirmed that #111 fixes it.

Can confirm this bug is still present on a very simple 8.6.4 installation today. Path 2325899-111 fixed it for a year/week contextual filter implementation.

Confirmed that #111 fixes it.

> IMHO we should implement this not just for nodes but for every entity with EntityChangedInterface

> Also reading through the comments, I noticed #48. I think this is a very valuable thing to consider as well since it would provide wider support for the functionality.

Yes, but... doing this would require a derivative plugin, which would mean the plugin ID would change from 'node_created' to something like 'entity_created:node', which would require a hook_update_N() to convert existing views that use 'node_created' as their argument default setting. Which rather complicates this patch.

It would be better to leave to a follow-up.

> I don't think that using a 0 cache age is the correct approach here. The cache age can be determined using the seconds until the date change. This can be done using strtotime('tomorrow') - time(). As users can set individual timezones this should then have the cache context for the current user, and use their timezone to calculate the max cache age.

Is that not also out of this issue's scope?

1. +++ b/core/modules/node/config/schema/node.views.schema.yml
   @@ -66,6 +66,14 @@ views.argument.node_vid:
   +views.argument_default.node_created:
   +  type: sequence
   +  label: 'Current node created time'
   +
   +views.argument_default.node_changed:
   +  type: sequence
   +  label: 'Current node changed time'
   +
    views.field.node:
   

   Is this necessary?

   These plugins have no configuration, so isn't it fine for them to just use the wildcard default:

   views.argument_default.*:
     type: mapping
     label: 'Base default argument'
   

   It's what the current_user ViewsArgumentDefault plugin does.

2. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
   @@ -0,0 +1,110 @@
   + * The current node changed argument default handler.
   
   +++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
   @@ -0,0 +1,110 @@
   + * The current node created argument default handler.
   

   While we're rerolling, both of these sentences read badly. Something like 'Provides the created time of the current node as default argument value.' assuming that's under 80 chars.

3. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
   @@ -0,0 +1,110 @@
   +  /**
   +   * Return the current node changed time if a current node can be found.
   +   */
   
   +++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
   @@ -0,0 +1,110 @@
   +  /**
   +   * Return the current node creation time if a current node can be found.
   +   */
   
   +++ b/core/modules/views/src/Plugin/views/argument_default/Date.php
   @@ -0,0 +1,105 @@
   +  /**
   +   * Return the default argument.
   +   */
   

   This method is defined in ArgumentDefaultPluginBase, so the docblocks here should be @inheritdoc.

   For the two node plugins, move the text to an inline comment as it's informative.

Straight reroll for now, just to see what this does now.

Addressed #122.

Also moved the date format in the 'current date default argument' to a class variable so that it is easy to create default arguments for different formats (like month/year/week) by just extending that class and overriding that variable.

...sigh...

Confirmed that #125 fixes it.

+1 for patch #125

Hi,

I had applied the patch with Drupal 8.9.x and it's throwing an error.

I have corrected that and made it compatible with 8.9.x.

Below is my review:

Before applying the patch

1) I have created the view for the 'page' content type and tried to add the 'Authored on' field in CONTEXTUAL FILTERS. But I was not able to apply these filters.
2) Then I reviewed dblog and I notice the below error.

After applying the patch

1) I ran update.php
2) Then I tried to add the 'Authored on' field in CONTEXTUAL FILTERS and I have added this field successfully.
3) Also, the view was working perfectly after applying the patch.

I also checked the patch with the coder module and corrected the errors from the list.

I was unable to fix one error because both class names are the same in core/modules/views/src/Plugin/views/argument_default/Date.php and \Drupal\views\Plugin\views\argument\Date.php

Below is the output screenshot of the coder.

Thanks,
Ren

As @rensingh99 found in #130 and the failing test(s) for 8.9.x (Patch Failed to Apply) in #125 I added ... this needs work.

Hi,

I had missed the patch to attach in my comment. Here is my updated patch

Thanks,
Ren

#125 worked for me

So we have patches in comments

- #125 for D8.7.x RTBCed
- #132 for D8.8.x and D8.9.x still needs work

Hi,

Patch #132 was failing due to fix in Remove the core key from views configuration.

I have corrected that.

Thanks,
Ren

Reroll the patch in #138

Fix coding standard and deprecated assertions

core/modules/views/src/Plugin/views/argument_default/Date.php
85 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements

$defaultTheme is required in DateArgumentDefaultTest

Hi, test out the patch from #145 with drupal core 8.8.6. it works. need to clear cache first to have the filter display. Thanks @jungle
set as RTBC

I'm not sure what version to put this. maybe 8.8.x-dev?

Thanks, but this can't be RTBC if the patch fails to apply.

We need fresh patches for 9.1.x, 9.0.x and 8.9.x.

Quick skim looks like this is probably too potentially disruptive for 8.8.x, but it does have the "Triaged D8 major" tag, so maybe. ;) A separate 8.8.x patch for that branch (or maybe re-test #145 if that's still legit for 8.8x.) would be good, just in case.

Thanks,
-Derek

Reroll from #145.
D9 patch applies cleanly to both 9.0.x and 9.1.x branch.

Fixing test failure and one CS violation of the patch for 9.x in #150

#152 looks like a random failure, re-queued.

Applied #150 to Drupal 8.9.4 to create views for "Today's day in history" with two contextual filters:
Content: node.field_date (day) / Provide default value = Current date
Content: node.field_date (month) / Provide default value = Current date

Works like a charm. Thank you.

@Lendude:

@jungle it seems unrelated changes have snuck in? I see a lot of assertEqual changed to assertEquals going on, it doesn’t seem like any of those changes are relevant?
But I have to admit that is one I left to die in a ditch, not convinced anymore we should do any of that, other then just deleting \Drupal\views\Plugin\views\argument\Date::getDefaultArgument and \Drupal\views\Plugin\views\argument\Date::defaultArgumentForm
This crap hasn’t worked since before 8.0.0, and providing a default argument for ‘the create time of the current node’ has always sounded completely pointless to me (the patch does provide a default argument for the current date which might actually be useful…)

But back when I started working on that it was still pretty much ‘it was in D7 so it must be in D8’, no matter how pointless it was

Changes of assertEqual -> assertEquals were wrong, have not swapped $expected and $actual, my bad. and some of them are out of scope/irrelevant.

The bug exists still in 9.1.x, manually tested. Due to my limited knowledge to views, I have no idea how to push this forward next :(. Thanks @Lendude's replies on slack.

Hi, as one of the maintainers for Calendar I have a question since that contrib is very much dependent on date contextual argument handling:

Is the 9.x patch *supposed* to work for datetime_range too? From my manual testing it doesn't. The error is:

SQLSTATE[22007]: Invalid datetime format: 7 ERROR: invalid value "T1" for "HH24" DETAIL: Value must be an integer.: SELECT "node_field_data"."created" AS "node_field_data_created", "node_field_data"."nid" AS "nid" FROM {node_field_data} "node_field_data" LEFT JOIN {node__field_date_range} "node__field_date_range" ON node_field_data.nid = node__field_date_range.entity_id AND node__field_date_range.deleted = :views_join_condition_0 WHERE ((TO_CHAR((TO_TIMESTAMP(node__field_date_range.field_date_range_value, 'YYYY-MM-DD HH24:MI:SS') + INTERVAL '7200 SECONDS'), 'MM') = :node__field_date_range_field_date_range_value_month)) AND (("node_field_data"."status" = :db_condition_placeholder_1) AND ("node_field_data"."type" IN (:db_condition_placeholder_2))) ORDER BY "node_field_data_created" DESC NULLS LAST LIMIT 10 OFFSET 0; Array ( [:node__field_date_range_field_date_range_value_month] => 10 [:db_condition_placeholder_1] => 1 [:db_condition_placeholder_2] => contact [:views_join_condition_0] => 0 )

I'll try dig in deeper, the HH24:MI:SS format looks odd.

Kind regards,
Eric

Works like a charm. This bug should be fixed for all minor versions of 9.

Reset version according to comment #158 and set to Needs review for test on 9.2.x and both #156 and #157 suggests some may still be wrong

As far as I can see is this a bugfix and not a disruptive change. So this can be merged to the current Drupal branch.

Rerolled the patch to branch 9.1.x as it had conflicts.

Tried to handle the failed test cases.

Thanks @anmolgoyal74 for fixing what I missed. Uploaded a new patch that fixes the deprecation warnings in the 9.2.x branch.

Working like a charm on my website (v. 9.1.4). I hope this will be integrated into Core....

Tried to review this. I am not able to follow all the steps to reproduce in the Issue Summary. The first is fine, but then the second "Set default argument to "Current date"' I can't. There is no indication where to find this value. I then skimmed the IS and I think the current date is available after applying the patch and in a contextual filter?

Add the patch does not apply to 9.3.x.

Reroll for 9.3.x + updated the issue summary with exact steps. No interdiff produced.

@quietone

see the screenshot.

When upgrading from Drupal 8 to Drupal 9, I got the same error: "The "date" plugin does not exist.". This prevented db updates.

I fixed the issue by finding the view in which the current date had been set as a default in a contextual filter, and removing the current date default. It did take a couple of hours, so it would be great if this issue could be fixed, though I don't have expertise to offer in terms of reviewing the patch.

#169 works for me on 9.3.3. Thanks!

#169 works for us

1. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
   @@ -0,0 +1,111 @@
   +/**
   + * Provides the 'changed' time of the current node as default argument value.
   + *
   + * @ingroup views_argument_default_plugins
   + *
   + * @ViewsArgumentDefault(
   + *   id = "node_changed",
   + *   title = @Translation("Current node 'changed' time")
   + * )
   + */
   

   Rather than a specific implementation for nodes it feels as though this should somehow be something generic that leverages \Drupal\Core\Entity\EntityChangedInterface::getChangedTime() - I think we could generate instances for all entity types and do something like \Drupal\Core\Entity\EntityForm::getEntityFromRouteMatch() to get the entity from the route match. I guess this could be a follow-up.

2. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeChanged.php
   @@ -0,0 +1,111 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheMaxAge() {
   +    return Cache::PERMANENT;
   +  }
   
   +++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
   @@ -0,0 +1,111 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCacheMaxAge() {
   +    return Cache::PERMANENT;
   +  }
   

   Are we should about this - I would have thought that it would be best add the node as a cacheable dependency - I think we need to vary by the node's cache tags for example.

3. +++ b/core/modules/node/src/Plugin/views/argument_default/NodeCreated.php
   @@ -0,0 +1,111 @@
   +/**
   + * Provides the created time of the current node as default argument value.
   + *
   + * @ingroup views_argument_default_plugins
   + *
   + * @ViewsArgumentDefault(
   + *   id = "node_created",
   + *   title = @Translation("Current node 'created' time")
   + * )
   + */
   +class NodeCreated extends ArgumentDefaultPluginBase implements CacheableDependencyInterface {
   

   Oddly there is no generic interface for getCreatedTime() so I think this plugin needs to be entity specific.

4. +++ b/core/modules/node/tests/src/Functional/Views/DateArgumentDefaultTest.php
   @@ -0,0 +1,189 @@
   +    // Test that the nodes with a create date in the same month are shown.
   ...
   +    // Test that the nodes with a title in the same create date are shown.
   

   This is the changed date and no the create date.

5. +++ b/core/modules/views/src/Plugin/views/argument/Date.php
   @@ -141,4 +141,14 @@ public function getFormula() {
   +  public function getArgFormat() {
   

   New code should use return type hints.

6. +++ b/core/modules/views/src/Plugin/views/argument_default/Date.php
   @@ -0,0 +1,113 @@
   +    $request_time = $this->request->server->get('REQUEST_TIME');
   

   This should be using the time service and not the REQUEST_TIME server variable.