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: https://github.com/backdrop/backdrop/pull/1928~ PR rebased by @indigoxela: https://github.com/backdrop/backdrop/pull/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.

[stpaultim]

Post in forum says that calendar modules release is blocked by this issue: https://forum.backdropcms.org/forum/calendar-module?page=0%2C0#comment-1666

What needs to happen with this issue/PR? Is it just waiting for tests?

[olafgrabienski]

If I understand the forum post https://forum.backdropcms.org/forum/event-list-view-list-filter-months correctly, it's not possible to filter a view by month in Backdrop. If that's the case and if this issue is the reason: Let's finish this fix. How can I help (other than by writing tests)?

[olafgrabienski]

Labeling as a milestone candidate for the next minor release 1.17. @mazzech and me were recently chatting about this issue. Don't have the coding skills to do so but we're really interested to get this forward.

[docwilmot]

Yes, I needed a calendar once, and tried to finish the port of Calendar module, which is blocked by this. I could not figure out after multiple tries how to do this in contrib without some core modifications. I consider this a bug, and its marked as such, so an earlier milestone is appropriate, according to our rules.

[indigoxela]

I'm all for adding the "Page by date" pager plugin to core.

The tricky part currently is that the PR has merge conflicts (it's pretty old now), but it seems @Al-Rozhkov isn't actively working on it anymore.

[olafgrabienski]

The tricky part currently is that the PR has merge conflicts

Do you think it makes sense to create a new PR? I guess that'd be OK (and welcome) as "Al-Rozhkov removed their assignment on 23 Dec 2017".

[indigoxela]

Do you think it makes sense to create a new PR?

I suspect we have to. Here's a fresh PR

So I rebased the work of @Al-Rozhkov. I might have broken something, though. (I've never rebased work of someone else's fork locally and created a new PR of that.)

Locally the date argument is working, but the pager doesn't show. Hard to tell what's missing, as the original code is pretty old.

[olafgrabienski]

Locally the date argument is working, but the pager doesn't show.

Not sure how to test this. Can you describe a respective view and the expected behavior?

[indigoxela]

Not sure how to test this.

I think it's too early to actually test this. (Doesn't work as expected yet, pager doesn't show.)

But for the curious I created a view in the sandbox, linked in the main menu.

My suspicion is that some variables for the pager aren't populated yet, but this needs some more digging.

[indigoxela]

The pager shows up now. http://3163.backdrop.backdrop.qa.backdropcms.org/date-view-test/2020-06

No clue if / how this actually worked back in 2017. The pager needs to access $min_date and $max_date.

[olafgrabienski]

The pager shows up now.

Great! I've seen a few warnings in DBlog, though:

• Warning: date_timezone_set() expects parameter 1 to be DateTime, boolean given in format_date() (line 2399 of /home/qa/github/backdrop/backdrop/pr/3163/core/includes/common.inc).
• Warning: date_format() expects parameter 1 to be DateTime, boolean given in format_date() (line 2423 of /home/qa/github/backdrop/backdrop/pr/3163/core/includes/common.inc).

Re testing: There is a use case mentioned in the forum which according to one of the replies should be solved by this PR. @mazzech Want to give the PR a try with your use case in mind?

[indigoxela]

Regarding the warnings: these appear when editing the view. Note: the PR still needs work. Functionality seems to be the same now compared to D7, but this needs some closer inspection.

Regarding the use case from forum: Looking at the screenshots there, I'm not sure if that case really needs this sort of pager (hard to tell based only on images).

The "Page by date" plugin is necessary to complete the port of the calendar module, which has a D7 usage of more than 100k.

[indigoxela]

I can't reproduce the php warnings from dblog anymore. So I guess, this is ready for testing. It still needs automated tests, though.

But before implementing them, we might want to discuss if the changes are sufficient now for the calendar module port.

To prevent misunderstandings, here's a screenshot of the pager this Issue is about:

Another thought: 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: https://github.com/backdrop/backdrop/pull/3163/commits/a2c78bc0084199ea98e4b92ab208d3c134513609

That commit was essential to get the plugin working, that's why I was really astounded that it was reported to have worked in 2017. To my understanding that should have been impossible as the pager plugin handler wasn't able to access the protected properties of the argument handler.

[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 https://github.com/backdrop/backdrop/commit/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 https://github.com/backdrop/backdrop/commit/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. :tada:

[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 https://github.com/backdrop/backdrop/pull/3163 into 1.x.