Add Views "Page by date" pager plugin

[Al-Rozhkov]

Date module was merged to Backdrop core in version 1.2.0

For some reason one views pager plugin wasn't ported (date_views\includes\date_views_plugin_pager.inc). I wonder was it intentional or wasn't. @Graham-72 and me going to finish port of much needed Calendar module which is depend on that pager plugin. So we need to decide how to recover pager plugin.

If removal wasn't intentional, we can restore pager in Backdrop core 1.8.0. Until then we can include pager in Calendar module beta release. At the time of release 1.8.0 we can make stable version of Calendar which will depend on 1.8.0 core.

Respective issue in Calendar queue.

---

PR by @Al-Rozhkov: backdrop/backdrop#1928
PR rebased by @indigoxela: backdrop/backdrop#3163

[Al-Rozhkov]

I have read Add date (field) module to core carefully and looks like it wasn't intentional to exclude something from date_views module. Not only pager plugin is missing. Argument handler for date fields which is responsible for views contextual filters doesn't work, although date_views_argument_handler.inc file is there.

Quote from @quicksketch comment:

Disadvantages: This is a bit rushed and we probably have some bugs that we have introduced.

The port is looks incomplete. I'm going to provide PR to fix this.

[Al-Rozhkov]

I have set up an example view on sandbox site with pager by date: http://1928.backdrop.backdrop.qa.backdropcms.org/page-by-date

Check out "Prev/Next" links on the top of the view.

[klonos]

Thank you for taking initiative on this @Al-Rozhkov 👍

[Al-Rozhkov]

Is it ok to add 1.8.0 milestone for this one?

[jenlampton]

Yes, thank you @Al-Rozhkov for catching the oversight. I'd love to see this get into 1.8! I know this was "missing" from the port of Date module, but it is still adding a feature to core. Going to tag it as a feature request (as well as a bug).

[jenlampton]

I gave the PR a quick code review and left some feedback. Overall, it looks great! There are a few things in the old Drupal code that were not up to snuff with Backdrop standards, a few minor cleanups needed, and I had a few questions. Otherwise, this looks super close!

[quicksketch]

PR needs some adjustments, mostly small though. Moving this to needs work.

[Al-Rozhkov]

Thank you for review. I pushed changes based on your comments.
I'm not sure about deleted variables in date_views_argument_handler_simple class. Everything works without them.

[quicksketch]

I'm not sure about deleted variables in date_views_argument_handler_simple class. Everything works without them.

Yeah, PHP doesn't require you to define properties used within an object, but including them provides you a way to document what is expected within the object. IDE's like PHPStorm also will use the property documentation to provide code-hints, and will hightlight unexpected properties as a possible bug, this can help you catch typos in your property names. We should leave them in place.

I'm not quite sure how to test these changes. For new features like this that is somewhat on the fringe of functionality, we should add new test coverage.

[jenlampton]

Looks like this still needs some work, and with code-freeze in just a few hours I'm going to bump the milestone.

[jenlampton]

Looks like this still needs some work, and with no attention since the last release, I'm going to remove the milestone. Should we get a PR ready we can always add one back.

[olafgrabienski]

Re testing: There is a use case mentioned in the forum which according to one of the replies should be solved by this PR.

Looking at the screenshots there, I'm not sure if that case really needs this sort of pager

Indeed, it doesn't. At least, I was able to build a view for the use case without this PR.

[olafgrabienski]

So I guess, this is ready for testing. (...) But before implementing them, we might want to discuss if the changes are sufficient now for the calendar module port.

@docwilmot Are you maybe up to test the calendar module with this PR?

[docwilmot]

Are you maybe up to test the calendar module with this PR?

Calendar module works almost completely with this PR. Theres a watchdog error, but no obvious loss of function.

It might be possible to move the pager plugin definition to the calendar module (as far as I know the only module that actually uses that pager), but then still at least one core change is needed: backdrop/backdrop@a2c78bc

I would agree with this as well. Havent tried.

[docwilmot]

I'm going to try and see if we can just commit backdrop/backdrop@a2c78bc let Calendar module own all the rest of this code instead

[ghost]

From Zulip chat:

@BWPanda
Was 'page by date' in Calendar or Date in D7?

@docwilmot
Date. We merged Date, but left that out somehow.

@BWPanda
Then I'd say it should be in core. People might have had D7 sites using that functionality from Date without having Calendar installed. It should stay the same.

[klonos]

Then I'd say it should be in core. People might have had D7 sites using that functionality from Date without having Calendar installed. It should stay the same.

I fully agree 👍

[docwilmot]

To give context to the last two comments above, I was able to move all the Page by Date code into Calendar module with backdrop/backdrop@a2c78bc as the only core change (plus renaming date_views.views.inc to date.views.inc which should be done anyway, as this is an error).

So the question was: is there any other project using the Page by Date plugin apart from Calendar? If not then we should move all that code to Calendar and let it be maintained there.

@BWPanda feels since that code was in Date and that Date was merged to core " it should be in core. People might have had D7 sites using that functionality from Date without having Calendar installed. It should stay the same."

I will not pursue moving this code around any further, so as not to waste time. If the consensus is this should be put back into core then this issue deserves at least a "works for me" tag. Wont code review yet, in case the pendulum swings again to putting it in Calendar.

@jenlampton would be good for a tiebreak

[quicksketch]

I think the rationalization that D7 Date owned it previously and it should be in Backdrop's Date port is a fairly good reasoning. It's also entirely possible to set up listings of dates that don't need a "calendar" at all, that are just paged lists of events by month like the one @indigoxela posted a screenshot of.

However, in the current PR, there is a swath of code related to setting config values such as:

    "date_views_month_format_with_year": "F Y",
    "date_views_month_format_without_year": "F",
    "date_views_day_format_with_year": "l, F j, Y",
    "date_views_day_format_without_year": "l, F j",
    "date_views_week_format_with_year": "F j, Y",
    "date_views_week_format_without_year": "F j"

These do not appear to be used anywhere in the PR. If they're necessary only for Calendar, then those perhaps should be moved to Calendar module. But as far as I can see, they might not be used anywhere at all.

[jenlampton]

I also think it should be in core, since it was in Date in Drupal 7. Even if calendar was the only contrib module that used the functionality, that doesn't mean that people building views with dates aren't also using it.

[indigoxela]

However, in the current PR, there is a swath of code related to setting config values such as ... These do not appear to be used anywhere

It appears to - but these variables are used. Here's the D7 commit that brought that setting in.
It's used in function theme_date_nav_title().

Regarding renaming of date_views.views.inc to date.views.inc - that maybe makes sense, but I didn't dare because of possible glitches. Are there any? (Apart of the fact that the old file won't get removed on updates.)

[indigoxela]

Phew, we have functional tests now. 🎉

[herbdool]

I think the code looks good. Since it's a major chunk of code I propose moving it to 1.17.0. An added benefit is that it's one more new feature for the blog post 😉

[herbdool]

I've also tested now and it works well.

[ghost]

Thanks for everyone's work on this! Especially @indigoxela, @Al-Rozhkov, @jenlampton, @quicksketch, @olafgrabienski, @docwilmot & @herbdool.. I've merged backdrop/backdrop#3163 into 1.x.