Move views argument date handlers from node module

Awesome change!

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/CreatedFullDate.phpundefined
@@ -2,22 +2,20 @@
+ * Definition of Drupal\views\Plugin\views\argument\CreatedFullDate.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/argument/CreatedYear.phpundefined
@@ -2,21 +2,19 @@
+ * Definition of Drupal\views\Plugin\views\argument\CreatedYear.

We should fix all or none of them if possible.

Awesome, with the assumption that we will add module = 'views' later

Is it ok to remove the prefix or should it be "views_"? Maybe it's me, but "created" looks little odd outside/without node.

Right, didn't know that, thanks.

In #1879496: Do we need to worry about plugin ID collisions? we're talking about namespacing the plugins.

In addition, the fact that the "module" key can be left off for Views is a hack that we should remove.

So let's add 'views_' to the ones we're moving there, and not remove the module key.

Just to be more consistent I would prefer the first patch and adapt if we decide something else later.

Well I definitely vote for the prefix one. I think it'll be more work to add the prefix later, and less work to remove it. But that's just me.

And the problem with the default of 'views' is that if a plugin author forgets the key, it won't blow up on discovery. And because the views module will be loaded, but not always their module, that module_exists call won't happen, and weird bugs will ensue. But that is a separate issue.

There seems to be only one valid way to determinate the winner ;) http://www.googlefight.com/index.php?lang=en_GB&word1=prefix&word2=module

Seriously I don't care much, as long it's not the current name.

Hey, lets do this without views_ prefix and googlefight in #1879496: Do we need to worry about plugin ID collisions?.

I'd be even fine with a date_ prefix: date_day, date_week, ...

Just created an issue to test all these files: #1934558: Create a test for views date handlers

It seems to be that we don't really have a standard in core, so changing our behaviour and have a inconsistent piece seems worse,
so I would vote for the non-prefix variant.

Yeah I agree!

Just a question. Is there anything about this that's specific to a created date/time? Isn't it more dealing with timestamps as a general concept..? And if so, it seems like that would be a better name, no?

Yeah indeed, YearDate would work as well. It might make sense to get in the tests first, as it feels wrong to work on not tested code.

#1934558: Create a test for views date handlers

Renamed all the plugins and files.

Cool, just committed that one. That probably means this needs a re-roll again (sorry!)

Also:

+++ b/core/modules/comment/comment.views.incundefined
@@ -215,7 +215,7 @@ function comment_views_data() {
-      'id' => 'node_created_day',
+      'id' => 'created_day',

Careful. :D

Forgot to rename the created_day one. Sadly you didn't pushed the commit yet, so this is just a rerole for your great review!

Good to know that the tests fail if we move things around.

This looks great now, but does not apply anymore.

We left a todo "remove the node dependency once the handlers are moved to views" in ArgumentDateTest. Can that be fixed here?

Oh nice cleanup!

I think user module dependency came from node so we could drop all of ArgumentDateTest::$modules and ArgumentDateTest::setUp().

Yeah you are right.

#33: drupal-1893906-33.patch queued for re-testing.

Cool, that's much better!

Committed and pushed to 8.x. Thanks!